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

--HG--
extra : moz-landing-system : lando

dom/workers/MessageEventRunnable.cpp

@@ -114,12 +114,17 @@ MessageEventRunnable::WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
       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();
 

dom/workers/WorkerError.cpp

@@ -188,12 +188,17 @@ private:
     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,
@@ -273,12 +278,17 @@ private:
   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,

dom/workers/WorkerPrivate.cpp

@@ -1588,6 +1588,10 @@ WorkerPrivate::Dispatch(already_AddRefed<WorkerRunnable> aRunnable,
     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());
     }
 
@@ -1761,12 +1765,6 @@ WorkerPrivate::Notify(WorkerStatus aStatus)
     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();
@@ -1813,6 +1811,21 @@ WorkerPrivate::Freeze(nsPIDOMWindowInner* aWindow)
 
   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);
 
@@ -1870,6 +1883,21 @@ WorkerPrivate::Thaw(nsPIDOMWindowInner* aWindow)
 
   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);
 
@@ -1880,19 +1908,6 @@ WorkerPrivate::Thaw(nsPIDOMWindowInner* aWindow)
 
   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;
@@ -1907,6 +1922,13 @@ 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
@@ -1929,18 +1951,13 @@ WorkerPrivate::ParentWindowResumed()
     }
   }
 
-  // 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
@@ -2714,6 +2731,7 @@ WorkerPrivate::WorkerPrivate(WorkerPrivate* aParent,
   // moment.
   if (aParent) {
     mMainThreadEventTarget = aParent->mMainThreadEventTarget;
+    mMainThreadDebuggeeEventTarget = aParent->mMainThreadDebuggeeEventTarget;
     return;
   }
 
@@ -2728,6 +2746,10 @@ WorkerPrivate::WorkerPrivate(WorkerPrivate* aParent,
   // 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()
@@ -3301,9 +3323,11 @@ WorkerPrivate::DoRunLoop(JSContext* aCx)
     }
 
     // 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();
     }
   }
@@ -3358,6 +3382,13 @@ WorkerPrivate::DispatchToMainThread(already_AddRefed<nsIRunnable> aRunnable,
   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()
 {

dom/workers/WorkerPrivate.h

@@ -53,6 +53,7 @@ class WorkerErrorReport;
 class WorkerEventTarget;
 class WorkerGlobalScope;
 class WorkerRunnable;
+class WorkerDebuggeeRunnable;
 class WorkerThread;
 
 // SharedMutex is a small wrapper around an (internal) reference-counted Mutex
@@ -535,6 +536,10 @@ public:
   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.
@@ -1068,13 +1073,6 @@ public:
     mLoadingWorkerScript = aLoadingWorkerScript;
   }
 
-  void
-  QueueRunnable(nsIRunnable* aRunnable)
-  {
-    AssertIsOnParentThread();
-    mQueuedRunnables.AppendElement(aRunnable);
-  }
-
   bool
   RegisterSharedWorker(SharedWorker* aSharedWorker, MessagePort* aPort);
 
@@ -1375,6 +1373,10 @@ private:
   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);
@@ -1408,9 +1410,6 @@ private:
 
   RefPtr<WorkerCSPEventListener> mCSPEventListener;
 
-  // Only used for top level workers.
-  nsTArray<nsCOMPtr<nsIRunnable>> mQueuedRunnables;
-
   // Protected by mMutex.
   nsTArray<RefPtr<WorkerRunnable>> mPreStartRunnables;
 

dom/workers/WorkerRunnable.cpp

@@ -119,6 +119,13 @@ WorkerRunnable::DispatchInternal()
     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()));
 }
 
@@ -748,5 +755,24 @@ 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

dom/workers/WorkerRunnable.h

@@ -541,6 +541,9 @@ class WorkerDebuggeeRunnable : public WorkerRunnable
     : 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.
@@ -549,6 +552,18 @@ class WorkerDebuggeeRunnable : public WorkerRunnable
   {
     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