Bug 1384336 - Fix to MainThreadInvoker to avoid deadlocks (r=aklotz)

When removing our Windows message loop pumping code in the content
process, a11y code on the MTA thread must have some way to wake up the
main thread. The main thread could be blocked either on a conditional
variable waiting for a Gecko event, or it could be blocked waiting on
a Windows HANDLE in IPC code (doing a sync message send). In the
former case, we wake it up by posting an event to the main thread. In
the latter case, we continue to use the asynchronous procedure call
mechanism.

MozReview-Commit-ID: FN6KWaGo9Zl

ipc/mscom/MainThreadInvoker.cpp

@@ -26,22 +26,27 @@ namespace {
  * our runnable. Since spinning is pointless in the uniprocessor case, we block
  * on an event that is set by the main thread once it has finished the runnable.
  */
-class MOZ_RAII SyncRunnable
+class SyncRunnable : public mozilla::Runnable
 {
 public:
-  explicit SyncRunnable(already_AddRefed<nsIRunnable>&& aRunnable)
-    : mRunnable(aRunnable)
-  {
-    MOZ_ASSERT(mRunnable);
-  }
+  explicit SyncRunnable(already_AddRefed<nsIRunnable> aRunnable)
+    : mozilla::Runnable("MainThreadInvoker")
+    , mRunnable(aRunnable)
+  {}
 
   ~SyncRunnable() = default;
 
-  void Run()
+  NS_IMETHOD Run()
   {
+    if (mHasRun) {
+      return NS_OK;
+    }
+    mHasRun = true;
+
     mRunnable->Run();
 
     mEvent.Signal();
+    return NS_OK;
   }
 
   bool WaitUntilComplete()
@@ -50,6 +55,7 @@ class MOZ_RAII SyncRunnable
   }
 
 private:
+  bool                      mHasRun = false;
   nsCOMPtr<nsIRunnable>     mRunnable;
   mozilla::mscom::SpinEvent mEvent;
 };
@@ -101,20 +107,21 @@ MainThreadInvoker::Invoke(already_AddRefed<nsIRunnable>&& aRunnable)
     return true;
   }
 
-  SyncRunnable syncRunnable(runnable.forget());
+  RefPtr<SyncRunnable> syncRunnable = new SyncRunnable(runnable.forget());
 
-  if (!::QueueUserAPC(&MainThreadAPC, sMainThread,
-                      reinterpret_cast<UINT_PTR>(&syncRunnable))) {
+  if (NS_FAILED(NS_DispatchToMainThread(syncRunnable, nsIEventTarget::DISPATCH_NORMAL))) {
     return false;
   }
 
-  // We should ensure a call to NtTestAlert() is made on the main thread so
-  // that the main thread will check for APCs during event processing. If we
-  // omit this then the main thread will not check its APC queue until it is
-  // idle.
-  widget::WinUtils::SetAPCPending();
+  // This ref gets released in MainThreadAPC when it runs.
+  SyncRunnable* syncRunnableRef = syncRunnable.get();
+  NS_ADDREF(syncRunnableRef);
+  if (!::QueueUserAPC(&MainThreadAPC, sMainThread,
+                      reinterpret_cast<UINT_PTR>(syncRunnableRef))) {
+    return false;
+  }
 
-  return syncRunnable.WaitUntilComplete();
+  return syncRunnable->WaitUntilComplete();
 }
 
 /* static */ VOID CALLBACK
@@ -125,6 +132,7 @@ MainThreadInvoker::MainThreadAPC(ULONG_PTR aParam)
   MOZ_ASSERT(NS_IsMainThread());
   auto runnable = reinterpret_cast<SyncRunnable*>(aParam);
   runnable->Run();
+  NS_RELEASE(runnable);
 }
 
 } // namespace mscom

widget/windows/WinUtils.cpp

@@ -685,15 +685,6 @@ WinUtils::MonitorFromRect(const gfx::Rect& rect)
 #define STATUS_SUCCESS ((NTSTATUS)0x00000000L)
 #endif
 
-static Atomic<bool> sAPCPending;
-
-/* static */
-void
-WinUtils::SetAPCPending()
-{
-  sAPCPending = true;
-}
-
 /* static */
 a11y::Accessible*
 WinUtils::GetRootAccessibleForHWND(HWND aHwnd)
@@ -712,11 +703,6 @@ bool
 WinUtils::PeekMessage(LPMSG aMsg, HWND aWnd, UINT aFirstMessage,
                       UINT aLastMessage, UINT aOption)
 {
-#ifdef ACCESSIBILITY
-  if (NS_IsMainThread() && sAPCPending.exchange(false)) {
-    while (sNtTestAlert() != STATUS_SUCCESS) ;
-  }
-#endif
 #ifdef NS_ENABLE_TSF
   RefPtr<ITfMessagePump> msgPump = TSFTextStore::GetMessagePump();
   if (msgPump) {
@@ -788,10 +774,6 @@ WinUtils::WaitForMessage(DWORD aTimeoutMs)
 #if defined(ACCESSIBILITY)
     if (result == WAIT_IO_COMPLETION) {
       if (NS_IsMainThread()) {
-        if (sAPCPending.exchange(false)) {
-          // Clear out any pending APCs
-          while (sNtTestAlert() != STATUS_SUCCESS) ;
-        }
         // We executed an APC that would have woken up the hang monitor. Since
         // there are no more APCs pending and we are now going to sleep again,
         // we should notify the hang monitor.

widget/windows/WinUtils.h

@@ -504,8 +504,6 @@ class WinUtils
   static bool GetAppInitDLLs(nsAString& aOutput);
 
 #ifdef ACCESSIBILITY
-  static void SetAPCPending();
-
   static a11y::Accessible* GetRootAccessibleForHWND(HWND aHwnd);
 #endif