Bug 1757766 - Avoid using NS_DISPATCH_SYNC in UiCompositorController::Destroy. r=gfx-reviewers,aosmond

On Android, we must dispatch UiCompositorController::Destroy to run on
the UI thread synchronously. We were using NS_DISPATCH_SYNC to do so,
but that works by starting a nested event loop that continues to
execute tasks on the thread we have dispatched from. This means that
we can start to execute a task which calls
nsBaseWidget::CreateCompositor whilst we are midway through
nsBaseWidget::DestroyCompositor. As well as generally seeming like a
terrible idea, this also causes an assertion failure in some tests.

To avoid this use SynchronousTask rather than NS_DISPATCH_SYNC, as it
actually blocks synchronously. Additionally, do the same thing for
APZInputBridgeChild::Destroy, as it is called from the same location
and poses the same risk.

Ideally we wouldn't have to call UiCompositorControllerChild::Destroy
synchronously at all, but it was added in bug 1392705 to fix severe
crashes. It might be a good idea to re-evaluate whether it is still
required at some point in the future.

Differential Revision: https://phabricator.services.mozilla.com/D140084

gfx/layers/ipc/APZInputBridgeChild.cpp

@@ -10,6 +10,7 @@
 #include "mozilla/gfx/GPUProcessManager.h"
 #include "mozilla/ipc/Endpoint.h"
 #include "mozilla/layers/APZThreadUtils.h"
+#include "mozilla/layers/SynchronousTask.h"
 
 namespace mozilla {
 namespace layers {
@@ -54,27 +55,27 @@ void APZInputBridgeChild::Open(Endpoint<PAPZInputBridgeChild>&& aEndpoint) {
 
 void APZInputBridgeChild::Destroy() {
   MOZ_ASSERT(XRE_IsParentProcess());
+  MOZ_ASSERT(NS_IsMainThread());
 
   // Destroy will get called from the main thread, so we must synchronously
   // dispatch to the controller thread to close the bridge.
-  if (!APZThreadUtils::IsControllerThread()) {
-    APZThreadUtils::RunOnControllerThread(
-        NewRunnableMethod("layers::APZInputBridgeChild::Destroy", this,
-                          &APZInputBridgeChild::Destroy),
-        nsIThread::DISPATCH_SYNC);
-    return;
-  }
+  layers::SynchronousTask task("layers::APZInputBridgeChild::Destroy");
+  APZThreadUtils::RunOnControllerThread(
+      NS_NewRunnableFunction("layers::APZInputBridgeChild::Destroy", [&]() {
+        APZThreadUtils::AssertOnControllerThread();
+        AutoCompleteTask complete(&task);
 
-  APZThreadUtils::AssertOnControllerThread();
+        // Clear the process token so that we don't notify the GPUProcessManager
+        // about an abnormal shutdown, thereby tearing down the GPU process.
+        mProcessToken = 0;
 
-  // Clear the process token so that we don't notify the GPUProcessManager
-  // about an abnormal shutdown, thereby tearing down the GPU process.
-  mProcessToken = 0;
+        if (mIsOpen) {
+          PAPZInputBridgeChild::Close();
+          mIsOpen = false;
+        }
+      }));
 
-  if (mIsOpen) {
-    PAPZInputBridgeChild::Close();
-    mIsOpen = false;
-  }
+  task.Wait();
 }
 
 void APZInputBridgeChild::ActorDestroy(ActorDestroyReason aWhy) {

gfx/layers/ipc/UiCompositorControllerChild.cpp

@@ -8,6 +8,7 @@
 
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/layers/CompositorThread.h"
+#include "mozilla/layers/SynchronousTask.h"
 #include "mozilla/layers/UiCompositorControllerMessageTypes.h"
 #include "mozilla/layers/UiCompositorControllerParent.h"
 #include "mozilla/gfx/GPUProcessManager.h"
@@ -28,8 +29,6 @@ static RefPtr<nsThread> GetUiThread() {
 }
 #endif  // defined(MOZ_WIDGET_ANDROID)
 
-static bool IsOnUiThread() { return GetUiThread()->IsOnCurrentThread(); }
-
 namespace mozilla {
 namespace layers {
 
@@ -156,31 +155,34 @@ bool UiCompositorControllerChild::EnableLayerUpdateNotifications(
 }
 
 void UiCompositorControllerChild::Destroy() {
-  if (!IsOnUiThread()) {
-    GetUiThread()->Dispatch(
-        NewRunnableMethod("layers::UiCompositorControllerChild::Destroy", this,
-                          &UiCompositorControllerChild::Destroy),
-        nsIThread::DISPATCH_SYNC);
-    return;
-  }
+  MOZ_ASSERT(NS_IsMainThread());
 
-  // Clear the process token so that we don't notify the GPUProcessManager
-  // about an abnormal shutdown, thereby tearing down the GPU process.
-  mProcessToken = 0;
+  layers::SynchronousTask task("UiCompositorControllerChild::Destroy");
+  GetUiThread()->Dispatch(NS_NewRunnableFunction(
+      "layers::UiCompositorControllerChild::Destroy", [&]() {
+        MOZ_ASSERT(GetUiThread()->IsOnCurrentThread());
+        AutoCompleteTask complete(&task);
 
-  if (mWidget) {
-    // Dispatch mWidget to main thread to prevent it from being destructed by
-    // the ui thread.
-    RefPtr<nsIWidget> widget = std::move(mWidget);
-    NS_ReleaseOnMainThread("UiCompositorControllerChild::mWidget",
-                           widget.forget());
-  }
+        // Clear the process token so that we don't notify the GPUProcessManager
+        // about an abnormal shutdown, thereby tearing down the GPU process.
+        mProcessToken = 0;
 
-  if (mIsOpen) {
-    // Close the underlying IPC channel.
-    PUiCompositorControllerChild::Close();
-    mIsOpen = false;
-  }
+        if (mWidget) {
+          // Dispatch mWidget to main thread to prevent it from being destructed
+          // by the ui thread.
+          RefPtr<nsIWidget> widget = std::move(mWidget);
+          NS_ReleaseOnMainThread("UiCompositorControllerChild::mWidget",
+                                 widget.forget());
+        }
+
+        if (mIsOpen) {
+          // Close the underlying IPC channel.
+          PUiCompositorControllerChild::Close();
+          mIsOpen = false;
+        }
+      }));
+
+  task.Wait();
 }
 
 bool UiCompositorControllerChild::DeallocPixelBuffer(Shmem& aMem) {
@@ -259,7 +261,7 @@ UiCompositorControllerChild::UiCompositorControllerChild(
 UiCompositorControllerChild::~UiCompositorControllerChild() = default;
 
 void UiCompositorControllerChild::OpenForSameProcess() {
-  MOZ_ASSERT(IsOnUiThread());
+  MOZ_ASSERT(GetUiThread()->IsOnCurrentThread());
 
   mIsOpen = Open(mParent, mozilla::layers::CompositorThread(),
                  mozilla::ipc::ChildSide);
@@ -278,7 +280,7 @@ void UiCompositorControllerChild::OpenForSameProcess() {
 
 void UiCompositorControllerChild::OpenForGPUProcess(
     Endpoint<PUiCompositorControllerChild>&& aEndpoint) {
-  MOZ_ASSERT(IsOnUiThread());
+  MOZ_ASSERT(GetUiThread()->IsOnCurrentThread());
 
   mIsOpen = aEndpoint.Bind(this);