Bug 1072144 part 3. Hoist the exception reporting out of WorkerRunnable::PostRun into WorkerRunnable::Run and make it unconditional. r=khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 01 Mar 2016 16:52:26 -0500
changeset 322820 9c83fcb222d0c5d3ee0d302b7cf60af7d655bec3
parent 322819 b2fcba1890b25f62d75b2af9270c16ffe159bd74
child 322821 d8745ad21a39a7e1a23e5571ff5ab582b51da68e
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
bugs1072144
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 1072144 part 3. Hoist the exception reporting out of WorkerRunnable::PostRun into WorkerRunnable::Run and make it unconditional. r=khuey
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerRunnable.cpp
dom/workers/WorkerRunnable.h
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -509,19 +509,19 @@ private:
     // Explicitly ignore NS_BINDING_ABORTED on rv.  Or more precisely, still
     // return false and don't SetWorkerScriptExecutedSuccessfully() in that
     // case, but don't throw anything on aCx.  The idea is to not dispatch error
     // events if our load is canceled with that error code.
     if (rv.ErrorCodeIs(NS_BINDING_ABORTED)) {
       rv.SuppressException();
       return false;
     }
-    // Make sure to propagate exceptions from rv onto aCx, so that our PostRun
-    // can report it.  We do this for all failures on rv, because now we're
-    // using rv to track all the state we care about.
+    // Make sure to propagate exceptions from rv onto aCx, so that they will get
+    // reported after we return.  We do this for all failures on rv, because now
+    // we're using rv to track all the state we care about.
     //
     // This is a little dumb, but aCx is in the null compartment here because we
     // set it up that way in our Run(), since we had not created the global at
     // that point yet.  So we need to enter the compartment of our global,
     // because setting a pending exception on aCx involves wrapping into its
     // current compartment.  Luckily we have a global now (else how would we
     // have a JS exception?) so we can just enter its compartment.
     JSAutoCompartment ac(aCx,
@@ -569,19 +569,19 @@ private:
     // Explicitly ignore NS_BINDING_ABORTED on rv.  Or more precisely, still
     // return false and don't SetWorkerScriptExecutedSuccessfully() in that
     // case, but don't throw anything on aCx.  The idea is to not dispatch error
     // events if our load is canceled with that error code.
     if (rv.ErrorCodeIs(NS_BINDING_ABORTED)) {
       rv.SuppressException();
       return false;
     }
-    // Make sure to propagate exceptions from rv onto aCx, so that our PostRun
-    // can report it.  We do this for all failures on rv, because now we're
-    // using rv to track all the state we care about.
+    // Make sure to propagate exceptions from rv onto aCx, so that they will get
+    // reported after we return.  We do this for all failures on rv, because now
+    // we're using rv to track all the state we care about.
     if (rv.MaybeSetPendingException(aCx)) {
       return false;
     }
 
     return true;
   }
 };
 
--- a/dom/workers/WorkerRunnable.cpp
+++ b/dom/workers/WorkerRunnable.cpp
@@ -179,20 +179,16 @@ WorkerRunnable::PostRun(JSContext* aCx, 
     default:
       MOZ_ASSERT_UNREACHABLE("Unknown behavior!");
   }
 #endif
 
   if (mBehavior == WorkerThreadModifyBusyCount) {
     aWorkerPrivate->ModifyBusyCountFromWorker(false);
   }
-
-  if (!aRunResult) {
-    JS_ReportPendingException(aCx);
-  }
 }
 
 // static
 WorkerRunnable*
 WorkerRunnable::FromRunnable(nsIRunnable* aRunnable)
 {
   MOZ_ASSERT(aRunnable);
 
@@ -344,16 +340,18 @@ WorkerRunnable::Run()
                  js::GetContextCompartment(cx),
                "Must either be in the null compartment or in our reflector "
                "compartment");
 
     ac.emplace(cx, mWorkerPrivate->GetWrapper());
   }
 
   bool result = WorkerRun(cx, mWorkerPrivate);
+  MOZ_ASSERT_IF(result, !JS_IsExceptionPending(cx));
+  JS_ReportPendingException(cx);
 
   // In the case of CompileScriptRunnnable, WorkerRun above can cause us to
   // lazily create a global, so we construct aes here before calling PostRun.
   if (targetIsWorkerThread && !aes && DefaultGlobalObject()) {
     aes.emplace(DefaultGlobalObject(), "worker runnable",
                 false, GetCurrentThreadJSContext());
     cx = aes->cx();
   }
--- a/dom/workers/WorkerRunnable.h
+++ b/dom/workers/WorkerRunnable.h
@@ -131,27 +131,29 @@ protected:
   // parent global (which is the compartment reflector would have been in), or
   // in the null compartment if there is no parent global.  For other mBehavior
   // values, we're running on the worker thread and aCx is in whatever
   // compartment GetCurrentThreadJSContext() was in when nsIRunnable::Run() got
   // called (XXXbz: Why is this a sane thing to be doing now that we have
   // multiple globals per worker???).  If it wasn't in a compartment, aCx will
   // be in either the debugger global's compartment or the worker's global's
   // compartment depending on whether IsDebuggerRunnable() is true.
+  //
+  // Immediately after WorkerRun returns, the caller will assert that either it
+  // returns false or there is no exception pending on aCx.  Then it will report
+  // any pending exceptions on aCx.
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) = 0;
 
   // By default asserts that Run() (and WorkerRun()) were called on the correct
   // thread.  Also sends an asynchronous message to the ParentThread if the
   // busy count was previously modified in PreDispatch().
   //
   // The aCx passed here is the same one as was passed to WorkerRun and is
-  // still in the same compartment.  If aRunResult is false, any failures on
-  // aCx are reported.  Otherwise failures are left to linger on the JSContext
-  // and maim later code (XXXbz: Aiming to fix that in bug 1072144).
+  // still in the same compartment.
   virtual void
   PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult);
 
   virtual bool
   DispatchInternal();
 
   // Calling Run() directly is not supported. Just call Dispatch() and
   // WorkerRun() will be called on the correct thread automatically.