Bug 1249652 part 3. Simplify way we handle canceling when ScriptLoaderRunnable::RunInternal fails by canceling things with its actual failure code, so we don't have to guess which failed loads are actual failures and which are just canceled via this mechanism. r=baku,khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 24 Feb 2016 10:38:31 -0500
changeset 285336 af4680dcbeb15fa3787454e17f7af24c7feaecda
parent 285335 09a878e63f05af0afd116f35d96d487bce554f51
child 285337 ee07c6a974bccea9ccf6b3bdcf4874b79308b188
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 3. Simplify way we handle canceling when ScriptLoaderRunnable::RunInternal fails by canceling things with its actual failure code, so we don't have to guess which failed loads are actual failures and which are just canceled via this mechanism. r=baku,khuey There is a bit of subtlety here with NS_BINDING_ABORTED. Before these changes, we would land in ReportLoadError, not do anything with NS_BINDING_ABORTED, and just return. If called from WorkerPrivate::Constructor we'd then go ahead and throw it on the ErrorResult, but I'm pretty sure we never ended up with NS_BINDING_ABORTED there. If called from ScriptExecutorRunnable::WorkerRun, we would proceed on to ScriptExecutorRunnable::PostRun and hence ShutdownScriptLoader where we would throw on the ErrorResult but NOT on the JSContext. Then we would unwind to our consumer and if that consumer was a toplevel script load we would suppress the exception on the ErrorResult. Otherwise we'd go ahead and throw the exception we ended up with to the caller. The upshot is that we used to not fire error events on a worker whose main script load was canceled with NS_BINDING_ABORTED. So we try to preserve that behavior explicitly for toplevel scripts.
dom/workers/ScriptLoader.cpp
dom/workers/ScriptLoader.h
dom/workers/WorkerPrivate.cpp
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -314,17 +314,16 @@ private:
           override;
 
   NS_DECL_NSICANCELABLERUNNABLE
 
   void
   ShutdownScriptLoader(JSContext* aCx,
                        WorkerPrivate* aWorkerPrivate,
                        bool aResult,
-                       nsresult aLoadResult,
                        bool aMutedError);
 
   void LogExceptionToConsole(JSContext* aCx,
                              WorkerPrivate* WorkerPrivate);
 };
 
 class CacheScriptLoader;
 
@@ -568,18 +567,19 @@ private:
   ~ScriptLoaderRunnable()
   { }
 
   NS_IMETHOD
   Run() override
   {
     AssertIsOnMainThread();
 
-    if (NS_FAILED(RunInternal())) {
-      CancelMainThread();
+    nsresult rv = RunInternal();
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      CancelMainThread(rv);
     }
 
     return NS_OK;
   }
 
   void
   LoadingFinished(uint32_t aIndex, nsresult aRv)
   {
@@ -706,17 +706,18 @@ private:
   Notify(JSContext* aCx, Status aStatus) override
   {
     mWorkerPrivate->AssertIsOnWorkerThread();
 
     if (aStatus >= Terminating && !mCanceled) {
       mCanceled = true;
 
       nsCOMPtr<nsIRunnable> runnable =
-        NS_NewRunnableMethod(this, &ScriptLoaderRunnable::CancelMainThread);
+        NS_NewRunnableMethod(this,
+          &ScriptLoaderRunnable::CancelMainThreadWithBindingAborted);
       NS_ASSERTION(runnable, "This should never fail!");
 
       if (NS_FAILED(NS_DispatchToMainThread(runnable))) {
         JS_ReportError(aCx, "Failed to cancel script loader!");
         return false;
       }
     }
 
@@ -725,17 +726,23 @@ private:
 
   bool
   IsMainWorkerScript() const
   {
     return mIsMainScript && mWorkerScriptType == WorkerScript;
   }
 
   void
-  CancelMainThread()
+  CancelMainThreadWithBindingAborted()
+  {
+    CancelMainThread(NS_BINDING_ABORTED);
+  }
+
+  void
+  CancelMainThread(nsresult aCancelResult)
   {
     AssertIsOnMainThread();
 
     if (mCanceledMainThread) {
       return;
     }
 
     mCanceledMainThread = true;
@@ -750,31 +757,31 @@ private:
       ScriptLoadInfo& loadInfo = mLoadInfos[index];
 
       // If promise or channel is non-null, their failures will lead to
       // LoadingFinished being called.
       bool callLoadingFinished = true;
 
       if (loadInfo.mCachePromise) {
         MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
-        loadInfo.mCachePromise->MaybeReject(NS_BINDING_ABORTED);
+        loadInfo.mCachePromise->MaybeReject(aCancelResult);
         loadInfo.mCachePromise = nullptr;
         callLoadingFinished = false;
       }
 
       if (loadInfo.mChannel) {
-        if (NS_SUCCEEDED(loadInfo.mChannel->Cancel(NS_BINDING_ABORTED))) {
+        if (NS_SUCCEEDED(loadInfo.mChannel->Cancel(aCancelResult))) {
           callLoadingFinished = false;
         } else {
           NS_WARNING("Failed to cancel channel!");
         }
       }
 
       if (callLoadingFinished && !loadInfo.Finished()) {
-        LoadingFinished(index, NS_BINDING_ABORTED);
+        LoadingFinished(index, aCancelResult);
       }
     }
 
     ExecuteFinishedScripts();
   }
 
   void
   DeleteCache()
@@ -1764,21 +1771,18 @@ ScriptExecutorRunnable::WorkerRun(JSCont
 
     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);
-      }
+      scriptloader::ReportLoadError(aCx, mScriptLoader.mRv,
+                                    loadInfo.mLoadResult);
       // Top level scripts only!
       if (mIsWorkerScript) {
         aWorkerPrivate->MaybeDispatchLoadFailedRunnable();
       }
       return true;
     }
 
     NS_ConvertUTF16toUTF8 filename(loadInfo.mURL);
@@ -1821,97 +1825,79 @@ ScriptExecutorRunnable::PostRun(JSContex
   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++) {
       if (!loadInfos[index].mExecutionResult) {
-        mutedError = mutedError || loadInfos[index].mMutedErrorFlag.valueOr(true);
-        loadResult = loadInfos[index].mLoadResult;
+        mutedError = loadInfos[index].mMutedErrorFlag.valueOr(true);
         result = false;
-
-        // If we have more than one loadInfos and one of them fails, the others
-        // are marked as NS_BINDING_ABORTED, but what we want to report is the
-        // error result of the 'real' failing one.
-        if (loadInfos[index].mLoadResult != NS_BINDING_ABORTED) {
-          loadResult = loadInfos[index].mLoadResult;
-          break;
-        }
+        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),
+    // The only way we can get here with "result" false but without
+    // mScriptLoader.mRv being a failure 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(),
                   !aRunResult);
-    ShutdownScriptLoader(aCx, aWorkerPrivate, result, loadResult, mutedError);
+    ShutdownScriptLoader(aCx, aWorkerPrivate, result, mutedError);
   }
 }
 
 NS_IMETHODIMP
 ScriptExecutorRunnable::Cancel()
 {
   if (mLastIndex == mScriptLoader.mLoadInfos.Length() - 1) {
     ShutdownScriptLoader(mWorkerPrivate->GetJSContext(), mWorkerPrivate,
-                         false, NS_OK, false);
+                         false, false);
   }
   return MainThreadWorkerSyncRunnable::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) {
-    // At this point there are three possibilities:
+    // At this point there are two 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...
+    // 2) mScriptLoader.mRv succeeded.  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);
   aWorkerPrivate->StopSyncLoop(mSyncLoopTarget, aResult);
 }
@@ -2029,38 +2015,46 @@ ChannelFromScriptURLWorkerThread(JSConte
 
   if (!syncLoop.Run()) {
     return NS_ERROR_FAILURE;
   }
 
   return getter->GetResult();
 }
 
-void ReportLoadError(JSContext* aCx, nsresult aLoadResult)
+void ReportLoadError(JSContext* aCx, ErrorResult& aRv, nsresult aLoadResult)
 {
+  MOZ_ASSERT(!aRv.Failed());
+
   switch (aLoadResult) {
-    case NS_BINDING_ABORTED:
-      // Canceled, don't set an exception.
-      break;
-
     case NS_ERROR_FILE_NOT_FOUND:
     case NS_ERROR_NOT_AVAILABLE:
-      Throw(aCx, NS_ERROR_DOM_NETWORK_ERR);
+      aRv.Throw(NS_ERROR_DOM_NETWORK_ERR);
       break;
 
     case NS_ERROR_MALFORMED_URI:
       aLoadResult = NS_ERROR_DOM_SYNTAX_ERR;
       MOZ_FALLTHROUGH;
+    case NS_BINDING_ABORTED:
+      // Note: we used to pretend like we didn't set an exception for
+      // NS_BINDING_ABORTED, but then ShutdownScriptLoader did it anyway.  The
+      // other callsite, in WorkerPrivate::Constructor, never passed in
+      // NS_BINDING_ABORTED.  So just throw it directly here.  Consumers will
+      // deal as needed.
+      MOZ_FALLTHROUGH;
     case NS_ERROR_DOM_SECURITY_ERR:
     case NS_ERROR_DOM_SYNTAX_ERR:
-      Throw(aCx, aLoadResult);
+      aRv.Throw(aLoadResult);
       break;
 
     default:
+      // We _could_ probably just throw aLoadResult on aRv, but let's preserve
+      // the old behavior for now and throw this string as an Error instead.
       JS_ReportError(aCx, "Failed to load script (nsresult = 0x%x)", aLoadResult);
+      aRv.StealExceptionFromJSContext(aCx);
   }
 }
 
 void
 LoadMainScript(JSContext* aCx, const nsAString& aScriptURL,
                WorkerScriptType aWorkerScriptType,
                ErrorResult& aRv)
 {
--- a/dom/workers/ScriptLoader.h
+++ b/dom/workers/ScriptLoader.h
@@ -42,17 +42,17 @@ ChannelFromScriptURLMainThread(nsIPrinci
                                nsIChannel** aChannel);
 
 nsresult
 ChannelFromScriptURLWorkerThread(JSContext* aCx,
                                  WorkerPrivate* aParent,
                                  const nsAString& aScriptURL,
                                  nsIChannel** aChannel);
 
-void ReportLoadError(JSContext* aCx, nsresult aLoadResult);
+void ReportLoadError(JSContext* aCx, ErrorResult& aRv, nsresult aLoadResult);
 
 void LoadMainScript(JSContext* aCx, const nsAString& aScriptURL,
                     WorkerScriptType aWorkerScriptType,
                     ErrorResult& aRv);
 
 void Load(JSContext* aCx,
           WorkerPrivate* aWorkerPrivate,
           const nsTArray<nsString>& aScriptURLs,
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -501,35 +501,38 @@ public:
 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())) {
-      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();
-      }
+    // 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.
+    //
+    // 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());
+    if (rv.MaybeSetPendingException(aCx)) {
       return false;
     }
 
     aWorkerPrivate->SetWorkerScriptExecutedSuccessfully();
     return true;
   }
 };
 
@@ -557,26 +560,29 @@ private:
       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())) {
-      if (rv.IsJSException()) {
-        rv.MaybeSetPendingException(aCx);
-      } else {
-        rv.SuppressException();
-      }
+    // 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 alll 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;
   }
 };
 
 class CloseEventRunnable final : public WorkerRunnable
@@ -4062,20 +4068,19 @@ WorkerPrivate::Constructor(JSContext* aC
 
   Maybe<WorkerLoadInfo> stackLoadInfo;
   if (!aLoadInfo) {
     stackLoadInfo.emplace();
 
     nsresult rv = GetLoadInfo(aCx, nullptr, parent, aScriptURL,
                               aIsChromeWorker, InheritLoadGroup,
                               aWorkerType, stackLoadInfo.ptr());
+    aRv.MightThrowJSException();
     if (NS_FAILED(rv)) {
-      // XXXkhuey this is weird, why throw again after setting an exception?
-      scriptloader::ReportLoadError(aCx, rv);
-      aRv.Throw(rv);
+      scriptloader::ReportLoadError(aCx, aRv, rv);
       return nullptr;
     }
 
     aLoadInfo = stackLoadInfo.ptr();
   }
 
   // NB: This has to be done before creating the WorkerPrivate, because it will
   // attempt to use static variables that are initialized in the RuntimeService