Avoid deadlock while creating platform views with multiple engines. (flutter#28224)

eggfly · web-flow · commit db593547a887 · 2021-08-23T08:57:02.000-07:00
diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc
@@ -23,8 +23,7 @@ RasterThreadMerger::RasterThreadMerger(
     fml::TaskQueueId gpu_queue_id)
     : platform_queue_id_(platform_queue_id),
       gpu_queue_id_(gpu_queue_id),
-      shared_merger_(shared_merger),
-      enabled_(true) {}
+      shared_merger_(shared_merger) {}
 
 void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) {
   merge_unmerge_callback_ = callback;
@@ -119,12 +118,12 @@ bool RasterThreadMerger::IsMerged() {
 
 void RasterThreadMerger::Enable() {
   std::scoped_lock lock(mutex_);
-  enabled_ = true;
+  shared_merger_->SetEnabledUnSafe(true);
 }
 
 void RasterThreadMerger::Disable() {
   std::scoped_lock lock(mutex_);
-  enabled_ = false;
+  shared_merger_->SetEnabledUnSafe(false);
 }
 
 bool RasterThreadMerger::IsEnabled() {
@@ -133,7 +132,7 @@ bool RasterThreadMerger::IsEnabled() {
 }
 
 bool RasterThreadMerger::IsEnabledUnSafe() const {
-  return enabled_;
+  return shared_merger_->IsEnabledUnSafe();
 }
 
 bool RasterThreadMerger::IsMergedUnSafe() const {
diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h
@@ -127,7 +127,6 @@ class RasterThreadMerger
   std::condition_variable merged_condition_;
   std::mutex mutex_;
   fml::closure merge_unmerge_callback_;
-  bool enabled_;
 
   bool IsMergedUnSafe() const;
 
diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc
@@ -361,6 +361,27 @@ TEST(RasterThreadMerger, IsEnabled) {
   ASSERT_TRUE(raster_thread_merger->IsEnabled());
 }
 
+TEST(RasterThreadMerger, TwoMergersWithSameThreadPairShareEnabledState) {
+  TaskQueueWrapper queue1;
+  TaskQueueWrapper queue2;
+  fml::TaskQueueId qid1 = queue1.GetTaskQueueId();
+  fml::TaskQueueId qid2 = queue2.GetTaskQueueId();
+  const auto merger1 =
+      RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2);
+  const auto merger2 =
+      RasterThreadMerger::CreateOrShareThreadMerger(merger1, qid1, qid2);
+  ASSERT_TRUE(merger1->IsEnabled());
+  ASSERT_TRUE(merger2->IsEnabled());
+
+  merger1->Disable();
+  ASSERT_FALSE(merger1->IsEnabled());
+  ASSERT_FALSE(merger2->IsEnabled());
+
+  merger2->Enable();
+  ASSERT_TRUE(merger1->IsEnabled());
+  ASSERT_TRUE(merger2->IsEnabled());
+}
+
 TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) {
   fml::MessageLoop* loop_platform = nullptr;
   fml::AutoResetWaitableEvent latch1;
diff --git a/fml/shared_thread_merger.cc b/fml/shared_thread_merger.cc
@@ -14,7 +14,8 @@ SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner,
                                        fml::TaskQueueId subsumed)
     : owner_(owner),
       subsumed_(subsumed),
-      task_queues_(fml::MessageLoopTaskQueues::GetInstance()) {}
+      task_queues_(fml::MessageLoopTaskQueues::GetInstance()),
+      enabled_(true) {}
 
 bool SharedThreadMerger::MergeWithLease(RasterThreadMergerId caller,
                                         size_t lease_term) {
@@ -85,6 +86,14 @@ bool SharedThreadMerger::IsMergedUnSafe() const {
   return !IsAllLeaseTermsZeroUnSafe();
 }
 
+bool SharedThreadMerger::IsEnabledUnSafe() const {
+  return enabled_;
+}
+
+void SharedThreadMerger::SetEnabledUnSafe(bool enabled) {
+  enabled_ = enabled;
+}
+
 bool SharedThreadMerger::IsAllLeaseTermsZeroUnSafe() const {
   return std::all_of(lease_term_by_caller_.begin(), lease_term_by_caller_.end(),
                      [&](const auto& item) { return item.second == 0; });
diff --git a/fml/shared_thread_merger.h b/fml/shared_thread_merger.h
@@ -42,6 +42,14 @@ class SharedThreadMerger
   // See the doc of |RasterThreadMerger::IsMergedUnSafe|.
   bool IsMergedUnSafe() const;
 
+  // It's called by |RasterThreadMerger::IsEnabledUnSafe|.
+  // See the doc of |RasterThreadMerger::IsEnabledUnSafe|.
+  bool IsEnabledUnSafe() const;
+
+  // It's called by |RasterThreadMerger::Enable| or
+  // |RasterThreadMerger::Disable|.
+  void SetEnabledUnSafe(bool enabled);
+
   // It's called by |RasterThreadMerger::DecrementLease|.
   // See the doc of |RasterThreadMerger::DecrementLease|.
   bool DecrementLease(RasterThreadMergerId caller);
@@ -51,6 +59,7 @@ class SharedThreadMerger
   fml::TaskQueueId subsumed_;
   fml::RefPtr<fml::MessageLoopTaskQueues> task_queues_;
   std::mutex mutex_;
+  bool enabled_;
 
   /// The |MergeWithLease| or |ExtendLeaseTo| method will record the caller
   /// into this lease_term_by_caller_ map, |UnMergeNowIfLastOne|
diff --git a/shell/common/shell.cc b/shell/common/shell.cc
@@ -709,17 +709,17 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
   //
   // This prevents false positives such as this method starts assuming that the
   // raster and platform queues have a given thread configuration, but then the
-  // configuration is changed by a task, and the asumption is not longer true.
+  // configuration is changed by a task, and the assumption is no longer true.
   //
-  // This incorrect assumption can lead to dead lock.
+  // This incorrect assumption can lead to deadlock.
   // See `should_post_raster_task` for more.
   rasterizer_->DisableThreadMergerIfNeeded();
 
   // The normal flow executed by this method is that the platform thread is
   // starting the sequence and waiting on the latch. Later the UI thread posts
   // raster_task to the raster thread which signals the latch. If the raster and
   // the platform threads are the same this results in a deadlock as the
-  // raster_task will never be posted to the plaform/raster thread that is
+  // raster_task will never be posted to the platform/raster thread that is
   // blocked on a latch. To avoid the described deadlock, if the raster and the
   // platform threads are the same, should_post_raster_task will be false, and
   // then instead of posting a task to the raster thread, the ui thread just
@@ -818,18 +818,18 @@ void Shell::OnPlatformViewDestroyed() {
   //
   // This prevents false positives such as this method starts assuming that the
   // raster and platform queues have a given thread configuration, but then the
-  // configuration is changed by a task, and the asumption is not longer true.
+  // configuration is changed by a task, and the assumption is no longer true.
   //
-  // This incorrect assumption can lead to dead lock.
+  // This incorrect assumption can lead to deadlock.
   // See `should_post_raster_task` for more.
   rasterizer_->DisableThreadMergerIfNeeded();
 
   // The normal flow executed by this method is that the platform thread is
   // starting the sequence and waiting on the latch. Later the UI thread posts
   // raster_task to the raster thread triggers signaling the latch(on the IO
   // thread). If the raster and the platform threads are the same this results
-  // in a deadlock as the raster_task will never be posted to the plaform/raster
-  // thread that is blocked on a latch.  To avoid the described deadlock, if the
+  // in a deadlock as the raster_task will never be posted to platform/raster
+  // thread that is blocked on a latch. To avoid the described deadlock, if the
   // raster and the platform threads are the same, should_post_raster_task will
   // be false, and then instead of posting a task to the raster thread, the ui
   // thread just signals the latch and the platform/raster thread follows with
