Bug 1450644 - Better shutdown approach for Workers - part 2 - Timeout + ControlRunnable, r=asuth
☠☠ backed out by 73615fe67ab6 ☠ ☠
authorAndrea Marchesini <amarchesini@mozilla.com>
Tue, 17 Apr 2018 20:51:04 +0200
changeset 414180 cbe3ad4833b670227c4b229dbbe29954d4b9fc25
parent 414179 0d2088370d0c8bad625758d8c1cc49761293501b
child 414181 08239799d43e6ddb85c9d149622151702ddac6f6
push id33861
push userccoroiu@mozilla.com
push dateWed, 18 Apr 2018 10:50:38 +0000
treeherdermozilla-central@4af4ae0aee55 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1450644
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1450644 - Better shutdown approach for Workers - part 2 - Timeout + ControlRunnable, r=asuth
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
dom/workers/test/WorkerDebugger_sharedWorker.js
dom/workers/test/test_WorkerDebugger.xul
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -67,16 +67,18 @@
 #endif
 
 // JS_MaybeGC will run once every second during normal execution.
 #define PERIODIC_GC_TIMER_DELAY_SEC 1
 
 // A shrinking GC will run five seconds after the last event is processed.
 #define IDLE_GC_TIMER_DELAY_SEC 5
 
+#define CANCELING_TIMEOUT 30000 // 30 seconds
+
 static mozilla::LazyLogModule sWorkerPrivateLog("WorkerPrivate");
 static mozilla::LazyLogModule sWorkerTimeoutsLog("WorkerTimeouts");
 
 mozilla::LogModule*
 WorkerLog()
 {
   return sWorkerPrivateLog;
 }
@@ -990,16 +992,60 @@ public:
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
     aWorkerPrivate->Cancel();
     return true;
   }
 };
 
+// A runnable to cancel the worker from the parent process.
+class CancelingWithTimeoutOnParentRunnable final : public WorkerControlRunnable
+{
+public:
+  explicit CancelingWithTimeoutOnParentRunnable(WorkerPrivate* aWorkerPrivate)
+    : WorkerControlRunnable(aWorkerPrivate, ParentThreadUnchangedBusyCount)
+  {}
+
+  bool
+  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
+  {
+    aWorkerPrivate->AssertIsOnParentThread();
+    aWorkerPrivate->StartCancelingTimer();
+    return true;
+  }
+};
+
+class CancelingTimerCallback final : public nsITimerCallback
+{
+public:
+  NS_DECL_ISUPPORTS
+
+  explicit CancelingTimerCallback(WorkerPrivate* aWorkerPrivate)
+    : mWorkerPrivate(aWorkerPrivate)
+  {}
+
+  NS_IMETHOD
+  Notify(nsITimer* aTimer) override
+  {
+    mWorkerPrivate->AssertIsOnParentThread();
+    mWorkerPrivate->Cancel();
+    return NS_OK;
+  }
+
+private:
+  ~CancelingTimerCallback() = default;
+
+  // Raw pointer here is OK because the timer is canceled during the shutdown
+  // steps.
+  WorkerPrivate* mWorkerPrivate;
+};
+
+NS_IMPL_ISUPPORTS(CancelingTimerCallback, nsITimerCallback)
+
 // This runnable starts the canceling of a worker after a self.close().
 class CancelingRunnable final : public Runnable
 {
 public:
   CancelingRunnable()
     : Runnable("CancelingRunnable")
   {}
 
@@ -1775,16 +1821,22 @@ WorkerPrivate::NotifyPrivate(WorkerStatu
   }
 
   NS_ASSERTION(aStatus != Terminating || 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();
 }
 
 bool
 WorkerPrivate::Freeze(nsPIDOMWindowInner* aWindow)
 {
   AssertIsOnParentThread();
@@ -4533,16 +4585,23 @@ WorkerPrivate::NotifyInternal(WorkerStat
   if (aStatus == Closing) {
     if (mSyncLoopStack.IsEmpty()) {
       // Here we use a normal runnable to know when the current JS chunk of code
       // is finished. We cannot use a WorkerRunnable because they are not
       // accepted any more by the worker, and we do not want to use a
       // WorkerControlRunnable because they are immediately executed.
       RefPtr<CancelingRunnable> r = new CancelingRunnable();
       mThread->nsThread::Dispatch(r.forget(), NS_DISPATCH_NORMAL);
+
+      // At the same time, we want to be sure that we interrupt infinite loops.
+      // The following runnable starts a timer that cancel the worker, from the
+      // parent thread, after CANCELING_TIMEOUT millseconds.
+      RefPtr<CancelingWithTimeoutOnParentRunnable> rr =
+        new CancelingWithTimeoutOnParentRunnable(this);
+      rr->Dispatch();
     }
     return true;
   }
 
   MOZ_ASSERT(aStatus == Terminating ||
              aStatus == Canceling ||
              aStatus == Killing);
 
@@ -4901,16 +4960,55 @@ WorkerPrivate::RescheduleTimeoutTimer(JS
     JS_ReportErrorASCII(aCx, "Failed to start timer!");
     return false;
   }
 
   return true;
 }
 
 void
+WorkerPrivate::StartCancelingTimer()
+{
+  AssertIsOnParentThread();
+
+  auto raii = MakeScopeExit([&] {
+    mCancelingTimer = nullptr;
+  });
+
+  MOZ_ASSERT(!mCancelingTimer);
+
+  if (WorkerPrivate* parent = GetParent()) {
+    mCancelingTimer = NS_NewTimer(parent->ControlEventTarget());
+  } else {
+    mCancelingTimer = NS_NewTimer();
+  }
+
+  if (NS_WARN_IF(!mCancelingTimer)) {
+    return;
+  }
+
+  // This is not needed if we are already in an advanced shutdown state.
+  {
+    MutexAutoLock lock(mMutex);
+    if (ParentStatus() >= Terminating) {
+      return;
+    }
+  }
+
+  RefPtr<CancelingTimerCallback> callback = new CancelingTimerCallback(this);
+  nsresult rv = mCancelingTimer->InitWithCallback(callback, CANCELING_TIMEOUT,
+                                                  nsITimer::TYPE_ONE_SHOT);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return;
+  }
+
+  raii.release();
+}
+
+void
 WorkerPrivate::UpdateContextOptionsInternal(
                                     JSContext* aCx,
                                     const JS::ContextOptions& aContextOptions)
 {
   AssertIsOnWorkerThread();
 
   JS::ContextOptionsRef(aCx) = aContextOptions;
 
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -1204,16 +1204,19 @@ public:
   { }
 #endif
 
 #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
   bool
   PrincipalIsValid() const;
 #endif
 
+  void
+  StartCancelingTimer();
+
 private:
   WorkerPrivate(WorkerPrivate* aParent,
                 const nsAString& aScriptURL, bool aIsChromeWorker,
                 WorkerType aWorkerType, const nsAString& aWorkerName,
                 const nsACString& aServiceWorkerScope,
                 WorkerLoadInfo& aLoadInfo);
 
   ~WorkerPrivate();
@@ -1416,16 +1419,18 @@ private:
   // This is only modified on the worker thread, but in DEBUG builds
   // AssertValidSyncLoop function iterates it on other threads. Therefore
   // modifications are done with mMutex held *only* in DEBUG builds.
   nsTArray<nsAutoPtr<SyncLoopInfo>> mSyncLoopStack;
 
   nsCOMPtr<nsITimer> mTimer;
   nsCOMPtr<nsITimerCallback> mTimerRunnable;
 
+  nsCOMPtr<nsITimer> mCancelingTimer;
+
   nsCOMPtr<nsITimer> mGCTimer;
 
   RefPtr<MemoryReporter> mMemoryReporter;
 
   // fired on the main thread if the worker script fails to load
   nsCOMPtr<nsIRunnable> mLoadFailedRunnable;
 
   RefPtr<PerformanceStorage> mPerformanceStorage;
--- a/dom/workers/test/WorkerDebugger_sharedWorker.js
+++ b/dom/workers/test/WorkerDebugger_sharedWorker.js
@@ -1,11 +1,17 @@
 "use strict";
 
 self.onconnect = function (event) {
   event.ports[0].onmessage = function (event) {
     switch (event.data) {
     case "close":
       close();
       break;
+
+    case "close_loop":
+      close();
+      // Let's loop forever.
+      while(1) {}
+      break;
     }
   };
 };
--- a/dom/workers/test/test_WorkerDebugger.xul
+++ b/dom/workers/test/test_WorkerDebugger.xul
@@ -88,28 +88,44 @@
              "debugger is not registered again.");
         let listener = {
           onRegistered: function () {
             ok(false,
                "Shared worker debugger should not be registered again.");
           },
         };
         wdm.addListener(listener);
+
         worker = new SharedWorker(SHARED_WORKER_URL);
 
         info("Send a message to the shared worker to tell it to close " +
              "itself, and wait for its debugger to be closed.");
         promise = waitForMultiple([
           waitForUnregister(SHARED_WORKER_URL),
           waitForDebuggerClose(sharedDbg)
         ]);
         worker.port.start();
         worker.port.postMessage("close");
         await promise;
 
+        promise = waitForRegister(SHARED_WORKER_URL);
+        worker = new SharedWorker(SHARED_WORKER_URL);
+        sharedDbg = await promise;
+
+        info("Send a message to the shared worker to tell it to close " +
+             "itself, then loop forever, and wait for its debugger to be closed.");
+        promise = waitForMultiple([
+          waitForUnregister(SHARED_WORKER_URL),
+          waitForDebuggerClose(sharedDbg)
+        ]);
+
+        worker.port.start();
+        worker.port.postMessage("close_loop");
+        await promise;
+
         wdm.removeListener(listener);
         SimpleTest.finish();
       })();
     }
 
   ]]>
   </script>