Bug 1875661 - Refactor shutdown for remote canvas. r=gfx-reviewers,lsalzman

This patch makes it so that we always shutdown gracefully by making
us flush the event queue for CanvasRenderThread, after blocking on the
task queues draining. This allows our dependencies like RemoteTextureMap
to shutdown successfully.

It also fixes a bug where CanvasTranslator::CanSend would still return
true on Windows, while blocked on in CanvasTranslator::ActorDestroy
waiting for the task queue to shutdown. The CanSend status is only
updated after ActorDestroy is called from IProtocol::DestroySubtree.

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

gfx/ipc/CanvasRenderThread.cpp

@@ -34,7 +34,8 @@ static bool sCanvasRenderThreadEverStarted = false;
 CanvasRenderThread::CanvasRenderThread(nsCOMPtr<nsIThread>&& aThread,
                                        nsCOMPtr<nsIThreadPool>&& aWorkers,
                                        bool aCreatedThread)
-    : mThread(std::move(aThread)),
+    : mMutex("CanvasRenderThread::mMutex"),
+      mThread(std::move(aThread)),
       mWorkers(std::move(aWorkers)),
       mCreatedThread(aCreatedThread) {}
 
@@ -148,16 +149,39 @@ void CanvasRenderThread::Shutdown() {
     return;
   }
 
+  // This closes all of the IPDL actors with possibly active task queues.
   CanvasManagerParent::Shutdown();
 
+  // Any task queues that are in the process of shutting down are tracked in
+  // mPendingShutdownTaskQueues. We need to block on each one until all events
+  // are flushed so that we can safely teardown RemoteTextureMap afterwards.
+  while (true) {
+    RefPtr<TaskQueue> taskQueue;
+    {
+      MutexAutoLock lock(sCanvasRenderThread->mMutex);
+
+      auto& pendingQueues = sCanvasRenderThread->mPendingShutdownTaskQueues;
+      if (pendingQueues.IsEmpty()) {
+        break;
+      }
+
+      taskQueue = pendingQueues.PopLastElement();
+    }
+    taskQueue->AwaitShutdownAndIdle();
+  }
+
+  bool createdThread = sCanvasRenderThread->mCreatedThread;
+  nsCOMPtr<nsIThread> oldThread = sCanvasRenderThread->GetCanvasRenderThread();
+  nsCOMPtr<nsIThreadPool> oldWorkers = sCanvasRenderThread->mWorkers;
+
+  // Ensure that we flush the CanvasRenderThread event queue before clearing our
+  // singleton.
+  NS_DispatchAndSpinEventLoopUntilComplete(
+      "CanvasRenderThread::Shutdown"_ns, oldThread,
+      NS_NewRunnableFunction("CanvasRenderThread::Shutdown", []() -> void {}));
+
   // Null out sCanvasRenderThread before we enter synchronous Shutdown,
   // from here on we are to be considered shut down for our consumers.
-  nsCOMPtr<nsIThreadPool> oldWorkers = sCanvasRenderThread->mWorkers;
-  nsCOMPtr<nsIThread> oldThread;
-  if (sCanvasRenderThread->mCreatedThread) {
-    oldThread = sCanvasRenderThread->GetCanvasRenderThread();
-    MOZ_ASSERT(oldThread);
-  }
   sCanvasRenderThread = nullptr;
 
   if (oldWorkers) {
@@ -166,7 +190,7 @@ void CanvasRenderThread::Shutdown() {
 
   // We do a synchronous shutdown here while spinning the MT event loop, but
   // only if we created a dedicated CanvasRender thread.
-  if (oldThread) {
+  if (createdThread) {
     oldThread->Shutdown();
   }
 }
@@ -216,6 +240,32 @@ CanvasRenderThread::CreateWorkerTaskQueue() {
       .forget();
 }
 
+/* static */ void CanvasRenderThread::ShutdownWorkerTaskQueue(
+    TaskQueue* aTaskQueue) {
+  MOZ_ASSERT(aTaskQueue);
+
+  aTaskQueue->BeginShutdown();
+
+  if (!sCanvasRenderThread) {
+    MOZ_ASSERT_UNREACHABLE("No CanvasRenderThread!");
+    return;
+  }
+
+  MutexAutoLock lock(sCanvasRenderThread->mMutex);
+  auto& pendingQueues = sCanvasRenderThread->mPendingShutdownTaskQueues;
+  pendingQueues.AppendElement(aTaskQueue);
+}
+
+/* static */ void CanvasRenderThread::FinishShutdownWorkerTaskQueue(
+    TaskQueue* aTaskQueue) {
+  if (!sCanvasRenderThread) {
+    return;
+  }
+
+  MutexAutoLock lock(sCanvasRenderThread->mMutex);
+  sCanvasRenderThread->mPendingShutdownTaskQueues.RemoveElement(aTaskQueue);
+}
+
 /* static */ void CanvasRenderThread::Dispatch(
     already_AddRefed<nsIRunnable> aRunnable) {
   if (!sCanvasRenderThread) {

gfx/ipc/CanvasRenderThread.h

@@ -8,8 +8,10 @@
 #define _include_gfx_ipc_CanvasRenderThread_h__
 
 #include "mozilla/AlreadyAddRefed.h"
+#include "mozilla/Mutex.h"
 #include "nsCOMPtr.h"
 #include "nsISupportsImpl.h"
+#include "nsTArray.h"
 
 class nsIRunnable;
 class nsIThread;
@@ -53,6 +55,10 @@ class CanvasRenderThread final {
 
   static already_AddRefed<TaskQueue> CreateWorkerTaskQueue();
 
+  static void ShutdownWorkerTaskQueue(TaskQueue* aTaskQueue);
+
+  static void FinishShutdownWorkerTaskQueue(TaskQueue* aTaskQueue);
+
   static void Dispatch(already_AddRefed<nsIRunnable> aRunnable);
 
  private:
@@ -60,13 +66,17 @@ class CanvasRenderThread final {
                      nsCOMPtr<nsIThreadPool>&& aWorkers, bool aCreatedThread);
   ~CanvasRenderThread();
 
+  Mutex mMutex;
+
   nsCOMPtr<nsIThread> const mThread;
 
   nsCOMPtr<nsIThreadPool> const mWorkers;
 
+  nsTArray<RefPtr<TaskQueue>> mPendingShutdownTaskQueues MOZ_GUARDED_BY(mMutex);
+
   // True if mThread points to CanvasRender thread, false if mThread points to
   // Compositor/Render thread.
-  bool mCreatedThread;
+  const bool mCreatedThread;
 };
 
 }  // namespace gfx

gfx/layers/CanvasDrawEventRecorder.cpp

@@ -98,8 +98,7 @@ bool CanvasDrawEventRecorder::Init(TextureType aTextureType,
   if (!mHelpers->InitTranslator(aTextureType, aBackendType,
                                 std::move(header->handle),
                                 std::move(bufferHandles), mDefaultBufferSize,
-                                std::move(readerSem), std::move(writerSem),
+                                std::move(readerSem), std::move(writerSem))) {
-                                /* aUseIPDLThread */ false)) {
     return false;
   }
 

gfx/layers/CanvasDrawEventRecorder.h

@@ -67,11 +67,13 @@ class CanvasDrawEventRecorder final : public gfx::DrawEventRecorderPrivate,
    public:
     virtual ~Helpers() = default;
 
-    virtual bool InitTranslator(
+    virtual bool InitTranslator(TextureType aTextureType,
-        TextureType aTextureType, gfx::BackendType aBackendType,
+                                gfx::BackendType aBackendType,
-        Handle&& aReadHandle, nsTArray<Handle>&& aBufferHandles,
+                                Handle&& aReadHandle,
-        uint64_t aBufferSize, CrossProcessSemaphoreHandle&& aReaderSem,
+                                nsTArray<Handle>&& aBufferHandles,
-        CrossProcessSemaphoreHandle&& aWriterSem, bool aUseIPDLThread) = 0;
+                                uint64_t aBufferSize,
+                                CrossProcessSemaphoreHandle&& aReaderSem,
+                                CrossProcessSemaphoreHandle&& aWriterSem) = 0;
 
     virtual bool AddBuffer(Handle&& aBufferHandle, uint64_t aBufferSize) = 0;
 

gfx/layers/ipc/CanvasChild.cpp

@@ -37,8 +37,7 @@ class RecorderHelpers final : public CanvasDrawEventRecorder::Helpers {
                       Handle&& aReadHandle, nsTArray<Handle>&& aBufferHandles,
                       uint64_t aBufferSize,
                       CrossProcessSemaphoreHandle&& aReaderSem,
-                      CrossProcessSemaphoreHandle&& aWriterSem,
+                      CrossProcessSemaphoreHandle&& aWriterSem) override {
-                      bool aUseIPDLThread) override {
     NS_ASSERT_OWNINGTHREAD(RecorderHelpers);
     if (NS_WARN_IF(!mCanvasChild)) {
       return false;
@@ -46,7 +45,7 @@ class RecorderHelpers final : public CanvasDrawEventRecorder::Helpers {
     return mCanvasChild->SendInitTranslator(
         aTextureType, aBackendType, std::move(aReadHandle),
         std::move(aBufferHandles), aBufferSize, std::move(aReaderSem),
-        std::move(aWriterSem), aUseIPDLThread);
+        std::move(aWriterSem));
   }
 
   bool AddBuffer(Handle&& aBufferHandle, uint64_t aBufferSize) override {

gfx/layers/ipc/CanvasTranslator.cpp

@@ -66,7 +66,8 @@ UniquePtr<TextureData> CanvasTranslator::CreateTextureData(
 CanvasTranslator::CanvasTranslator(
     layers::SharedSurfacesHolder* aSharedSurfacesHolder,
     const dom::ContentParentId& aContentId, uint32_t aManagerId)
-    : mSharedSurfacesHolder(aSharedSurfacesHolder),
+    : mTranslationTaskQueue(gfx::CanvasRenderThread::CreateWorkerTaskQueue()),
+      mSharedSurfacesHolder(aSharedSurfacesHolder),
       mMaxSpinCount(StaticPrefs::gfx_canvas_remote_max_spin_count()),
       mContentId(aContentId),
       mManagerId(aManagerId) {
@@ -133,7 +134,7 @@ mozilla::ipc::IPCResult CanvasTranslator::RecvInitTranslator(
     TextureType aTextureType, gfx::BackendType aBackendType,
     Handle&& aReadHandle, nsTArray<Handle>&& aBufferHandles,
     uint64_t aBufferSize, CrossProcessSemaphoreHandle&& aReaderSem,
-    CrossProcessSemaphoreHandle&& aWriterSem, bool aUseIPDLThread) {
+    CrossProcessSemaphoreHandle&& aWriterSem) {
   if (mHeaderShmem) {
     return IPC_FAIL(this, "RecvInitTranslator called twice.");
   }
@@ -167,10 +168,6 @@ mozilla::ipc::IPCResult CanvasTranslator::RecvInitTranslator(
         << "GFX: CanvasTranslator failed creating WebGL shared context";
   }
 
-  if (!aUseIPDLThread) {
-    mTranslationTaskQueue = gfx::CanvasRenderThread::CreateWorkerTaskQueue();
-  }
-
   // Use the first buffer as our current buffer.
   mDefaultBufferSize = aBufferSize;
   auto handleIter = aBufferHandles.begin();
@@ -365,20 +362,20 @@ void CanvasTranslator::NextBuffer() {
 void CanvasTranslator::ActorDestroy(ActorDestroyReason why) {
   MOZ_ASSERT(gfx::CanvasRenderThread::IsInCanvasRenderThread());
 
-  if (!mTranslationTaskQueue) {
+  // Since we might need to access the actor status off the owning IPDL thread,
-    gfx::CanvasRenderThread::Dispatch(
+  // we need to cache it here.
-        NewRunnableMethod("CanvasTranslator::FinishShutdown", this,
+  mIPDLClosed = true;
-                          &CanvasTranslator::FinishShutdown));
+
+  DispatchToTaskQueue(NewRunnableMethod("CanvasTranslator::ClearTextureInfo",
+                                        this,
+                                        &CanvasTranslator::ClearTextureInfo));
+
+  if (mTranslationTaskQueue) {
+    gfx::CanvasRenderThread::ShutdownWorkerTaskQueue(mTranslationTaskQueue);
     return;
   }
-
-  mTranslationTaskQueue->BeginShutdown();
-  mTranslationTaskQueue->AwaitShutdownAndIdle();
-  FinishShutdown();
 }
 
-void CanvasTranslator::FinishShutdown() { ClearTextureInfo(); }
-
 bool CanvasTranslator::CheckDeactivated() {
   if (mDeactivated) {
     return true;
@@ -471,7 +468,7 @@ void CanvasTranslator::CheckAndSignalWriter() {
         // The writer is making a decision about whether to wait. So, we must
         // wait until it has decided to avoid races. Check if the writer is
         // closed to avoid hangs.
-        if (!CanSend()) {
+        if (mIPDLClosed) {
           return;
         }
         continue;
@@ -566,7 +563,7 @@ void CanvasTranslator::TranslateRecording() {
         [&](RecordedEvent* recordedEvent) -> bool {
           // Make sure that the whole event was read from the stream.
           if (!mCurrentMemReader.good()) {
-            if (!CanSend()) {
+            if (mIPDLClosed) {
               // The other side has closed only warn about read failure.
               gfxWarning() << "Failed to read event type: "
                            << recordedEvent->GetType();
@@ -605,7 +602,7 @@ void CanvasTranslator::TranslateRecording() {
   case _typeenum: {                                                    \
     auto e = _class(mCurrentMemReader);                                \
     if (!mCurrentMemReader.good()) {                                   \
-      if (!CanSend()) {                                                \
+      if (mIPDLClosed) {                                               \
         /* The other side has closed only warn about read failure. */  \
         gfxWarning() << "Failed to read event type: " << _typeenum;    \
       } else {                                                         \
@@ -1120,6 +1117,10 @@ void CanvasTranslator::ClearTextureInfo() {
     mRemoteTextureOwner->UnregisterAllTextureOwners();
     mRemoteTextureOwner = nullptr;
   }
+  if (mTranslationTaskQueue) {
+    gfx::CanvasRenderThread::FinishShutdownWorkerTaskQueue(
+        mTranslationTaskQueue);
+  }
 }
 
 already_AddRefed<gfx::SourceSurface> CanvasTranslator::LookupExternalSurface(

gfx/layers/ipc/CanvasTranslator.h

@@ -75,14 +75,14 @@ class CanvasTranslator final : public gfx::InlineTranslator,
    * @param aBufferSize size of buffers and the default size
    * @param aReaderSem reading blocked semaphore for the CanvasEventRingBuffer
    * @param aWriterSem writing blocked semaphore for the CanvasEventRingBuffer
-   * @param aUseIPDLThread if true, use the IPDL thread instead of the worker
-   *        pool for translation requests
    */
-  ipc::IPCResult RecvInitTranslator(
+  ipc::IPCResult RecvInitTranslator(TextureType aTextureType,
-      TextureType aTextureType, gfx::BackendType aBackendType,
+                                    gfx::BackendType aBackendType,
-      Handle&& aReadHandle, nsTArray<Handle>&& aBufferHandles,
+                                    Handle&& aReadHandle,
-      uint64_t aBufferSize, CrossProcessSemaphoreHandle&& aReaderSem,
+                                    nsTArray<Handle>&& aBufferHandles,
-      CrossProcessSemaphoreHandle&& aWriterSem, bool aUseIPDLThread);
+                                    uint64_t aBufferSize,
+                                    CrossProcessSemaphoreHandle&& aReaderSem,
+                                    CrossProcessSemaphoreHandle&& aWriterSem);
 
   /**
    * Restart the translation from a Stopped state.
@@ -285,8 +285,6 @@ class CanvasTranslator final : public gfx::InlineTranslator,
 
   bool ReadPendingEvent(EventType& aEventType);
 
-  void FinishShutdown();
-
   bool CheckDeactivated();
 
   void Deactivate();
@@ -326,8 +324,8 @@ class CanvasTranslator final : public gfx::InlineTranslator,
 
   void ClearCachedResources();
 
-  RefPtr<TaskQueue> mTranslationTaskQueue;
+  const RefPtr<TaskQueue> mTranslationTaskQueue;
-  RefPtr<SharedSurfacesHolder> mSharedSurfacesHolder;
+  const RefPtr<SharedSurfacesHolder> mSharedSurfacesHolder;
 #if defined(XP_WIN)
   RefPtr<ID3D11Device> mDevice;
 #endif
@@ -384,6 +382,7 @@ class CanvasTranslator final : public gfx::InlineTranslator,
   UniquePtr<gfx::DataSourceSurface::ScopedMap> mPreparedMap;
   Atomic<bool> mDeactivated{false};
   Atomic<bool> mBlocked{false};
+  Atomic<bool> mIPDLClosed{false};
   bool mIsInTransaction = false;
   bool mDeviceResetInProgress = false;
 };

gfx/layers/ipc/PCanvas.ipdl

@@ -31,15 +31,12 @@ parent:
    * handles for the initial buffers for translation. aBufferSize is the size of
    * each aBufferHandles' memory and the default size. aReaderSem and aWriterSem
    * are handles for the semaphores to handle waiting on either side.
-   * aUseIPDLThread if true, use the IPDL thread instead of the worker pool for
-   * translation requests
    */
   async InitTranslator(TextureType aTextureType, BackendType aBackendType,
                        Handle aHeaderHandle, Handle[] aBufferHandles,
                        uint64_t aBufferSize,
                        CrossProcessSemaphoreHandle aReaderSem,
-                       CrossProcessSemaphoreHandle aWriterSem,
+                       CrossProcessSemaphoreHandle aWriterSem);
-                       bool aUseIPDLThread);
 
   /**
    * Restart the translation from a Stopped state.