Bug 1251369. Use an AutoJSAPI that reports its own exceptions around the main runloop in workers. r=khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 26 Feb 2016 15:23:13 -0500
changeset 322210 fce4115d2040062f75a6c7bb98a3ea0f6700d729
parent 322209 d6c6898a18f62838992ddb8d6e8e633ed6240941
child 322211 53bb059dd388065181e77f2a935dc680fad41b38
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
bugs1251369, 1251518
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 1251369. Use an AutoJSAPI that reports its own exceptions around the main runloop in workers. r=khuey The silly leading ": " on the error messages is due to bug 1251518.
dom/security/test/csp/file_child-src_worker-redirect.html
dom/workers/RuntimeService.cpp
dom/workers/ScriptLoader.cpp
dom/workers/ScriptLoader.h
dom/workers/WorkerPrivate.cpp
dom/workers/test/test_404.html
dom/workers/test/test_bug1036484.html
dom/workers/test/test_loadError.html
--- a/dom/security/test/csp/file_child-src_worker-redirect.html
+++ b/dom/security/test/csp/file_child-src_worker-redirect.html
@@ -19,17 +19,17 @@
       worker = new Worker('file_redirect_worker.sjs?path='
           + escape("/tests/dom/security/test/csp/file_child-src_worker.js")
           + "&redir=" + redir
           + "&page_id=" + page_id
           );
 
       worker.onerror = function(error) {
         var msg = error.message;
-        if (msg.match(/^: NetworkError/) || msg.match(/Failed to load script/)) {
+        if (msg.match(/^: NetworkError/) || msg.match(/Failed to load worker script/)) {
           // this means CSP blocked it
           msg = "blocked";
         }
         window.parent.postMessage({id:page_id, message:msg}, 'http://mochi.test:8888');
         error.preventDefault();
       };
 
       worker.onmessage = function(ev) {
--- a/dom/workers/RuntimeService.cpp
+++ b/dom/workers/RuntimeService.cpp
@@ -2631,18 +2631,20 @@ WorkerThreadPrimaryRunnable::Run()
         stack->sampleRuntime(rt);
       }
 #endif
 
       {
         JSAutoRequest ar(cx);
 
         mWorkerPrivate->DoRunLoop(cx);
-
-        JS_ReportPendingException(cx);
+        // The AutoJSAPI in DoRunLoop should have reported any exceptions left
+        // on cx.  Note that we still need the JSAutoRequest above because
+        // AutoJSAPI on workers does NOT enter a request!
+        MOZ_ASSERT(!JS_IsExceptionPending(cx));
       }
 
       BackgroundChild::CloseForCurrentThread();
 
 #ifdef MOZ_ENABLE_PROFILER_SPS
       if (stack) {
         stack->sampleRuntime(nullptr);
       }
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -1772,17 +1772,17 @@ 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, mScriptLoader.mRv,
-                                    loadInfo.mLoadResult);
+                                    loadInfo.mLoadResult, loadInfo.mURL);
       // Top level scripts only!
       if (mIsWorkerScript) {
         aWorkerPrivate->MaybeDispatchLoadFailedRunnable();
       }
       return true;
     }
 
     NS_ConvertUTF16toUTF8 filename(loadInfo.mURL);
@@ -2014,47 +2014,66 @@ ChannelFromScriptURLWorkerThread(JSConte
 
   if (!syncLoop.Run()) {
     return NS_ERROR_FAILURE;
   }
 
   return getter->GetResult();
 }
 
-void ReportLoadError(JSContext* aCx, ErrorResult& aRv, nsresult aLoadResult)
+void ReportLoadError(JSContext* aCx, ErrorResult& aRv, nsresult aLoadResult,
+                     const nsAString& aScriptURL)
 {
   MOZ_ASSERT(!aRv.Failed());
 
   switch (aLoadResult) {
     case NS_ERROR_FILE_NOT_FOUND:
     case NS_ERROR_NOT_AVAILABLE:
-      aRv.Throw(NS_ERROR_DOM_NETWORK_ERR);
+      aLoadResult = NS_ERROR_DOM_NETWORK_ERR;
       break;
 
     case NS_ERROR_MALFORMED_URI:
       aLoadResult = NS_ERROR_DOM_SYNTAX_ERR;
-      MOZ_FALLTHROUGH;
+      break;
+
     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;
+      // deal as needed.  But note that we do NOT want to ThrowDOMException()
+      // for this case, because that will make it impossible for consumers to
+      // realize that our error was NS_BINDING_ABORTED.
+      aRv.Throw(aLoadResult);
+      return;
+
     case NS_ERROR_DOM_SECURITY_ERR:
     case NS_ERROR_DOM_SYNTAX_ERR:
-      aRv.Throw(aLoadResult);
+      break;
+
+    case NS_ERROR_DOM_BAD_URI:
+      // This is actually a security error.
+      aLoadResult = NS_ERROR_DOM_SECURITY_ERR;
       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);
+      // For lack of anything better, go ahead and throw a NetworkError here.
+      // We don't want to throw a JS exception, because for toplevel script
+      // loads that would get squelched.
+      aRv.ThrowDOMException(NS_ERROR_DOM_NETWORK_ERR,
+        nsPrintfCString("Failed to load worker script at %s (nsresult = 0x%x)",
+                        NS_ConvertUTF16toUTF8(aScriptURL).get(),
+                        aLoadResult));
+      return;
   }
+
+  aRv.ThrowDOMException(aLoadResult,
+                        NS_LITERAL_CSTRING("Failed to load worker script at \"") +
+                        NS_ConvertUTF16toUTF8(aScriptURL) +
+                        NS_LITERAL_CSTRING("\""));
 }
 
 void
 LoadMainScript(JSContext* aCx, const nsAString& aScriptURL,
                WorkerScriptType aWorkerScriptType,
                ErrorResult& aRv)
 {
   WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx);
--- a/dom/workers/ScriptLoader.h
+++ b/dom/workers/ScriptLoader.h
@@ -42,17 +42,18 @@ ChannelFromScriptURLMainThread(nsIPrinci
                                nsIChannel** aChannel);
 
 nsresult
 ChannelFromScriptURLWorkerThread(JSContext* aCx,
                                  WorkerPrivate* aParent,
                                  const nsAString& aScriptURL,
                                  nsIChannel** aChannel);
 
-void ReportLoadError(JSContext* aCx, ErrorResult& aRv, nsresult aLoadResult);
+void ReportLoadError(JSContext* aCx, ErrorResult& aRv, nsresult aLoadResult,
+                     const nsAString& aScriptURL);
 
 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
@@ -4027,17 +4027,17 @@ WorkerPrivate::Constructor(JSContext* aC
   if (!aLoadInfo) {
     stackLoadInfo.emplace();
 
     nsresult rv = GetLoadInfo(aCx, nullptr, parent, aScriptURL,
                               aIsChromeWorker, InheritLoadGroup,
                               aWorkerType, stackLoadInfo.ptr());
     aRv.MightThrowJSException();
     if (NS_FAILED(rv)) {
-      scriptloader::ReportLoadError(aCx, aRv, rv);
+      scriptloader::ReportLoadError(aCx, aRv, rv, aScriptURL);
       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
@@ -4373,16 +4373,24 @@ WorkerPrivate::DoRunLoop(JSContext* aCx)
   {
     MutexAutoLock lock(mMutex);
     mJSContext = aCx;
 
     MOZ_ASSERT(mStatus == Pending);
     mStatus = Running;
   }
 
+  // Now that we've done that, we can go ahead and set up our AutoJSAPI.  We
+  // can't before this point, because it can't find the right JSContext before
+  // then, since it gets it from our mJSContext.
+  AutoJSAPI jsapi;
+  jsapi.Init();
+  MOZ_ASSERT(jsapi.cx() == aCx);
+  jsapi.TakeOwnershipOfErrorReporting();
+
   EnableMemoryReporter();
 
   InitializeGCTimers();
 
   Maybe<JSAutoCompartment> workerCompartment;
 
   for (;;) {
     Status currentStatus;
--- a/dom/workers/test/test_404.html
+++ b/dom/workers/test/test_404.html
@@ -20,17 +20,17 @@ Tests of DOM Worker Threads
 
   worker.onmessage = function(event) {
     ok(false, "Shouldn't ever get a message!");
     SimpleTest.finish();
   }
 
   worker.onerror = function(event) {
     is(event.target, worker);
-    is(event.message, ": NetworkError: A network error occurred.");
+    is(event.message, ': NetworkError: Failed to load worker script at "nonexistent_worker.js"');
     event.preventDefault();
     SimpleTest.finish();
   };
 
   worker.postMessage("dummy");
 
   SimpleTest.waitForExplicitFinish();
 
--- a/dom/workers/test/test_bug1036484.html
+++ b/dom/workers/test/test_bug1036484.html
@@ -20,17 +20,17 @@ function test(script) {
   var worker = new Worker(script);
 
   worker.onmessage = function(event) {
     ok(false, "Shouldn't ever get a message!");
   }
 
   worker.onerror = function(event) {
     is(event.target, worker);
-    is(event.message, ": NetworkError: A network error occurred.");
+    ok(event.message.startsWith(": NetworkError: Failed to load worker script"))
     event.preventDefault();
     runTests();
   };
 
   worker.postMessage("dummy");
 }
 
 var tests = [ '404_server.sjs', '404_server.sjs?js' ];
--- a/dom/workers/test/test_loadError.html
+++ b/dom/workers/test/test_loadError.html
@@ -8,16 +8,18 @@
   <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <pre id="test">
 <script class="testbody" type="text/javascript">
 "use strict";
 
+var loadErrorMessage = ': SecurityError: Failed to load worker script at "about:blank"';
+
 function nextTest() {
   (function(){
     function workerfunc() {
       var subworker = new Worker("about:blank");
       subworker.onerror = function(e) {
         e.preventDefault();
         postMessage(e.message);
       }
@@ -30,30 +32,35 @@ function nextTest() {
         URL.revokeObjectURL(u);
         is(i, 0, 'worker creation succeeded');
       } catch (e) {
         is(i, 1, 'worker creation failed');
         SimpleTest.finish();
         return;
       }
       w.onmessage = function(e) {
-        is(e.data.indexOf('Error: Failed to load script'), 0, "Error: Failed to load script");
+        is(e.data, loadErrorMessage,
+           "Should catch the error when loading inner script");
         if (++i < 2) callworker(i);
         else SimpleTest.finish();
       };
+      w.onerrror = function(e) {
+        ok(false, "Should not get any errors from this worker");
+      }
     }
     callworker(0);
   })();
 }
 
 try {
   var worker = new Worker("about:blank");
   worker.onerror = function(e) {
     e.preventDefault();
-    ok(true, "Shouldn't success!");
+    is(e.message, loadErrorMessage,
+       "Should get the right error from the toplevel script");
     nextTest();
   }
 
   worker.onmessage = function(event) {
     ok(false, "Shouldn't get a message!");
     SimpleTest.finish();
   }
 } catch (e) {