Bug 1208687: Only discard events from the outermost queue. r=ehsan a=sylvestre
authorKyle Huey <khuey@kylehuey.com>
Mon, 28 Sep 2015 14:34:28 -0700
changeset 296216 333965c37e8dac23c3c1ae2de771e4a7d80cefee
parent 296215 fc9b7d64d4fb47fb561e63813088397052a992f0
child 296217 4f2a1db61cfdfe46666a6464faa38452b341505c
push id5245
push userraliiev@mozilla.com
push dateThu, 29 Oct 2015 11:30:51 +0000
treeherdermozilla-beta@dac831dc1bd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan, sylvestre
bugs1208687, 914762
milestone43.0a2
Bug 1208687: Only discard events from the outermost queue. r=ehsan a=sylvestre When workers shut down we discard the event queue rather than running it to completion. Originally workers managed their event queue themselves and would simply iterate through the array of events and cancel them all. After bug 914762 this was done by setting a (thread-)global "canceling" flag and then calling NS_ProcessPendingEvents. But this neglects that a shut down request can be received while the worker is in a sync queue. In this case, calling NS_ProcessPendingEvents will process any events pending in the sync queue, which is *not* the queue we need to cancel. The fix is, if we are in a sync queue when NotifyInternal is called, to defer clearing the queue until the top-most sync queue is destroyed and we are about to return to the regular event queue. Only then can we call NS_ProcessPendingEvents to clear out the queue. Because we can never process any events from this queue while sync queues are active, the timing of the mass cancellation is unchanged from the perspective of events in the regular queue.
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
testing/web-platform/meta/workers/interfaces/WorkerGlobalScope/close/incoming-message.html.ini
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -570,16 +570,23 @@ private:
 
     event->SetTrusted(true);
 
     globalScope->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
 
     return true;
   }
 
+  NS_IMETHOD Cancel() override
+  {
+    // We need to run regardless.
+    Run();
+    return WorkerRunnable::Cancel();
+  }
+
   virtual void
   PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
           override
   {
     // Report errors.
     WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult);
 
     // Match the busy count increase from NotifyRunnable.
@@ -1225,16 +1232,23 @@ private:
   {
     if (mTimer) {
       mTimer->Cancel();
       mTimer = nullptr;
     }
 
     return true;
   }
+
+  NS_IMETHOD Cancel() override
+  {
+    // We need to run regardless.
+    Run();
+    return WorkerRunnable::Cancel();
+  }
 };
 
 class UpdateRuntimeOptionsRunnable final : public WorkerControlRunnable
 {
   JS::RuntimeOptions mRuntimeOptions;
 
 public:
   UpdateRuntimeOptionsRunnable(
@@ -3807,16 +3821,17 @@ WorkerPrivate::WorkerPrivate(JSContext* 
   , mErrorHandlerRecursionCount(0)
   , mNextTimeoutId(1)
   , mStatus(Pending)
   , mFrozen(false)
   , mTimerRunning(false)
   , mRunningExpiredTimeouts(false)
   , mCloseHandlerStarted(false)
   , mCloseHandlerFinished(false)
+  , mPendingEventQueueClearing(false)
   , mMemoryReporterRunning(false)
   , mBlockedForMemoryReporter(false)
   , mCancelAllPendingRunnables(false)
   , mPeriodicGCTimerRunning(false)
   , mIdleGCTimerRunning(false)
   , mWorkerScriptExecutedSuccessfully(false)
 {
   MOZ_ASSERT_IF(!IsDedicatedWorker(), !aSharedWorkerName.IsVoid());
@@ -4635,16 +4650,17 @@ WorkerPrivate::IsOnCurrentThread(bool* a
 }
 
 void
 WorkerPrivate::ScheduleDeletion(WorkerRanOrNot aRanOrNot)
 {
   AssertIsOnWorkerThread();
   MOZ_ASSERT(mChildWorkers.IsEmpty());
   MOZ_ASSERT(mSyncLoopStack.IsEmpty());
+  MOZ_ASSERT(!mPendingEventQueueClearing);
 
   ClearMainEventQueue(aRanOrNot);
 #ifdef DEBUG
   if (WorkerRan == aRanOrNot) {
     nsIThread* currentThread = NS_GetCurrentThread();
     MOZ_ASSERT(currentThread);
     MOZ_ASSERT(!NS_HasPendingEvents(currentThread));
   }
@@ -4865,16 +4881,17 @@ WorkerPrivate::ProcessAllControlRunnable
   return result;
 }
 
 void
 WorkerPrivate::ClearMainEventQueue(WorkerRanOrNot aRanOrNot)
 {
   AssertIsOnWorkerThread();
 
+  MOZ_ASSERT(!mSyncLoopStack.Length());
   MOZ_ASSERT(!mCancelAllPendingRunnables);
   mCancelAllPendingRunnables = true;
 
   if (WorkerNeverRan == aRanOrNot) {
     for (uint32_t count = mPreStartRunnables.Length(), index = 0;
          index < count;
          index++) {
       nsRefPtr<WorkerRunnable> runnable = mPreStartRunnables[index].forget();
@@ -5227,16 +5244,21 @@ WorkerPrivate::DestroySyncLoop(uint32_t 
 #endif
 
     // This will delete |loopInfo|!
     mSyncLoopStack.RemoveElementAt(aLoopIndex);
   }
 
   MOZ_ALWAYS_TRUE(NS_SUCCEEDED(aThread->PopEventQueue(nestedEventTarget)));
 
+  if (!mSyncLoopStack.Length() && mPendingEventQueueClearing) {
+    ClearMainEventQueue(WorkerRan);
+    mPendingEventQueueClearing = false;
+  }
+
   return result;
 }
 
 void
 WorkerPrivate::StopSyncLoop(nsIEventTarget* aSyncLoopTarget, bool aResult)
 {
   AssertIsOnWorkerThread();
   AssertValidSyncLoop(aSyncLoopTarget);
@@ -5493,17 +5515,23 @@ WorkerPrivate::NotifyInternal(JSContext*
   MOZ_ASSERT(previousStatus >= Canceling || mKillTime.IsNull());
 
   // Let all our features know the new status.
   NotifyFeatures(aCx, aStatus);
 
   // If this is the first time our status has changed then we need to clear the
   // main event queue.
   if (previousStatus == Running) {
-    ClearMainEventQueue(WorkerRan);
+    // NB: If we're in a sync loop, we can't clear the queue immediately,
+    // because this is the wrong queue. So we have to defer it until later.
+    if (mSyncLoopStack.Length()) {
+      mPendingEventQueueClearing = true;
+    } else {
+      ClearMainEventQueue(WorkerRan);
+    }
   }
 
   // If we've run the close handler, we don't need to do anything else.
   if (mCloseHandlerFinished) {
     return true;
   }
 
   // If the worker script never ran, or failed to compile, we don't need to do
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -939,16 +939,17 @@ class WorkerPrivate : public WorkerPriva
   uint32_t mErrorHandlerRecursionCount;
   uint32_t mNextTimeoutId;
   Status mStatus;
   bool mFrozen;
   bool mTimerRunning;
   bool mRunningExpiredTimeouts;
   bool mCloseHandlerStarted;
   bool mCloseHandlerFinished;
+  bool mPendingEventQueueClearing;
   bool mMemoryReporterRunning;
   bool mBlockedForMemoryReporter;
   bool mCancelAllPendingRunnables;
   bool mPeriodicGCTimerRunning;
   bool mIdleGCTimerRunning;
   bool mWorkerScriptExecutedSuccessfully;
   bool mPreferences[WORKERPREF_COUNT];
   bool mOnLine;
deleted file mode 100644
--- a/testing/web-platform/meta/workers/interfaces/WorkerGlobalScope/close/incoming-message.html.ini
+++ /dev/null
@@ -1,7 +0,0 @@
-[incoming-message.html]
-  type: testharness
-  disabled:
-    if debug: unstable
-  [close() and incoming message]
-    expected: FAIL
-