Re-enable WeakPtr ThreadChecker and fix associated failures (flutter#12257)

This re-enables thread safety checks for WeakPtr. WeakPtrs can't be used on a thread other than the one the WeakPtrFactory was created on.

This fixes the unit tests and adds a getUnsafe() method to WeakPtr to work around the remaining unresolved locations where we are using WeakPtr unsafely.

Author: George Wright

fml/memory/thread_checker.h

@@ -55,9 +55,7 @@ class ThreadChecker final {
 #endif
 };
 
-// TODO(chinmaygarde): Re-enable this after auditing all new users of
-// fml::WeakPtr.
-#if !defined(NDEBUG) && false
+#if !defined(NDEBUG)
 #define FML_DECLARE_THREAD_CHECKER(c) fml::ThreadChecker c
 #define FML_DCHECK_CREATION_THREAD_IS_CURRENT(c) \
   FML_DCHECK((c).IsCreationThreadCurrent())

fml/memory/weak_ptr.h

@@ -84,6 +84,18 @@ class WeakPtr {
     return *this ? ptr_ : nullptr;
   }
 
+  // TODO(gw280): Remove all remaining usages of getUnsafe().
+  // No new usages of getUnsafe() are allowed.
+  //
+  // https://github.com/flutter/flutter/issues/42949
+  T* getUnsafe() const {
+    // This is an unsafe method to get access to the raw pointer.
+    // We still check the flag_ to determine if the pointer is valid
+    // but callees should note that this WeakPtr could have been
+    // invalidated on another thread.
+    return flag_ && flag_->is_valid() ? ptr_ : nullptr;
+  }
+
   T& operator*() const {
     FML_DCHECK_CREATION_THREAD_IS_CURRENT(checker_.checker);
     FML_DCHECK(*this);

lib/ui/painting/image_decoder_unittests.cc

@@ -168,11 +168,15 @@ TEST_F(ImageDecoderFixtureTest, ValidImageResultsInSuccess) {
   fml::AutoResetWaitableEvent latch;
 
   std::unique_ptr<IOManager> io_manager;
-  std::unique_ptr<ImageDecoder> image_decoder;
 
+  auto release_io_manager = [&]() {
+    io_manager.reset();
+    latch.Signal();
+  };
   auto decode_image = [&]() {
-    image_decoder = std::make_unique<ImageDecoder>(
-        runners, loop->GetTaskRunner(), io_manager->GetWeakIOManager());
+    std::unique_ptr<ImageDecoder> image_decoder =
+        std::make_unique<ImageDecoder>(runners, loop->GetTaskRunner(),
+                                       io_manager->GetWeakIOManager());
 
     ImageDecoder::ImageDescriptor image_descriptor;
     image_descriptor.data = OpenFixtureAsSkData("DashInNooglerHat.jpg");
@@ -183,7 +187,7 @@ TEST_F(ImageDecoderFixtureTest, ValidImageResultsInSuccess) {
     ImageDecoder::ImageResult callback = [&](SkiaGPUObject<SkImage> image) {
       ASSERT_TRUE(runners.GetUITaskRunner()->RunsTasksOnCurrentThread());
       ASSERT_TRUE(image.get());
-      latch.Signal();
+      runners.GetIOTaskRunner()->PostTask(release_io_manager);
     };
     image_decoder->Decode(std::move(image_descriptor), callback);
   };
@@ -210,12 +214,17 @@ TEST_F(ImageDecoderFixtureTest, ExifDataIsRespectedOnDecode) {
   fml::AutoResetWaitableEvent latch;
 
   std::unique_ptr<IOManager> io_manager;
-  std::unique_ptr<ImageDecoder> image_decoder;
+
+  auto release_io_manager = [&]() {
+    io_manager.reset();
+    latch.Signal();
+  };
 
   SkISize decoded_size = SkISize::MakeEmpty();
   auto decode_image = [&]() {
-    image_decoder = std::make_unique<ImageDecoder>(
-        runners, loop->GetTaskRunner(), io_manager->GetWeakIOManager());
+    std::unique_ptr<ImageDecoder> image_decoder =
+        std::make_unique<ImageDecoder>(runners, loop->GetTaskRunner(),
+                                       io_manager->GetWeakIOManager());
 
     ImageDecoder::ImageDescriptor image_descriptor;
     image_descriptor.data = OpenFixtureAsSkData("Horizontal.jpg");
@@ -227,7 +236,7 @@ TEST_F(ImageDecoderFixtureTest, ExifDataIsRespectedOnDecode) {
       ASSERT_TRUE(runners.GetUITaskRunner()->RunsTasksOnCurrentThread());
       ASSERT_TRUE(image.get());
       decoded_size = image.get()->dimensions();
-      latch.Signal();
+      runners.GetIOTaskRunner()->PostTask(release_io_manager);
     };
     image_decoder->Decode(std::move(image_descriptor), callback);
   };
@@ -257,11 +266,16 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithoutAGPUContext) {
   fml::AutoResetWaitableEvent latch;
 
   std::unique_ptr<IOManager> io_manager;
-  std::unique_ptr<ImageDecoder> image_decoder;
+
+  auto release_io_manager = [&]() {
+    io_manager.reset();
+    latch.Signal();
+  };
 
   auto decode_image = [&]() {
-    image_decoder = std::make_unique<ImageDecoder>(
-        runners, loop->GetTaskRunner(), io_manager->GetWeakIOManager());
+    std::unique_ptr<ImageDecoder> image_decoder =
+        std::make_unique<ImageDecoder>(runners, loop->GetTaskRunner(),
+                                       io_manager->GetWeakIOManager());
 
     ImageDecoder::ImageDescriptor image_descriptor;
     image_descriptor.data = OpenFixtureAsSkData("DashInNooglerHat.jpg");
@@ -272,7 +286,7 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithoutAGPUContext) {
     ImageDecoder::ImageResult callback = [&](SkiaGPUObject<SkImage> image) {
       ASSERT_TRUE(runners.GetUITaskRunner()->RunsTasksOnCurrentThread());
       ASSERT_TRUE(image.get());
-      latch.Signal();
+      runners.GetIOTaskRunner()->PostTask(release_io_manager);
     };
     image_decoder->Decode(std::move(image_descriptor), callback);
   };
@@ -355,6 +369,20 @@ TEST_F(ImageDecoderFixtureTest, CanDecodeWithResizes) {
   ASSERT_EQ(decoded_size(100, {}), SkISize::Make(100, 133));
   ASSERT_EQ(decoded_size({}, 100), SkISize::Make(75, 100));
   ASSERT_EQ(decoded_size(100, 100), SkISize::Make(100, 100));
+
+  // Destroy the IO manager
+  runners.GetIOTaskRunner()->PostTask([&]() {
+    io_manager.reset();
+    latch.Signal();
+  });
+  latch.Wait();
+
+  // Destroy the image decoder
+  runners.GetUITaskRunner()->PostTask([&]() {
+    image_decoder.reset();
+    latch.Signal();
+  });
+  latch.Wait();
 }
 
 TEST_F(ImageDecoderFixtureTest, CanResizeWithoutDecode) {
@@ -441,6 +469,20 @@ TEST_F(ImageDecoderFixtureTest, CanResizeWithoutDecode) {
   ASSERT_EQ(decoded_size(100, {}), SkISize::Make(100, 133));
   ASSERT_EQ(decoded_size({}, 100), SkISize::Make(75, 100));
   ASSERT_EQ(decoded_size(100, 100), SkISize::Make(100, 100));
+
+  // Destroy the IO manager
+  runners.GetIOTaskRunner()->PostTask([&]() {
+    io_manager.reset();
+    latch.Signal();
+  });
+  latch.Wait();
+
+  // Destroy the image decoder
+  runners.GetUITaskRunner()->PostTask([&]() {
+    image_decoder.reset();
+    latch.Signal();
+  });
+  latch.Wait();
 }
 
 }  // namespace testing

lib/ui/ui_dart_state.cc

@@ -79,10 +79,16 @@ const TaskRunners& UIDartState::GetTaskRunners() const {
 }
 
 fml::RefPtr<flutter::SkiaUnrefQueue> UIDartState::GetSkiaUnrefQueue() const {
-  if (!io_manager_) {
+  // TODO(gw280): The WeakPtr here asserts that we are derefing it on the
+  // same thread as it was created on. As we can't guarantee that currently
+  // we're being called from the IO thread (construction thread), we need
+  // to use getUnsafe() here to avoid failing the assertion.
+  //
+  // https://github.com/flutter/flutter/issues/42946
+  if (!io_manager_.getUnsafe()) {
     return nullptr;
   }
-  return io_manager_->GetSkiaUnrefQueue();
+  return io_manager_.getUnsafe()->GetSkiaUnrefQueue();
 }
 
 void UIDartState::ScheduleMicrotask(Dart_Handle closure) {
@@ -114,6 +120,7 @@ void UIDartState::AddOrRemoveTaskObserver(bool add) {
 }
 
 fml::WeakPtr<GrContext> UIDartState::GetResourceContext() const {
+  FML_DCHECK(task_runners_.GetIOTaskRunner()->RunsTasksOnCurrentThread());
   if (!io_manager_) {
     return {};
   }

lib/ui/ui_dart_state.h

@@ -14,6 +14,7 @@
 #include "flutter/flow/skia_gpu_object.h"
 #include "flutter/fml/build_config.h"
 #include "flutter/fml/memory/weak_ptr.h"
+#include "flutter/fml/synchronization/waitable_event.h"
 #include "flutter/lib/ui/io_manager.h"
 #include "flutter/lib/ui/isolate_name_server/isolate_name_server.h"
 #include "flutter/lib/ui/painting/image_decoder.h"

shell/common/input_events_unittests.cc

@@ -127,7 +127,14 @@ static void TestSimulatedInputEvents(
   });
 
   simulation.wait();
-  shell.reset();
+
+  TaskRunners task_runners = fixture->GetTaskRunnersForFixture();
+  fml::AutoResetWaitableEvent latch;
+  task_runners.GetPlatformTaskRunner()->PostTask([&shell, &latch]() mutable {
+    shell.reset();
+    latch.Signal();
+  });
+  latch.Wait();
 
   // Make sure that all events have been consumed so
   // https://github.com/flutter/flutter/issues/40863 won't happen again.

shell/common/persistent_cache_unittests.cc

@@ -87,7 +87,7 @@ TEST_F(ShellTest, CacheSkSLWorks) {
   settings.dump_skp_on_shader_compilation = true;
   auto normal_config = RunConfiguration::InferFromSettings(settings);
   normal_config.SetEntrypoint("emptyMain");
-  shell.reset();
+  DestroyShell(std::move(shell));
   shell = CreateShell(settings);
   PlatformViewNotifyCreated(shell.get());
   RunEngine(shell.get(), std::move(normal_config));
@@ -120,6 +120,7 @@ TEST_F(ShellTest, CacheSkSLWorks) {
     return true;
   };
   fml::VisitFiles(dir.fd(), remove_visitor);
+  DestroyShell(std::move(shell));
 }
 
 }  // namespace testing

shell/common/shell.cc

@@ -90,6 +90,13 @@ std::unique_ptr<Shell> Shell::CreateShellOnPlatformThread(
   std::promise<fml::WeakPtr<ShellIOManager>> weak_io_manager_promise;
   auto weak_io_manager_future = weak_io_manager_promise.get_future();
   auto io_task_runner = shell->GetTaskRunners().GetIOTaskRunner();
+
+  // TODO(gw280): The WeakPtr here asserts that we are derefing it on the
+  // same thread as it was created on. We are currently on the IO thread
+  // inside this lambda but we need to deref the PlatformView, which was
+  // constructed on the platform thread.
+  //
+  // https://github.com/flutter/flutter/issues/42948
   fml::TaskRunner::RunNowOrPostTask(
       io_task_runner,
       [&io_manager_promise,                          //
@@ -99,7 +106,7 @@ std::unique_ptr<Shell> Shell::CreateShellOnPlatformThread(
   ]() {
         TRACE_EVENT0("flutter", "ShellSetupIOSubsystem");
         auto io_manager = std::make_unique<ShellIOManager>(
-            platform_view->CreateResourceContext(), io_task_runner);
+            platform_view.getUnsafe()->CreateResourceContext(), io_task_runner);
         weak_io_manager_promise.set_value(io_manager->GetWeakPtr());
         io_manager_promise.set_value(std::move(io_manager));
       });
@@ -277,11 +284,21 @@ Shell::Shell(DartVMRef vm, TaskRunners task_runners, Settings settings)
     : task_runners_(std::move(task_runners)),
       settings_(std::move(settings)),
       vm_(std::move(vm)),
-      weak_factory_(this) {
+      weak_factory_(this),
+      weak_factory_gpu_(nullptr) {
   FML_CHECK(vm_) << "Must have access to VM to create a shell.";
   FML_DCHECK(task_runners_.IsValid());
   FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());
 
+  // Generate a WeakPtrFactory for use with the GPU thread. This does not need
+  // to wait on a latch because it can only ever be used from the GPU thread
+  // from this class, so we have ordering guarantees.
+  fml::TaskRunner::RunNowOrPostTask(
+      task_runners_.GetGPUTaskRunner(), fml::MakeCopyable([this]() mutable {
+        this->weak_factory_gpu_ =
+            std::make_unique<fml::WeakPtrFactory<Shell>>(this);
+      }));
+
   // Install service protocol handlers.
 
   service_protocol_handlers_[ServiceProtocol::kScreenshotExtensionName] = {
@@ -331,11 +348,13 @@ Shell::~Shell() {
 
   fml::TaskRunner::RunNowOrPostTask(
       task_runners_.GetGPUTaskRunner(),
-      fml::MakeCopyable(
-          [rasterizer = std::move(rasterizer_), &gpu_latch]() mutable {
-            rasterizer.reset();
-            gpu_latch.Signal();
-          }));
+      fml::MakeCopyable([rasterizer = std::move(rasterizer_),
+                         weak_factory_gpu = std::move(weak_factory_gpu_),
+                         &gpu_latch]() mutable {
+        rasterizer.reset();
+        weak_factory_gpu.reset();
+        gpu_latch.Signal();
+      }));
   gpu_latch.Wait();
 
   fml::TaskRunner::RunNowOrPostTask(
@@ -393,9 +412,6 @@ void Shell::RunEngine(RunConfiguration run_configuration,
   FML_DCHECK(is_setup_);
   FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());
 
-  if (!weak_engine_) {
-    result(Engine::RunStatus::Failure);
-  }
   fml::TaskRunner::RunNowOrPostTask(
       task_runners_.GetUITaskRunner(),
       fml::MakeCopyable(
@@ -486,8 +502,13 @@ bool Shell::Setup(std::unique_ptr<PlatformView> platform_view,
   PersistentCache::GetCacheForProcess()->SetIsDumpingSkp(
       settings_.dump_skp_on_shader_compilation);
 
-  // Shell::Setup is running on the UI thread so we can do the following.
-  display_refresh_rate_ = weak_engine_->GetDisplayRefreshRate();
+  // TODO(gw280): The WeakPtr here asserts that we are derefing it on the
+  // same thread as it was created on. Shell is constructed on the platform
+  // thread but we need to call into the Engine on the UI thread, so we need
+  // to use getUnsafe() here to avoid failing the assertion.
+  //
+  // https://github.com/flutter/flutter/issues/42947
+  display_refresh_rate_ = weak_engine_.getUnsafe()->GetDisplayRefreshRate();
 
   return true;
 }
@@ -1069,7 +1090,7 @@ void Shell::OnFrameRasterized(const FrameTiming& timing) {
     // never be reported until the next animation starts.
     frame_timings_report_scheduled_ = true;
     task_runners_.GetGPUTaskRunner()->PostDelayedTask(
-        [self = weak_factory_.GetWeakPtr()]() {
+        [self = weak_factory_gpu_->GetWeakPtr()]() {
           if (!self.get()) {
             return;
           }

shell/common/shell.h

@@ -74,7 +74,8 @@ enum class DartErrorCode {
 /// platform task runner. In case the embedder wants to directly access a shell
 /// subcomponent, it is the embedder's responsibility to acquire a weak pointer
 /// to that component and post a task to the task runner used by the component
-/// to access its methods.
+/// to access its methods. The shell must also be destroyed on the platform
+/// task runner.
 ///
 /// There is no explicit API to bootstrap and shutdown the Dart VM. The first
 /// instance of the shell in the process bootstraps the Dart VM and the
@@ -516,6 +517,10 @@ class Shell final : public PlatformView::Delegate,
 
   fml::WeakPtrFactory<Shell> weak_factory_;
 
+  // For accessing the Shell via the GPU thread, necessary for various
+  // rasterizer callbacks.
+  std::unique_ptr<fml::WeakPtrFactory<Shell>> weak_factory_gpu_;
+
   friend class testing::ShellTest;
 
   FML_DISALLOW_COPY_AND_ASSIGN(Shell);

shell/common/shell_benchmarks.cc

@@ -78,7 +78,15 @@ static void StartupAndShutdownShell(benchmark::State& state,
 
   {
     benchmarking::ScopedPauseTiming pause(state, !measure_shutdown);
-    shell.reset();  // Shutdown is synchronous.
+    // Shutdown must occur synchronously on the platform thread.
+    fml::AutoResetWaitableEvent latch;
+    fml::TaskRunner::RunNowOrPostTask(
+        thread_host->platform_thread->GetTaskRunner(),
+        [&shell, &latch]() mutable {
+          shell.reset();
+          latch.Signal();
+        });
+    latch.Wait();
     thread_host.reset();
   }