Bug 1249652 part 2. ScriptExecutorRunnable::WorkerRun should immediately move JS exceptions to its ErrorResult instead of allowing them to linger on the JSContext. r=baku,khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 24 Feb 2016 10:38:31 -0500
changeset 285335 09a878e63f05af0afd116f35d96d487bce554f51
parent 285334 cfa41433ed01a8dba9572f1d86ab6213fd349324
child 285336 af4680dcbeb15fa3787454e17f7af24c7feaecda
push id72326
push userbzbarsky@mozilla.com
push dateWed, 24 Feb 2016 15:38:49 +0000
treeherdermozilla-inbound@ee07c6a974bc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, khuey
bugs1249652
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 1249652 part 2. ScriptExecutorRunnable::WorkerRun should immediately move JS exceptions to its ErrorResult instead of allowing them to linger on the JSContext. r=baku,khuey
dom/workers/ScriptLoader.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerRunnable.h
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -1714,58 +1714,71 @@ ScriptExecutorRunnable::IsDebuggerRunnab
   // In the latter case, the runnable needs to be dispatched to the debugger
   // queue.
   return mScriptLoader.mWorkerScriptType == DebuggerScript;
 }
 
 bool
 ScriptExecutorRunnable::WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
 {
+  aWorkerPrivate->AssertIsOnWorkerThread();
+
   nsTArray<ScriptLoadInfo>& loadInfos = mScriptLoader.mLoadInfos;
 
   // Don't run if something else has already failed.
   for (uint32_t index = 0; index < mFirstIndex; index++) {
     ScriptLoadInfo& loadInfo = loadInfos.ElementAt(index);
 
     NS_ASSERTION(!loadInfo.mChannel, "Should no longer have a channel!");
     NS_ASSERTION(loadInfo.mExecutionScheduled, "Should be scheduled!");
 
     if (!loadInfo.mExecutionResult) {
       return true;
     }
   }
 
+  // If nothing else has failed, our ErrorResult better not be a failure either.
+  MOZ_ASSERT(!mScriptLoader.mRv.Failed(), "Who failed it and why?");
+
   JS::Rooted<JSObject*> global(aCx);
 
   if (mIsWorkerScript) {
     WorkerGlobalScope* globalScope =
       aWorkerPrivate->GetOrCreateGlobalScope(aCx);
     if (NS_WARN_IF(!globalScope)) {
       NS_WARNING("Failed to make global!");
       return false;
     }
 
     global.set(globalScope->GetWrapper());
   } else {
+    // XXXbz Icky action at a distance...  Would be better to capture this state
+    // in mScriptLoader!
     global.set(JS::CurrentGlobalOrNull(aCx));
   }
 
   MOZ_ASSERT(global);
 
   JSAutoCompartment ac(aCx, global);
 
   for (uint32_t index = mFirstIndex; index <= mLastIndex; index++) {
     ScriptLoadInfo& loadInfo = loadInfos.ElementAt(index);
 
     NS_ASSERTION(!loadInfo.mChannel, "Should no longer have a channel!");
     NS_ASSERTION(loadInfo.mExecutionScheduled, "Should be scheduled!");
     NS_ASSERTION(!loadInfo.mExecutionResult, "Should not have executed yet!");
 
+    MOZ_ASSERT(!mScriptLoader.mRv.Failed(), "Who failed it and why?");
+    mScriptLoader.mRv.MightThrowJSException();
     if (NS_FAILED(loadInfo.mLoadResult)) {
       scriptloader::ReportLoadError(aCx, loadInfo.mLoadResult);
+      // Don't try to steal if ReportLoadError didn't actually throw.
+      if (JS_IsExceptionPending(aCx)) {
+        mScriptLoader.mRv.StealExceptionFromJSContext(aCx);
+      }
       // Top level scripts only!
       if (mIsWorkerScript) {
         aWorkerPrivate->MaybeDispatchLoadFailedRunnable();
       }
       return true;
     }
 
     NS_ConvertUTF16toUTF8 filename(loadInfo.mURL);
@@ -1782,31 +1795,37 @@ ScriptExecutorRunnable::WorkerRun(JSCont
     options.setMutedErrors(loadInfo.mMutedErrorFlag.valueOr(true));
 
     JS::SourceBufferHolder srcBuf(loadInfo.mScriptTextBuf,
                                   loadInfo.mScriptTextLength,
                                   JS::SourceBufferHolder::GiveOwnership);
     loadInfo.mScriptTextBuf = nullptr;
     loadInfo.mScriptTextLength = 0;
 
+    // Our ErrorResult still shouldn't be a failure.
+    MOZ_ASSERT(!mScriptLoader.mRv.Failed(), "Who failed it and why?");
     JS::Rooted<JS::Value> unused(aCx);
     if (!JS::Evaluate(aCx, options, srcBuf, &unused)) {
+      mScriptLoader.mRv.StealExceptionFromJSContext(aCx);
       return true;
     }
 
     loadInfo.mExecutionResult = true;
   }
 
   return true;
 }
 
 void
 ScriptExecutorRunnable::PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                                 bool aRunResult)
 {
+  aWorkerPrivate->AssertIsOnWorkerThread();
+  MOZ_ASSERT(!JS_IsExceptionPending(aCx), "Who left an exception on there?");
+
   nsTArray<ScriptLoadInfo>& loadInfos = mScriptLoader.mLoadInfos;
 
   if (mLastIndex == loadInfos.Length() - 1) {
     // All done. If anything failed then return false.
     bool result = true;
     nsresult loadResult = NS_OK;
     bool mutedError = false;
     for (uint32_t index = 0; index < loadInfos.Length(); index++) {
@@ -1820,16 +1839,23 @@ ScriptExecutorRunnable::PostRun(JSContex
         // error result of the 'real' failing one.
         if (loadInfos[index].mLoadResult != NS_BINDING_ABORTED) {
           loadResult = loadInfos[index].mLoadResult;
           break;
         }
       }
     }
 
+    // The only way we can get here with "result" false but without either
+    // mScriptLoader.mRv or loadResult being failures is if we're loading the
+    // main worker script and GetOrCreateGlobalScope() fails.  In that case we
+    // would have returned false from WorkerRun, so assert that.
+    MOZ_ASSERT_IF(!result && !mScriptLoader.mRv.Failed() &&
+                  NS_SUCCEEDED(loadResult),
+                  !aRunResult);
     ShutdownScriptLoader(aCx, aWorkerPrivate, result, loadResult, mutedError);
   }
 }
 
 NS_IMETHODIMP
 ScriptExecutorRunnable::Cancel()
 {
   if (mLastIndex == mScriptLoader.mLoadInfos.Length() - 1) {
@@ -1841,28 +1867,49 @@ ScriptExecutorRunnable::Cancel()
 
 void
 ScriptExecutorRunnable::ShutdownScriptLoader(JSContext* aCx,
                                              WorkerPrivate* aWorkerPrivate,
                                              bool aResult,
                                              nsresult aLoadResult,
                                              bool aMutedError)
 {
+  aWorkerPrivate->AssertIsOnWorkerThread();
+
   MOZ_ASSERT(mLastIndex == mScriptLoader.mLoadInfos.Length() - 1);
 
   if (mIsWorkerScript && aWorkerPrivate->IsServiceWorker()) {
     aWorkerPrivate->SetLoadingWorkerScript(false);
   }
 
   if (!aResult) {
-    // If this error has to be muted, we have to clear the pending exception,
-    // if any, and use the ErrorResult object to throw a new exception.
-    if (aMutedError && JS_IsExceptionPending(aCx)) {
-      LogExceptionToConsole(aCx, aWorkerPrivate);
-      mScriptLoader.mRv.Throw(NS_ERROR_FAILURE);
+    // At this point there are three possibilities:
+    //
+    // 1) mScriptLoader.mRv.Failed().  In that case we just want to leave it
+    //    as-is, except if it has a JS exception and we need to mute JS
+    //    exceptions.  In that case, we log the exception without firing any
+    //    events and then replace it on the ErrorResult with a generic
+    //    NS_ERROR_FAILURE for lack of anything better.  XXXbz: This should
+    //    throw a NetworkError per spec updates.  See bug 1249673.
+    //
+    // 2) mScriptLoader.mRv succeeded, but aLoadResult is a failure status.
+    //    This indicates some failure somewhere along the way (e.g. network
+    //    failure).  Just throw that failure status.  XXXbz: Shouldn't this also
+    //    generally become NetworkError?
+    //
+    // 3) mScriptLoader.mRv succeeded and aLoadResult is also success.  As far
+    //    as I can tell, this can only happen when loading the main worker
+    //    script and GetOrCreateGlobalScope() fails or if
+    //    ScriptExecutorRunnable::Cancel got called.  Does it matter what we
+    //    throw in this case?  I'm not sure...
+    if (mScriptLoader.mRv.Failed()) {
+      if (aMutedError && mScriptLoader.mRv.IsJSException()) {
+        LogExceptionToConsole(aCx, aWorkerPrivate);
+        mScriptLoader.mRv.Throw(NS_ERROR_FAILURE);
+      }
     } else if (NS_FAILED(aLoadResult)) {
       mScriptLoader.mRv.Throw(aLoadResult);
     } else {
       mScriptLoader.mRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     }
   }
 
   aWorkerPrivate->RemoveFeature(aCx, &mScriptLoader);
@@ -1870,22 +1917,26 @@ ScriptExecutorRunnable::ShutdownScriptLo
 }
 
 void
 ScriptExecutorRunnable::LogExceptionToConsole(JSContext* aCx,
                                               WorkerPrivate* aWorkerPrivate)
 {
   aWorkerPrivate->AssertIsOnWorkerThread();
 
+  MOZ_ASSERT(mScriptLoader.mRv.IsJSException());
+
   JS::Rooted<JS::Value> exn(aCx);
-  if (!JS_GetPendingException(aCx, &exn)) {
+  if (!ToJSValue(aCx, mScriptLoader.mRv, &exn)) {
     return;
   }
 
-  JS_ClearPendingException(aCx);
+  // Now the exception state should all be in exn.
+  MOZ_ASSERT(!JS_IsExceptionPending(aCx));
+  MOZ_ASSERT(!mScriptLoader.mRv.Failed());
 
   js::ErrorReport report(aCx);
   if (!report.init(aCx, exn)) {
     JS_ClearPendingException(aCx);
     return;
   }
 
   RefPtr<xpc::ErrorReport> xpcReport = new xpc::ErrorReport();
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -497,22 +497,39 @@ public:
   : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount),
     mScriptURL(aScriptURL)
   { }
 
 private:
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
+    aWorkerPrivate->AssertIsOnWorkerThread();
+
     ErrorResult rv;
     scriptloader::LoadMainScript(aCx, mScriptURL, WorkerScript, rv);
+    // If rv has a JS exception, put it right back on aCx, so that our PostRun
+    // can report it.  Otherwise, I guess suppress the exception since that's
+    // what we used to do, though this seems moderately weird.
+    rv.WouldReportJSException();
     if (NS_WARN_IF(rv.Failed())) {
-      // I guess suppress the exception since that's what we used to
-      // do, though this seems moderately weird.
-      rv.SuppressException();
+      if (rv.IsJSException()) {
+        // 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,
+          aWorkerPrivate->GlobalScope()->GetGlobalJSObject());
+        rv.MaybeSetPendingException(aCx);
+      } else {
+        rv.SuppressException();
+      }
       return false;
     }
 
     aWorkerPrivate->SetWorkerScriptExecutedSuccessfully();
     return true;
   }
 };
 
@@ -526,32 +543,40 @@ public:
   : WorkerDebuggerRunnable(aWorkerPrivate),
     mScriptURL(aScriptURL)
   { }
 
 private:
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
+    aWorkerPrivate->AssertIsOnWorkerThread();
+
     WorkerDebuggerGlobalScope* globalScope =
       aWorkerPrivate->CreateDebuggerGlobalScope(aCx);
     if (!globalScope) {
       NS_WARNING("Failed to make global!");
       return false;
     }
 
     JS::Rooted<JSObject*> global(aCx, globalScope->GetWrapper());
 
     ErrorResult rv;
     JSAutoCompartment ac(aCx, global);
     scriptloader::LoadMainScript(aCx, mScriptURL, DebuggerScript, rv);
+    // If rv has a JS exception, put it right back on aCx, so that our PostRun
+    // can report it.  Otherwise, I guess suppress the exception since that's
+    // what we used to do, though this seems moderately weird.
+    rv.WouldReportJSException();
     if (NS_WARN_IF(rv.Failed())) {
-      // I guess suppress the exception since that's what we used to
-      // do, though this seems moderately weird.
-      rv.SuppressException();
+      if (rv.IsJSException()) {
+        rv.MaybeSetPendingException(aCx);
+      } else {
+        rv.SuppressException();
+      }
       return false;
     }
 
     return true;
   }
 };
 
 class CloseEventRunnable final : public WorkerRunnable
--- a/dom/workers/WorkerRunnable.h
+++ b/dom/workers/WorkerRunnable.h
@@ -222,17 +222,18 @@ protected:
 
   virtual ~WorkerSyncRunnable();
 
   virtual bool
   DispatchInternal() override;
 };
 
 // This runnable is identical to WorkerSyncRunnable except it is meant to be
-// used on the main thread only.
+// created on and dispatched from the main thread only.  Its WorkerRun/PostRun
+// will run on the worker thread.
 class MainThreadWorkerSyncRunnable : public WorkerSyncRunnable
 {
 protected:
   // Passing null for aSyncLoopTarget is allowed and will result in the behavior
   // of a normal WorkerRunnable.
   MainThreadWorkerSyncRunnable(WorkerPrivate* aWorkerPrivate,
                                nsIEventTarget* aSyncLoopTarget)
   : WorkerSyncRunnable(aWorkerPrivate, aSyncLoopTarget)