Bug 1249102. Make overrides of WorkerRunnable::PostRun a bit more consistent. r=khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 18 Feb 2016 18:02:51 -0500
changeset 321138 298964285e1f1f1b88e4cd7ec0cda6ab489c7aed
parent 321137 6cb092fbf4fb5122b9a4aa845eadafa57aea09c1
child 321139 ea64b102f2b55c55ab0aa4b91f0f6c8b88e461e6
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs1249102
milestone47.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 1249102. Make overrides of WorkerRunnable::PostRun a bit more consistent. r=khuey Specifically we make the following changes: 1) Remove WorkerSameThreadRunnable::PostRun, because it does exactly the same things as WorkerRunnable::PostRun. 2) Always treat ModifyBusyCountFromWorker as infallible in terms of throwing JS exceptions. 3) Change ExtendableFunctionalEventWorkerRunnable::PostRun to properly call its superclass PostRun so we will correctly decrement the busy count our PreDispatch incremented. 4) Document why some overrides of PreDispatch/PostDispatch are needed and don't call into the superclass
dom/base/WebSocket.cpp
dom/notification/Notification.cpp
dom/workers/ServiceWorkerPrivate.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerRunnable.cpp
dom/workers/WorkerRunnable.h
--- a/dom/base/WebSocket.cpp
+++ b/dom/base/WebSocket.cpp
@@ -2518,23 +2518,29 @@ public:
   void PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
   {
     aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false);
   }
 
   bool
   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
+    // We don't call WorkerRunnable::PreDispatch because it would assert the
+    // wrong thing about which thread we're on.
+    AssertIsOnMainThread();
     return true;
   }
 
   void
   PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                bool aDispatchResult)
   {
+    // We don't call WorkerRunnable::PostDispatch because it would assert the
+    // wrong thing about which thread we're on.
+    AssertIsOnMainThread();
   }
 
 private:
   RefPtr<WebSocketImpl> mImpl;
 };
 
 } // namespace
 
@@ -2691,23 +2697,31 @@ public:
   void PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
   {
     aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false);
   }
 
   bool
   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
+    // We don't call WorkerRunnable::PreDispatch because it would assert the
+    // wrong thing about which thread we're on.  We're on whichever thread the
+    // channel implementation is running on (probably the main thread or socket
+    // transport thread).
     return true;
   }
 
   void
   PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                bool aDispatchResult)
   {
+    // We don't call WorkerRunnable::PreDispatch because it would assert the
+    // wrong thing about which thread we're on.  We're on whichever thread the
+    // channel implementation is running on (probably the main thread or socket
+    // transport thread).
   }
 
 private:
   nsCOMPtr<nsIRunnable> mEvent;
 };
 
 } // namespace
 
--- a/dom/notification/Notification.cpp
+++ b/dom/notification/Notification.cpp
@@ -370,23 +370,29 @@ protected:
   explicit NotificationWorkerRunnable(WorkerPrivate* aWorkerPrivate)
     : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
   {
   }
 
   bool
   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
+    // We don't call WorkerRunnable::PreDispatch because it would assert the
+    // wrong thing about which thread we're on.
+    AssertIsOnMainThread();
     return true;
   }
 
   void
   PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                bool aDispatchResult) override
   {
+    // We don't call WorkerRunnable::PostDispatch because it would assert the
+    // wrong thing about which thread we're on.
+    AssertIsOnMainThread();
   }
 
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
     aWorkerPrivate->AssertIsOnWorkerThread();
     aWorkerPrivate->ModifyBusyCountFromWorker(aCx, true);
     WorkerRunInternal(aCx, aWorkerPrivate);
--- a/dom/workers/ServiceWorkerPrivate.cpp
+++ b/dom/workers/ServiceWorkerPrivate.cpp
@@ -314,16 +314,18 @@ public:
   }
 
   void
   PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
   {
     nsCOMPtr<nsIRunnable> runnable =
       new RegistrationUpdateRunnable(mRegistration, true /* time check */);
     NS_DispatchToMainThread(runnable.forget());
+
+    ExtendableEventWorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult);
   }
 };
 
 /*
  * Fires 'install' event on the ServiceWorkerGlobalScope. Modifies busy count
  * since it fires the event. This is ok since there can't be nested
  * ServiceWorkers, so the parent thread -> worker thread requirement for
  * runnables is satisfied.
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -605,19 +605,17 @@ private:
   virtual void
   PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
           override
   {
     // Report errors.
     WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult);
 
     // Match the busy count increase from NotifyRunnable.
-    if (!aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false)) {
-      JS_ReportPendingException(aCx);
-    }
+    aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false);
 
     aWorkerPrivate->CloseHandlerFinished();
   }
 };
 
 class MessageEventRunnable final : public WorkerRunnable
                                  , public StructuredCloneHolder
 {
--- a/dom/workers/WorkerRunnable.cpp
+++ b/dom/workers/WorkerRunnable.cpp
@@ -218,19 +218,17 @@ WorkerRunnable::PostRun(JSContext* aCx, 
       break;
 
     default:
       MOZ_ASSERT_UNREACHABLE("Unknown behavior!");
   }
 #endif
 
   if (mBehavior == WorkerThreadModifyBusyCount) {
-    if (!aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false)) {
-      aRunResult = false;
-    }
+    aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false);
   }
 
   if (!aRunResult) {
     JS_ReportPendingException(aCx);
   }
 }
 
 // static
@@ -639,43 +637,31 @@ WorkerCheckAPIExposureOnMainThreadRunnab
   rv.SuppressException();
   return ok;
 }
 
 bool
 WorkerSameThreadRunnable::PreDispatch(JSContext* aCx,
                                       WorkerPrivate* aWorkerPrivate)
 {
+  // We don't call WorkerRunnable::PreDispatch, because we're using
+  // WorkerThreadModifyBusyCount for mBehavior, and WorkerRunnable will assert
+  // that PreDispatch is on the parent thread in that case.
   aWorkerPrivate->AssertIsOnWorkerThread();
   return true;
 }
 
 void
 WorkerSameThreadRunnable::PostDispatch(JSContext* aCx,
                                        WorkerPrivate* aWorkerPrivate,
                                        bool aDispatchResult)
 {
+  // We don't call WorkerRunnable::PostDispatch, because we're using
+  // WorkerThreadModifyBusyCount for mBehavior, and WorkerRunnable will assert
+  // that PostDispatch is on the parent thread in that case.
   aWorkerPrivate->AssertIsOnWorkerThread();
   if (aDispatchResult) {
     DebugOnly<bool> willIncrement = aWorkerPrivate->ModifyBusyCountFromWorker(aCx, true);
     // Should never fail since if this thread is still running, so should the
     // parent and it should be able to process a control runnable.
     MOZ_ASSERT(willIncrement);
   }
 }
-
-void
-WorkerSameThreadRunnable::PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
-                                  bool aRunResult)
-{
-  MOZ_ASSERT(aCx);
-  MOZ_ASSERT(aWorkerPrivate);
-
-  aWorkerPrivate->AssertIsOnWorkerThread();
-
-  DebugOnly<bool> willDecrement = aWorkerPrivate->ModifyBusyCountFromWorker(aCx, false);
-  MOZ_ASSERT(willDecrement);
-
-  if (!aRunResult) {
-    JS_ReportPendingException(aCx);
-  }
-}
-
--- a/dom/workers/WorkerRunnable.h
+++ b/dom/workers/WorkerRunnable.h
@@ -400,19 +400,18 @@ protected:
 
   virtual bool
   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override;
 
   virtual void
   PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                bool aDispatchResult) override;
 
-  virtual void
-  PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
-          bool aRunResult) override;
+  // We just delegate PostRun to WorkerRunnable, since it does exactly
+  // what we want.
 };
 
 // Base class for the runnable objects, which makes a synchronous call to
 // dispatch the tasks from the worker thread to the main thread.
 //
 // Note that the derived class must override MainThreadRun.
 class WorkerMainThreadRunnable : public nsRunnable
 {