Bug 1434856 - Move runnable prioritization checks outside of event queue locks. r=erahm, a=RyanVM
authorNathan Froyd <froydnj@mozilla.com>
Fri, 02 Feb 2018 13:55:05 -0500
changeset 454672 d4c3d0d6a6f9cd9375d0592fb85b8262371bfe8d
parent 454671 9c48b0136c8aa07f8dd8abf0af06dd331652776d
child 454673 103ade833ddf3c7424d651a024490c054ee1e48a
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm, RyanVM
bugs1434856
milestone59.0
Bug 1434856 - Move runnable prioritization checks outside of event queue locks. r=erahm, a=RyanVM 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
xpcom/tests/unit/xpcshell.ini
xpcom/threads/AbstractEventQueue.h
xpcom/threads/EventQueue.h
xpcom/threads/PrioritizedEventQueue.cpp
xpcom/threads/PrioritizedEventQueue.h
xpcom/threads/ThreadEventQueue.cpp
new file mode 100644
--- /dev/null
+++ b/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);
+}
--- a/xpcom/tests/unit/xpcshell.ini
+++ b/xpcom/tests/unit/xpcshell.ini
@@ -17,16 +17,17 @@ generated-files =
 # Bug 902073: test fails consistently on Android x86
 skip-if = os == "android"
 [test_bug374754.js]
 [test_bug476919.js]
 # Bug 676998: test fails consistently on 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]
 [test_hidden_files.js]
 [test_home.js]
 # Bug 676998: test fails consistently on Android
 fail-if = os == "android"
 [test_iniProcessor.js]
--- a/xpcom/threads/AbstractEventQueue.h
+++ b/xpcom/threads/AbstractEventQueue.h
@@ -30,22 +30,27 @@ enum class EventPriority
 // - PrioritizedEventQueue: Contains a queue for each priority level.
 //       Has heuristics to decide which queue to pop from. Events are
 //       pushed into the queue corresponding to their priority.
 //       Used for the main thread.
 //
 // 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;
 
   // Get an event from the front of the queue. aPriority is an out param. If the
   // implementation supports priorities, then this should be the same priority
   // that the event was pushed with. aPriority may be null. This should return
   // null if the queue is non-empty but the event in front is not ready to run.
--- a/xpcom/threads/EventQueue.h
+++ b/xpcom/threads/EventQueue.h
@@ -13,16 +13,18 @@
 
 class nsIRunnable;
 
 namespace mozilla {
 
 class EventQueue final : public AbstractEventQueue
 {
 public:
+  static const bool SupportsPrioritization = false;
+
   EventQueue() {}
   explicit EventQueue(EventPriority aPriority);
 
   void PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
                 EventPriority aPriority,
                 const MutexAutoLock& aProofOfLock) final override;
   already_AddRefed<nsIRunnable> GetEvent(EventPriority* aPriority,
                                          const MutexAutoLock& aProofOfLock) final override;
--- a/xpcom/threads/PrioritizedEventQueue.cpp
+++ b/xpcom/threads/PrioritizedEventQueue.cpp
@@ -33,25 +33,16 @@ template<class InnerQueueT>
 void
 PrioritizedEventQueue<InnerQueueT>::PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
                                              EventPriority aPriority,
                                              const MutexAutoLock& aProofOfLock)
 {
   // 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;
   }
 
   switch (priority) {
   case EventPriority::High:
     mHighQueue->PutEvent(event.forget(), priority, aProofOfLock);
--- a/xpcom/threads/PrioritizedEventQueue.h
+++ b/xpcom/threads/PrioritizedEventQueue.h
@@ -35,16 +35,18 @@ namespace mozilla {
 //   and normal-priority events available, we interleave popping from the
 //   normal and high queues.
 // - We do not select events from the idle queue if the current idle period
 //   is almost over.
 template<class InnerQueueT>
 class PrioritizedEventQueue final : public AbstractEventQueue
 {
 public:
+  static const bool SupportsPrioritization = true;
+
   PrioritizedEventQueue(UniquePtr<InnerQueueT> aHighQueue,
                         UniquePtr<InnerQueueT> aInputQueue,
                         UniquePtr<InnerQueueT> aNormalQueue,
                         UniquePtr<InnerQueueT> aIdleQueue,
                         already_AddRefed<nsIIdlePeriod> aIdlePeriod);
 
   void PutEvent(already_AddRefed<nsIRunnable>&& aEvent,
                 EventPriority aPriority,
--- a/xpcom/threads/ThreadEventQueue.cpp
+++ b/xpcom/threads/ThreadEventQueue.cpp
@@ -78,16 +78,32 @@ ThreadEventQueue<InnerQueueT>::PutEventI
                                                 NestedSink* aSink)
 {
   // We want to leak the reference when we fail to dispatch it, so that
   // we won't release the event in a wrong thread.
   LeakRefPtr<nsIRunnable> event(Move(aEvent));
   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) {
       return false;
     }
 
     if (aSink) {
       if (!aSink->mQueue) {