Bug 1426467: Part 4: Segregate WorkerDebuggeeRunnables into their own queues. r=baku
authorJim Blandy <jimb@mozilla.com>
Tue, 23 Oct 2018 06:30:30 +0000
changeset 491168 0b9e4abc5911fb241ba351c07e7cc2f3e4fb2ed7
parent 491167 f3d51256c5d728e4b324525917b481565f5690d9
child 491169 d1052b2d28f6c4239e8870465ccc09ef95f308ac
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersbaku
bugs1426467
milestone65.0a1
Bug 1426467: Part 4: Segregate WorkerDebuggeeRunnables into their own queues. r=baku Remove WorkerPrivate::mQueuedRunnables and its associated functions. The approach they implement can never be correct, as the parent window gets 'resumed' whenever the debugger resumes execution after a breakpoint. The interrupted JavaScript invocation has not yet completed, so it is not yet time to run mQueuedRunnables. Simply re-enqueing them at that point can cause messages from the worker to arrive out of order. Instead, we create a separate ThrottledEventQueue, WorkerPrivate::mMainThreadDebuggeeEventTarget especially for WorkerDebuggeeRunnables, runnables sent from the worker to the main thread that should not be delivered to a paused or frozen content window. This queue is paused and resumed by WorkerPrivate::Freeze, WorkerPrivate::Thaw, WorkerPrivate::ParentWindowPaused, and WorkerPrivate::ParentWindowResumed. Since this affects when WorkerDebuggeeRunnables are delivered relative to other administrative worker runnables, WorkerDebuggeeRunnable must use a ThreadSafeWorkerRef to ensure that the WorkerPrivate sticks around long enough for them to run properly. Depends on D9219 Differential Revision: https://phabricator.services.mozilla.com/D9220
dom/workers/MessageEventRunnable.cpp
dom/workers/WorkerError.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
dom/workers/WorkerRunnable.cpp
dom/workers/WorkerRunnable.h
--- a/dom/workers/MessageEventRunnable.cpp
+++ b/dom/workers/MessageEventRunnable.cpp
@@ -109,22 +109,27 @@ MessageEventRunnable::WorkerRun(JSContex
 {
   if (mBehavior == ParentThreadUnchangedBusyCount) {
     // Don't fire this event if the JS object has been disconnected from the
     // private object.
     if (!aWorkerPrivate->IsAcceptingEvents()) {
       return true;
     }
 
-    if (aWorkerPrivate->IsFrozen() ||
-        aWorkerPrivate->IsParentWindowPaused()) {
-      MOZ_ASSERT(!IsDebuggerRunnable());
-      aWorkerPrivate->QueueRunnable(this);
-      return true;
-    }
+    // Once a window has frozen its workers, their
+    // mMainThreadDebuggeeEventTargets should be paused, and their
+    // WorkerDebuggeeRunnables should not be being executed. The same goes for
+    // WorkerDebuggeeRunnables sent from child to parent workers, but since a
+    // frozen parent worker runs only control runnables anyway, that is taken
+    // care of naturally.
+    MOZ_ASSERT(!aWorkerPrivate->IsFrozen());
+
+    // Similarly for paused windows; all its workers should have been informed.
+    // (Subworkers are unaffected by paused windows.)
+    MOZ_ASSERT(!aWorkerPrivate->IsParentWindowPaused());
 
     aWorkerPrivate->AssertInnerWindowIsCorrect();
 
     return DispatchDOMEvent(aCx, aWorkerPrivate,
                             aWorkerPrivate->ParentEventTargetRef(),
                             !aWorkerPrivate->GetParent());
   }
 
--- a/dom/workers/WorkerError.cpp
+++ b/dom/workers/WorkerError.cpp
@@ -183,22 +183,27 @@ private:
 
     WorkerPrivate* parent = aWorkerPrivate->GetParent();
     if (parent) {
       innerWindowId = 0;
     }
     else {
       AssertIsOnMainThread();
 
-      if (aWorkerPrivate->IsFrozen() ||
-          aWorkerPrivate->IsParentWindowPaused()) {
-        MOZ_ASSERT(!IsDebuggerRunnable());
-        aWorkerPrivate->QueueRunnable(this);
-        return true;
-      }
+      // Once a window has frozen its workers, their
+      // mMainThreadDebuggeeEventTargets should be paused, and their
+      // WorkerDebuggeeRunnables should not be being executed. The same goes for
+      // WorkerDebuggeeRunnables sent from child to parent workers, but since a
+      // frozen parent worker runs only control runnables anyway, that is taken
+      // care of naturally.
+      MOZ_ASSERT(!aWorkerPrivate->IsFrozen());
+
+      // Similarly for paused windows; all its workers should have been informed.
+      // (Subworkers are unaffected by paused windows.)
+      MOZ_ASSERT(!aWorkerPrivate->IsParentWindowPaused());
 
       if (aWorkerPrivate->IsSharedWorker()) {
         aWorkerPrivate->BroadcastErrorToSharedWorkers(aCx, &mReport,
                                                       /* isErrorEvent */ true);
         return true;
       }
 
       // Service workers do not have a main thread parent global, so normal
@@ -268,22 +273,27 @@ private:
 
     // Dispatch may fail if the worker was canceled, no need to report that as
     // an error, so don't call base class PostDispatch.
   }
 
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
-    if (aWorkerPrivate->IsFrozen() ||
-        aWorkerPrivate->IsParentWindowPaused()) {
-      MOZ_ASSERT(!IsDebuggerRunnable());
-      aWorkerPrivate->QueueRunnable(this);
-      return true;
-    }
+    // Once a window has frozen its workers, their
+    // mMainThreadDebuggeeEventTargets should be paused, and their
+    // WorkerDebuggeeRunnables should not be being executed. The same goes for
+    // WorkerDebuggeeRunnables sent from child to parent workers, but since a
+    // frozen parent worker runs only control runnables anyway, that is taken
+    // care of naturally.
+    MOZ_ASSERT(!aWorkerPrivate->IsFrozen());
+
+    // Similarly for paused windows; all its workers should have been informed.
+    // (Subworkers are unaffected by paused windows.)
+    MOZ_ASSERT(!aWorkerPrivate->IsParentWindowPaused());
 
     if (aWorkerPrivate->IsSharedWorker()) {
       aWorkerPrivate->BroadcastErrorToSharedWorkers(aCx, nullptr,
                                                     /* isErrorEvent */ false);
       return true;
     }
 
     if (aWorkerPrivate->IsServiceWorker()) {
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1583,16 +1583,20 @@ WorkerPrivate::Dispatch(already_AddRefed
                  "down!");
       return NS_ERROR_UNEXPECTED;
     }
 
     nsresult rv;
     if (aSyncLoopTarget) {
       rv = aSyncLoopTarget->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);
     } else {
+      // WorkerDebuggeeRunnables don't need any special treatment here. True,
+      // they should not be delivered to a frozen worker. But frozen workers
+      // aren't drawing from the thread's main event queue anyway, only from
+      // mControlQueue.
       rv = mThread->DispatchAnyThread(WorkerThreadFriendKey(), runnable.forget());
     }
 
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
 
     mCondVar.Notify();
@@ -1756,22 +1760,16 @@ WorkerPrivate::Notify(WorkerStatus aStat
     }
 #endif
 
     // Worker never got a chance to run, go ahead and delete it.
     ScheduleDeletion(WorkerPrivate::WorkerNeverRan);
     return true;
   }
 
-  NS_ASSERTION(aStatus != Canceling || mQueuedRunnables.IsEmpty(),
-               "Shouldn't have anything queued!");
-
-  // Anything queued will be discarded.
-  mQueuedRunnables.Clear();
-
   // No Canceling timeout is needed.
   if (mCancelingTimer) {
     mCancelingTimer->Cancel();
     mCancelingTimer = nullptr;
   }
 
   RefPtr<NotifyRunnable> runnable = new NotifyRunnable(this, aStatus);
   return runnable->Dispatch();
@@ -1808,16 +1806,31 @@ WorkerPrivate::Freeze(nsPIDOMWindowInner
 
     if (!allFrozen || mParentFrozen) {
       return true;
     }
   }
 
   mParentFrozen = true;
 
+  // WorkerDebuggeeRunnables sent from a worker to content must not be delivered
+  // while the worker is frozen.
+  //
+  // Since a top-level worker and all its children share the same
+  // mMainThreadDebuggeeEventTarget, it's sufficient to do this only in the
+  // top-level worker.
+  if (aWindow) {
+    // This is called from WorkerPrivate construction, and We may not have
+    // allocated mMainThreadDebuggeeEventTarget yet.
+    if (mMainThreadDebuggeeEventTarget) {
+      // Pausing a ThrottledEventQueue is infallible.
+      MOZ_ALWAYS_SUCCEEDS(mMainThreadDebuggeeEventTarget->SetIsPaused(true));
+    }
+  }
+
   {
     MutexAutoLock lock(mMutex);
 
     if (mParentStatus >= Canceling) {
       return true;
     }
   }
 
@@ -1865,53 +1878,62 @@ WorkerPrivate::Thaw(nsPIDOMWindowInner* 
       return true;
     }
   }
 
   MOZ_ASSERT(mParentFrozen);
 
   mParentFrozen = false;
 
+  // Delivery of WorkerDebuggeeRunnables to the window may resume.
+  //
+  // Since a top-level worker and all its children share the same
+  // mMainThreadDebuggeeEventTarget, it's sufficient to do this only in the
+  // top-level worker.
+  if (aWindow) {
+    // Since the worker is no longer frozen, only a paused parent window should
+    // require the queue to remain paused.
+    //
+    // This can only fail if the ThrottledEventQueue cannot dispatch its executor
+    // to the main thread, in which case the main thread was never going to draw
+    // runnables from it anyway, so the failure doesn't matter.
+    Unused << mMainThreadDebuggeeEventTarget->SetIsPaused(IsParentWindowPaused());
+  }
+
   {
     MutexAutoLock lock(mMutex);
 
     if (mParentStatus >= Canceling) {
       return true;
     }
   }
 
   EnableDebugger();
 
-  // Execute queued runnables before waking up the worker, otherwise the worker
-  // could post new messages before we run those that have been queued.
-  if (!IsParentWindowPaused() && !mQueuedRunnables.IsEmpty()) {
-    MOZ_ASSERT(IsDedicatedWorker());
-
-    nsTArray<nsCOMPtr<nsIRunnable>> runnables;
-    mQueuedRunnables.SwapElements(runnables);
-
-    for (uint32_t index = 0; index < runnables.Length(); index++) {
-      runnables[index]->Run();
-    }
-  }
-
   RefPtr<ThawRunnable> runnable = new ThawRunnable(this);
   if (!runnable->Dispatch()) {
     return false;
   }
 
   return true;
 }
 
 void
 WorkerPrivate::ParentWindowPaused()
 {
   AssertIsOnMainThread();
   MOZ_ASSERT_IF(IsDedicatedWorker(), mParentWindowPausedDepth == 0);
   mParentWindowPausedDepth += 1;
+
+  // This is called from WorkerPrivate construction, and we may not have
+  // allocated mMainThreadDebuggeeEventTarget yet.
+  if (mMainThreadDebuggeeEventTarget) {
+    // Pausing a ThrottledEventQueue is infallible.
+    MOZ_ALWAYS_SUCCEEDS(mMainThreadDebuggeeEventTarget->SetIsPaused(true));
+  }
 }
 
 void
 WorkerPrivate::ParentWindowResumed()
 {
   AssertIsOnMainThread();
 
   MOZ_ASSERT(mParentWindowPausedDepth > 0);
@@ -1924,28 +1946,23 @@ WorkerPrivate::ParentWindowResumed()
   {
     MutexAutoLock lock(mMutex);
 
     if (mParentStatus >= Canceling) {
       return;
     }
   }
 
-  // Execute queued runnables before waking up, otherwise the worker could post
-  // new messages before we run those that have been queued.
-  if (!IsFrozen() && !mQueuedRunnables.IsEmpty()) {
-    MOZ_ASSERT(IsDedicatedWorker());
-
-    nsTArray<nsCOMPtr<nsIRunnable>> runnables;
-    mQueuedRunnables.SwapElements(runnables);
-
-    for (uint32_t index = 0; index < runnables.Length(); index++) {
-      runnables[index]->Run();
-    }
-  }
+  // Since the window is no longer paused, the queue should only remain paused
+  // if the worker is frozen.
+  //
+  // This can only fail if the ThrottledEventQueue cannot dispatch its executor
+  // to the main thread, in which case the main thread was never going to draw
+  // runnables from it anyway, so the failure doesn't matter.
+  Unused << mMainThreadDebuggeeEventTarget->SetIsPaused(IsFrozen());
 }
 
 void
 WorkerPrivate::PropagateFirstPartyStorageAccessGranted()
 {
   AssertIsOnParentThread();
 
   {
@@ -2709,30 +2726,35 @@ WorkerPrivate::WorkerPrivate(WorkerPriva
   nsCOMPtr<nsISerialEventTarget> target;
 
   // A child worker just inherits the parent workers ThrottledEventQueue
   // and main thread target for now.  This is mainly due to the restriction
   // that ThrottledEventQueue can only be created on the main thread at the
   // moment.
   if (aParent) {
     mMainThreadEventTarget = aParent->mMainThreadEventTarget;
+    mMainThreadDebuggeeEventTarget = aParent->mMainThreadDebuggeeEventTarget;
     return;
   }
 
   MOZ_ASSERT(NS_IsMainThread());
   target = GetWindow() ? GetWindow()->EventTargetFor(TaskCategory::Worker) : nullptr;
 
   if (!target) {
     target = GetMainThreadSerialEventTarget();
     MOZ_DIAGNOSTIC_ASSERT(target);
   }
 
   // Throttle events to the main thread using a ThrottledEventQueue specific to
   // this tree of worker threads.
   mMainThreadEventTarget = ThrottledEventQueue::Create(target);
+  mMainThreadDebuggeeEventTarget = ThrottledEventQueue::Create(target);
+  if (IsParentWindowPaused() || IsFrozen()) {
+    MOZ_ALWAYS_SUCCEEDS(mMainThreadDebuggeeEventTarget->SetIsPaused(true));
+  }
 }
 
 WorkerPrivate::~WorkerPrivate()
 {
   DropJSObjects(this);
 
   mWorkerControlEventTarget->ForgetWorkerPrivate(this);
 
@@ -3296,19 +3318,21 @@ WorkerPrivate::DoRunLoop(JSContext* aCx)
 
     if (!debuggerRunnablesPending && !normalRunnablesPending) {
       // Both the debugger event queue and the normal event queue has been
       // exhausted, cancel the periodic GC timer and schedule the idle GC timer.
       SetGCTimerMode(IdleTimer);
     }
 
     // If the worker thread is spamming the main thread faster than it can
-    // process the work, then pause the worker thread until the MT catches
-    // up.
-    if (mMainThreadEventTarget->Length() > 5000) {
+    // process the work, then pause the worker thread until the main thread
+    // catches up.
+    size_t queuedEvents = mMainThreadEventTarget->Length() +
+                          mMainThreadDebuggeeEventTarget->Length();
+    if (queuedEvents > 5000) {
       mMainThreadEventTarget->AwaitIdle();
     }
   }
 
   MOZ_CRASH("Shouldn't get here!");
 }
 
 void
@@ -3353,16 +3377,23 @@ WorkerPrivate::DispatchToMainThread(nsIR
 
 nsresult
 WorkerPrivate::DispatchToMainThread(already_AddRefed<nsIRunnable> aRunnable,
                                     uint32_t aFlags)
 {
   return mMainThreadEventTarget->Dispatch(std::move(aRunnable), aFlags);
 }
 
+nsresult
+WorkerPrivate::DispatchDebuggeeToMainThread(already_AddRefed<WorkerDebuggeeRunnable> aRunnable,
+                                            uint32_t aFlags)
+{
+  return mMainThreadDebuggeeEventTarget->Dispatch(std::move(aRunnable), aFlags);
+}
+
 nsISerialEventTarget*
 WorkerPrivate::ControlEventTarget()
 {
   return mWorkerControlEventTarget;
 }
 
 nsISerialEventTarget*
 WorkerPrivate::HybridEventTarget()
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -48,16 +48,17 @@ class SharedWorker;
 class WorkerControlRunnable;
 class WorkerCSPEventListener;
 class WorkerDebugger;
 class WorkerDebuggerGlobalScope;
 class WorkerErrorReport;
 class WorkerEventTarget;
 class WorkerGlobalScope;
 class WorkerRunnable;
+class WorkerDebuggeeRunnable;
 class WorkerThread;
 
 // SharedMutex is a small wrapper around an (internal) reference-counted Mutex
 // object. It exists to avoid changing a lot of code to use Mutex* instead of
 // Mutex&.
 class SharedMutex
 {
   typedef mozilla::Mutex Mutex;
@@ -530,16 +531,20 @@ public:
   nsresult
   DispatchToMainThread(nsIRunnable* aRunnable,
                        uint32_t aFlags = NS_DISPATCH_NORMAL);
 
   nsresult
   DispatchToMainThread(already_AddRefed<nsIRunnable> aRunnable,
                        uint32_t aFlags = NS_DISPATCH_NORMAL);
 
+  nsresult
+  DispatchDebuggeeToMainThread(already_AddRefed<WorkerDebuggeeRunnable> aRunnable,
+                               uint32_t aFlags = NS_DISPATCH_NORMAL);
+
   // Get an event target that will dispatch runnables as control runnables on
   // the worker thread.  Implement nsICancelableRunnable if you wish to take
   // action on cancelation.
   nsISerialEventTarget*
   ControlEventTarget();
 
   // Get an event target that will attempt to dispatch a normal WorkerRunnable,
   // but if that fails will then fall back to a control runnable.
@@ -1063,23 +1068,16 @@ public:
   // top level script.
   void
   SetLoadingWorkerScript(bool aLoadingWorkerScript)
   {
     // any thread
     mLoadingWorkerScript = aLoadingWorkerScript;
   }
 
-  void
-  QueueRunnable(nsIRunnable* aRunnable)
-  {
-    AssertIsOnParentThread();
-    mQueuedRunnables.AppendElement(aRunnable);
-  }
-
   bool
   RegisterSharedWorker(SharedWorker* aSharedWorker, MessagePort* aPort);
 
   void
   BroadcastErrorToSharedWorkers(JSContext* aCx,
                                 const WorkerErrorReport* aReport,
                                 bool aIsErrorEvent);
 
@@ -1370,16 +1368,20 @@ private:
   RefPtr<WorkerDebuggerGlobalScope> mDebuggerScope;
   nsTArray<WorkerPrivate*> mChildWorkers;
   nsTObserverArray<WorkerHolder*> mHolders;
   nsTArray<nsAutoPtr<TimeoutInfo>> mTimeouts;
   RefPtr<ThrottledEventQueue> mMainThreadEventTarget;
   RefPtr<WorkerEventTarget> mWorkerControlEventTarget;
   RefPtr<WorkerEventTarget> mWorkerHybridEventTarget;
 
+  // A pauseable queue for WorkerDebuggeeRunnables directed at the main thread.
+  // See WorkerDebuggeeRunnable for details.
+  RefPtr<ThrottledEventQueue> mMainThreadDebuggeeEventTarget;
+
   struct SyncLoopInfo
   {
     explicit SyncLoopInfo(EventTarget* aEventTarget);
 
     RefPtr<EventTarget> mEventTarget;
     bool mCompleted;
     bool mResult;
 #ifdef DEBUG
@@ -1403,19 +1405,16 @@ private:
 
   // fired on the main thread if the worker script fails to load
   nsCOMPtr<nsIRunnable> mLoadFailedRunnable;
 
   RefPtr<PerformanceStorage> mPerformanceStorage;
 
   RefPtr<WorkerCSPEventListener> mCSPEventListener;
 
-  // Only used for top level workers.
-  nsTArray<nsCOMPtr<nsIRunnable>> mQueuedRunnables;
-
   // Protected by mMutex.
   nsTArray<RefPtr<WorkerRunnable>> mPreStartRunnables;
 
   // Only touched on the parent thread (currently this is always the main
   // thread as SharedWorkers are always top-level).
   nsTArray<RefPtr<SharedWorker>> mSharedWorkers;
 
   JS::UniqueChars mDefaultLocale; // nulled during worker JSContext init
--- a/dom/workers/WorkerRunnable.cpp
+++ b/dom/workers/WorkerRunnable.cpp
@@ -114,16 +114,23 @@ WorkerRunnable::DispatchInternal()
   }
 
   MOZ_ASSERT(mBehavior == ParentThreadUnchangedBusyCount);
 
   if (WorkerPrivate* parent = mWorkerPrivate->GetParent()) {
     return NS_SUCCEEDED(parent->Dispatch(runnable.forget()));
   }
 
+  if (IsDebuggeeRunnable()) {
+    RefPtr<WorkerDebuggeeRunnable> debuggeeRunnable =
+      runnable.forget().downcast<WorkerDebuggeeRunnable>();
+    return NS_SUCCEEDED(mWorkerPrivate->DispatchDebuggeeToMainThread(debuggeeRunnable.forget(),
+                                                                     NS_DISPATCH_NORMAL));
+  }
+
   return NS_SUCCEEDED(mWorkerPrivate->DispatchToMainThread(runnable.forget()));
 }
 
 void
 WorkerRunnable::PostDispatch(WorkerPrivate* aWorkerPrivate,
                              bool aDispatchResult)
 {
   MOZ_ASSERT(aWorkerPrivate);
@@ -743,10 +750,29 @@ WorkerProxyToMainThreadRunnable::PostDis
 }
 
 void
 WorkerProxyToMainThreadRunnable::ReleaseWorker()
 {
   mWorkerRef = nullptr;
 }
 
+bool
+WorkerDebuggeeRunnable::PreDispatch(WorkerPrivate* aWorkerPrivate)
+{
+  if (mBehavior == ParentThreadUnchangedBusyCount) {
+    RefPtr<StrongWorkerRef> strongRef =
+      StrongWorkerRef::Create(aWorkerPrivate, "WorkerDebuggeeRunnable::mSender");
+    if (!strongRef) {
+      return false;
+    }
+
+    mSender = new ThreadSafeWorkerRef(strongRef);
+    if (!mSender) {
+      return false;
+    }
+  }
+
+  return WorkerRunnable::PreDispatch(aWorkerPrivate);
+}
+
 } // dom namespace
 } // mozilla namespace
--- a/dom/workers/WorkerRunnable.h
+++ b/dom/workers/WorkerRunnable.h
@@ -536,22 +536,37 @@ private:
 class WorkerDebuggeeRunnable : public WorkerRunnable
 {
  protected:
   WorkerDebuggeeRunnable(WorkerPrivate* aWorkerPrivate,
                          TargetAndBusyBehavior aBehavior = ParentThreadUnchangedBusyCount)
     : WorkerRunnable(aWorkerPrivate, aBehavior)
   { }
 
+  bool
+  PreDispatch(WorkerPrivate* aWorkerPrivate) override;
+
  private:
   // This override is deliberately private: it doesn't make sense to call it if
   // we know statically that we are a WorkerDebuggeeRunnable.
   bool
   IsDebuggeeRunnable() const override
   {
     return true;
   }
+
+  // Runnables sent upwards, to the content window or parent worker, must keep
+  // their sender alive until they are delivered: they check back with the
+  // sender in case it has been terminated after having dispatched the runnable
+  // (in which case it should not be acted upon); and runnables sent to content
+  // wait until delivery to determine the target window, since
+  // WorkerPrivate::GetWindow may only be used on the main thread.
+  //
+  // Runnables sent downwards, from content to a worker or from a worker to a
+  // child, keep the sender alive because they are WorkerThreadModifyBusyCount
+  // runnables, and so leave this null.
+  RefPtr<ThreadSafeWorkerRef> mSender;
 };
 
 } // dom namespace
 } // mozilla namespace
 
 #endif // mozilla_dom_workers_workerrunnable_h__