Bug 1173139 - Reorder global creation on workers, r=khuey.
authorBen Turner <bent.mozilla@gmail.com>
Wed, 10 Jun 2015 14:12:55 -0700
changeset 248171 49480f1c4071c59c8ae1afced4d38805f1a60479
parent 248170 56ede9a5462d857baf323c246113ba738f26bee4
child 248172 6289cdbce7b7b3950706d5e5bef4daf29c604f55
push id28893
push userkwierso@gmail.com
push dateFri, 12 Jun 2015 00:02:58 +0000
treeherdermozilla-central@8cf9d3e497f9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs1173139
milestone41.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 1173139 - Reorder global creation on workers, r=khuey.
dom/workers/ScriptLoader.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerScope.cpp
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -1580,37 +1580,34 @@ ScriptExecutorRunnable::WorkerRun(JSCont
     NS_ASSERTION(!loadInfo.mChannel, "Should no longer have a channel!");
     NS_ASSERTION(loadInfo.mExecutionScheduled, "Should be scheduled!");
 
     if (!loadInfo.mExecutionResult) {
       return true;
     }
   }
 
-  JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
-  NS_ASSERTION(global, "Must have a global by now!");
+  JS::Rooted<JSObject*> global(aCx);
 
-  // Determine whether we want to be discarding source on this global to save
-  // memory. It would make more sense to do this when we create the global, but
-  // the information behind UsesSystemPrincipal() et al isn't finalized until
-  // the call to SetPrincipal during the first script load. After that, however,
-  // it never changes. So we can just idempotently set the bits here.
-  //
-  // Note that we read a pref that is cached on the main thread. This is benignly
-  // racey.
-  if (xpc::ShouldDiscardSystemSource()) {
-    bool discard = aWorkerPrivate->UsesSystemPrincipal() ||
-                   aWorkerPrivate->IsInPrivilegedApp();
-    JS::CompartmentOptionsRef(global).setDiscardSource(discard);
+  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 {
+    global.set(JS::CurrentGlobalOrNull(aCx));
   }
 
-  // Similar to the above.
-  if (xpc::ExtraWarningsForSystemJS() && aWorkerPrivate->UsesSystemPrincipal()) {
-      JS::CompartmentOptionsRef(global).extraWarningsOverride().set(true);
-  }
+  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!");
 
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1039,31 +1039,22 @@ public:
   : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount),
     mScriptURL(aScriptURL)
   { }
 
 private:
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
-    WorkerGlobalScope* globalScope =
-      aWorkerPrivate->GetOrCreateGlobalScope(aCx);
-    if (!globalScope) {
-      NS_WARNING("Failed to make global!");
+    if (!scriptloader::LoadMainScript(aCx, mScriptURL, WorkerScript)) {
       return false;
     }
 
-    JS::Rooted<JSObject*> global(aCx, globalScope->GetWrapper());
-
-    JSAutoCompartment ac(aCx, global);
-    bool result = scriptloader::LoadMainScript(aCx, mScriptURL, WorkerScript);
-    if (result) {
-      aWorkerPrivate->SetWorkerScriptExecutedSuccessfully();
-    }
-    return result;
+    aWorkerPrivate->SetWorkerScriptExecutedSuccessfully();
+    return true;
   }
 };
 
 class CompileDebuggerScriptRunnable final : public WorkerDebuggerRunnable
 {
   nsString mScriptURL;
 
 public:
@@ -6038,17 +6029,19 @@ WorkerPrivate::RunCurrentSyncLoop()
 
     if (normalRunnablesPending) {
       // Make sure the periodic timer is running before we continue.
       SetGCTimerMode(PeriodicTimer);
 
       MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(mThread, false));
 
       // Now *might* be a good time to GC. Let the JS engine make the decision.
-      JS_MaybeGC(cx);
+      if (JS::CurrentGlobalOrNull(cx)) {
+        JS_MaybeGC(cx);
+      }
     }
   }
 
   // Make sure that the stack didn't change underneath us.
   MOZ_ASSERT(mSyncLoopStack[currentLoopIndex] == loopInfo);
 
   return DestroySyncLoop(currentLoopIndex);
 }
@@ -6275,17 +6268,19 @@ WorkerPrivate::EnterDebuggerEventLoop()
         mDebuggerQueue.Pop(runnable);
       }
 
       MOZ_ASSERT(runnable);
       static_cast<nsIRunnable*>(runnable)->Run();
       runnable->Release();
 
       // Now *might* be a good time to GC. Let the JS engine make the decision.
-      JS_MaybeGC(cx);
+      if (JS::CurrentGlobalOrNull(cx)) {
+        JS_MaybeGC(cx);
+      }
     }
   }
 }
 
 void
 WorkerPrivate::LeaveDebuggerEventLoop()
 {
   AssertIsOnWorkerThread();
@@ -6520,17 +6515,18 @@ WorkerPrivate::ReportError(JSContext* aC
   }
 
   mErrorHandlerRecursionCount++;
 
   // Don't want to run the scope's error handler if this is a recursive error or
   // if there was an error in the close handler or if we ran out of memory.
   bool fireAtScope = mErrorHandlerRecursionCount == 1 &&
                      !mCloseHandlerStarted &&
-                     errorNumber != JSMSG_OUT_OF_MEMORY;
+                     errorNumber != JSMSG_OUT_OF_MEMORY &&
+                     JS::CurrentGlobalOrNull(aCx);
 
   if (!ReportErrorRunnable::ReportError(aCx, this, fireAtScope, nullptr, message,
                                         filename, line, lineNumber,
                                         columnNumber, flags, errorNumber, 0)) {
     JS_ReportPendingException(aCx);
   }
 
   mErrorHandlerRecursionCount--;
@@ -6913,17 +6909,17 @@ WorkerPrivate::UpdateGCZealInternal(JSCo
 #endif
 
 void
 WorkerPrivate::GarbageCollectInternal(JSContext* aCx, bool aShrinking,
                                       bool aCollectChildren)
 {
   AssertIsOnWorkerThread();
 
-  if (!JS::CurrentGlobalOrNull(aCx)) {
+  if (!GlobalScope()) {
     // We haven't compiled anything yet. Just bail out.
     return;
   }
 
   if (aShrinking || aCollectChildren) {
     JSRuntime* rt = JS_GetRuntime(aCx);
     JS::PrepareForFullGC(rt);
 
@@ -7162,23 +7158,26 @@ WorkerPrivate::GetOrCreateGlobalScope(JS
       globalScope = new DedicatedWorkerGlobalScope(this);
     }
 
     JS::Rooted<JSObject*> global(aCx);
     NS_ENSURE_TRUE(globalScope->WrapGlobalObject(aCx, &global), nullptr);
 
     JSAutoCompartment ac(aCx, global);
 
+    // RegisterBindings() can spin a nested event loop so we have to set mScope
+    // before calling it, and we have to make sure to unset mScope if it fails.
+    mScope = Move(globalScope);
+
     if (!RegisterBindings(aCx, global)) {
+      mScope = nullptr;
       return nullptr;
     }
 
     JS_FireOnNewGlobalObject(aCx, global);
-
-    mScope = globalScope.forget();
   }
 
   return mScope;
 }
 
 WorkerDebuggerGlobalScope*
 WorkerPrivate::CreateDebuggerGlobalScope(JSContext* aCx)
 {
--- a/dom/workers/WorkerScope.cpp
+++ b/dom/workers/WorkerScope.cpp
@@ -390,16 +390,30 @@ DedicatedWorkerGlobalScope::WrapGlobalOb
                                              JS::MutableHandle<JSObject*> aReflector)
 {
   mWorkerPrivate->AssertIsOnWorkerThread();
   MOZ_ASSERT(!mWorkerPrivate->IsSharedWorker());
 
   JS::CompartmentOptions options;
   mWorkerPrivate->CopyJSCompartmentOptions(options);
 
+  const bool usesSystemPrincipal = mWorkerPrivate->UsesSystemPrincipal();
+
+  // Note that xpc::ShouldDiscardSystemSource() and
+  // xpc::ExtraWarningsForSystemJS() read prefs that are cached on the main
+  // thread. This is benignly racey.
+  const bool discardSource = (usesSystemPrincipal ||
+                              mWorkerPrivate->IsInPrivilegedApp()) &&
+                             xpc::ShouldDiscardSystemSource();
+  const bool extraWarnings = usesSystemPrincipal &&
+                             xpc::ExtraWarningsForSystemJS();
+
+  options.setDiscardSource(discardSource)
+         .extraWarningsOverride().set(extraWarnings);
+
   return DedicatedWorkerGlobalScopeBinding_workers::Wrap(aCx, this, this,
                                                          options,
                                                          GetWorkerPrincipal(),
                                                          true, aReflector);
 }
 
 void
 DedicatedWorkerGlobalScope::PostMessage(JSContext* aCx,