Bug 1434856 - move runnable prioritization checks outside of event queue locks; r=erahm

Otherwise, we might enter JS, decide to GC, and deadlock because we were
trying to dispatch tasks to the main thread's event queue while holding
the lock for the event queue.

xpcom/tests/unit/test_bug1434856.js

@@ -0,0 +1,26 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/  */
+
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
+
+function run_test() {
+  let complete = false;
+
+  let runnable = {
+    internalQI: XPCOMUtils.generateQI([Ci.nsIRunnable]),
+    QueryInterface(iid) {
+      // Attempt to schedule another runnable.  This simulates a GC/CC
+      // being scheduled while executing the JS QI.
+      Services.tm.dispatchToMainThread(() => false);
+      return this.internalQI(iid);
+    },
+
+    run() {
+      complete = true;
+    }
+  };
+
+  Services.tm.dispatchToMainThread(runnable);
+  Services.tm.spinEventLoopUntil(() => complete);
+}

xpcom/tests/unit/xpcshell.ini

@@ -22,6 +22,7 @@ skip-if = os == "android"
 fail-if = os == "android"
 [test_bug478086.js]
 [test_bug725015.js]
+[test_bug1434856.js]
 [test_debugger_malloc_size_of.js]
 [test_file_createUnique.js]
 [test_file_equality.js]

xpcom/threads/AbstractEventQueue.h

@@ -35,12 +35,17 @@ enum class EventPriority
 // Since AbstractEventQueue implementations are unsynchronized, they should be
 // wrapped in an outer SynchronizedEventQueue implementation (like
 // ThreadEventQueue).
+//
+// Subclasses should also define a `static const bool SupportsPrioritization`
+// member to indicate whether the subclass cares about runnable priorities
+// implemented through nsIRunnablePriority.
 class AbstractEventQueue
 {
 public:
   // Add an event to the end of the queue. Implementors are free to use
-  // aPriority however they wish. They may ignore it if the runnable has its own
-  // intrinsic priority (via nsIRunnablePriority).
+  // aPriority however they wish.  If the runnable supports nsIRunnablePriority
+  // and the implementing class supports prioritization, aPriority represents
+  // the result of calling nsIRunnablePriority::GetPriority().
   virtual void PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
                         EventPriority aPriority,
                         const MutexAutoLock& aProofOfLock) = 0;

xpcom/threads/EventQueue.h

@@ -18,6 +18,8 @@ namespace mozilla {
 class EventQueue final : public AbstractEventQueue
 {
 public:
+  static const bool SupportsPrioritization = false;
+
   EventQueue() {}
   explicit EventQueue(EventPriority aPriority);
 

xpcom/threads/PrioritizedEventQueue.cpp

@@ -38,15 +38,6 @@ PrioritizedEventQueue<InnerQueueT>::PutEvent(already_AddRefed<nsIRunnable>&& aEv
   // Double check the priority with a QI.
   RefPtr<nsIRunnable> event(aEvent);
   EventPriority priority = aPriority;
-  if (nsCOMPtr<nsIRunnablePriority> runnablePrio = do_QueryInterface(event)) {
-    uint32_t prio = nsIRunnablePriority::PRIORITY_NORMAL;
-    runnablePrio->GetPriority(&prio);
-    if (prio == nsIRunnablePriority::PRIORITY_HIGH) {
-      priority = EventPriority::High;
-    } else if (prio == nsIRunnablePriority::PRIORITY_INPUT) {
-      priority = EventPriority::Input;
-    }
-  }
 
   if (priority == EventPriority::Input && mInputQueueState == STATE_DISABLED) {
     priority = EventPriority::Normal;

xpcom/threads/PrioritizedEventQueue.h

@@ -40,6 +40,8 @@ template<class InnerQueueT>
 class PrioritizedEventQueue final : public AbstractEventQueue
 {
 public:
+  static const bool SupportsPrioritization = true;
+
   PrioritizedEventQueue(UniquePtr<InnerQueueT> aHighQueue,
                         UniquePtr<InnerQueueT> aInputQueue,
                         UniquePtr<InnerQueueT> aNormalQueue,

xpcom/threads/ThreadEventQueue.cpp

@@ -83,6 +83,22 @@ ThreadEventQueue<InnerQueueT>::PutEventInternal(already_AddRefed<nsIRunnable>&&
   nsCOMPtr<nsIThreadObserver> obs;
 
   {
+    // Check if the runnable wants to override the passed-in priority.
+    // Do this outside the lock, so runnables implemented in JS can QI
+    // (and possibly GC) outside of the lock.
+    if (InnerQueueT::SupportsPrioritization) {
+      auto* e = event.get();    // can't do_QueryInterface on LeakRefPtr.
+      if (nsCOMPtr<nsIRunnablePriority> runnablePrio = do_QueryInterface(e)) {
+        uint32_t prio = nsIRunnablePriority::PRIORITY_NORMAL;
+        runnablePrio->GetPriority(&prio);
+        if (prio == nsIRunnablePriority::PRIORITY_HIGH) {
+          aPriority = EventPriority::High;
+        } else if (prio == nsIRunnablePriority::PRIORITY_INPUT) {
+          aPriority = EventPriority::Input;
+        }
+      }
+    }
+
     MutexAutoLock lock(mLock);
 
     if (mEventsAreDoomed) {