From 2831e61a9dfd9d7845e1c45480ed0bc9e134c8bb Mon Sep 17 00:00:00 2001
From: Jason Simmons <jason-simmons@users.noreply.github.com>
Date: Mon, 4 Aug 2025 10:58:57 -0700
Subject: [PATCH] [Impeller] Terminate the fence waiter but do not reset it
 during ContextVK shutdown (#173085)

ContextVK::Shutdown was clearing the context's fence_waiter_. This could
cause crashes if a pending task such as image decoding is still holding
a reference to the context after ContextVK::Shutdown is called.

The image decode task will submit a command buffer through the context,
and CommandQueueVK::Submit will get a null pointer deference when it
tries to use the fence waiter.

This PR changes ContextVK::Shutdown to terminate the fence waiter
instead of clearing it. FenceWaiterVK::Terminate will now stop the
waiter thread and wait for the thread to exit.

This ensures that the fence waiter thread is stopped in
ContextVK::Shutdown even if something else is holding a reference to the
FenceWaiterVK. Tasks like image decoding will now get an error result
instead of a crash when submitting a command buffer after context
shutdown.
---
 .../impeller/renderer/backend/vulkan/context_vk.cc |  2 +-
 .../renderer/backend/vulkan/fence_waiter_vk.cc     |  5 ++++-
 .../backend/vulkan/fence_waiter_vk_unittests.cc    | 14 ++++++++++----
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc
index d75db4942bb..3220f7ec946 100644
--- a/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc
+++ b/engine/src/flutter/impeller/renderer/backend/vulkan/context_vk.cc
@@ -601,7 +601,7 @@ void ContextVK::Shutdown() {
   // pointers ensures that cleanup happens in a correct order.
   //
   // tl;dr: Without it, we get thread::join failures on shutdown.
-  fence_waiter_.reset();
+  fence_waiter_->Terminate();
   resource_manager_.reset();
 
   raster_message_loop_->Terminate();
diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.cc
index ddd617a5803..73755abbc7e 100644
--- a/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.cc
+++ b/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.cc
@@ -59,7 +59,6 @@ FenceWaiterVK::FenceWaiterVK(std::weak_ptr<DeviceHolderVK> device_holder)
 
 FenceWaiterVK::~FenceWaiterVK() {
   Terminate();
-  waiter_thread_->join();
 }
 
 bool FenceWaiterVK::AddFence(vk::UniqueFence fence,
@@ -207,9 +206,13 @@ bool FenceWaiterVK::Wait() {
 void FenceWaiterVK::Terminate() {
   {
     std::scoped_lock lock(wait_set_mutex_);
+    if (terminate_) {
+      return;
+    }
     terminate_ = true;
   }
   wait_set_cv_.notify_one();
+  waiter_thread_->join();
 }
 
 }  // namespace impeller
diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc
index ef2ff5d40cb..6998dbc2da9 100644
--- a/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc
+++ b/engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc
@@ -116,14 +116,20 @@ TEST(FenceWaiterVKTest, InProgressFencesStillWaitIfTerminated) {
   raw_fence = MockFence::GetRawPointer(fence);
   waiter->AddFence(std::move(fence), [&signal]() { signal.Signal(); });
 
-  // Terminate the waiter.
-  waiter->Terminate();
+  // Signal the fence after a delay.
+  std::thread thread([&]() {
+    std::this_thread::sleep_for(std::chrono::milliseconds{100u});
+    raw_fence->SetStatus(vk::Result::eSuccess);
+  });
 
-  // Signal the fence.
-  raw_fence->SetStatus(vk::Result::eSuccess);
+  // Terminate the waiter.  This will block until all pending fences are
+  // signalled.
+  waiter->Terminate();
 
   // This will hang if the fence was not signalled.
   signal.Wait();
+
+  thread.join();
 }
 
 }  // namespace testing