Bug 1072144 part 5. Stop fiddling with compartments on the JSContext before calling PostRun in WorkerRunnable::Run. Add some documentation explaining what's going on. r=khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 01 Mar 2016 16:52:26 -0500
changeset 322822 856f85004f4363d395fbb4aab8109995bc1b39c3
parent 322821 d8745ad21a39a7e1a23e5571ff5ab582b51da68e
child 322823 0d93fe6d72c6722c2e825594b6f2f3e52736ccda
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 5. Stop fiddling with compartments on the JSContext before calling PostRun in WorkerRunnable::Run. Add some documentation explaining what's going on. r=khuey
dom/workers/WorkerRunnable.cpp
dom/workers/WorkerRunnable.h
--- a/dom/workers/WorkerRunnable.cpp
+++ b/dom/workers/WorkerRunnable.cpp
@@ -307,19 +307,16 @@ WorkerRunnable::Run()
     }
   }
 
   // We might run script as part of WorkerRun, so we need an AutoEntryScript.
   // This is part of the HTML spec for workers at:
   // http://www.whatwg.org/specs/web-apps/current-work/#run-a-worker
   // If we don't have a globalObject we have to use an AutoJSAPI instead, but
   // this is OK as we won't be running script in these circumstances.
-  // It's important that aes is declared after jsapi, because if WorkerRun
-  // creates a global then we construct aes before PostRun and we need them to
-  // be destroyed in the correct order.
   Maybe<mozilla::dom::AutoJSAPI> jsapi;
   Maybe<mozilla::dom::AutoEntryScript> aes;
   JSContext* cx;
   if (globalObject) {
     aes.emplace(globalObject, "Worker runnable",
                 isMainThread,
                 isMainThread ? nullptr : GetCurrentThreadJSContext());
     cx = aes->cx();
@@ -372,25 +369,36 @@ WorkerRunnable::Run()
 
   result = WorkerRun(cx, mWorkerPrivate);
   MOZ_ASSERT_IF(result, !JS_IsExceptionPending(cx));
   JS_ReportPendingException(cx);
 
   // We can't even assert that this didn't create our global, since in the case
   // of CompileScriptRunnable it _does_.
 
-  // 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();
-  }
-
+  // It would be nice to avoid passing a JSContext to PostRun, but in the case
+  // of ScriptExecutorRunnable we need to know the current compartment on the
+  // JSContext (the one we set up based on the global returned from PreRun) so
+  // that we can sanely do exception reporting.  In particular, we want to make
+  // sure that we do our JS_SetPendingException while still in that compartment,
+  // because otherwise we might end up trying to create a cross-compartment
+  // wrapper when we try to move the JS exception from our runnable's
+  // ErrorResult to the JSContext, and that's not desirable in this case.
+  //
+  // We _could_ skip passing a JSContext here and then in
+  // ScriptExecutorRunnable::PostRun end up grabbing it from the WorkerPrivate
+  // and looking at its current compartment.  But that seems like slightly weird
+  // action-at-a-distance...
+  //
+  // In any case, we do NOT try to change the compartment on the JSContext at
+  // this point; in the one case in which we could do that
+  // (CompileScriptRunnable) it actually doesn't matter which compartment we're
+  // in for PostRun.
   PostRun(cx, mWorkerPrivate, result);
+  MOZ_ASSERT(!JS_IsExceptionPending(cx));
 
   return result ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 WorkerRunnable::Cancel()
 {
   uint32_t canceledCount = ++mCanceled;
--- a/dom/workers/WorkerRunnable.h
+++ b/dom/workers/WorkerRunnable.h
@@ -137,33 +137,37 @@ protected:
   // an AutoJSAPI (or AutoEntryScript) that we set up on the stack.  If
   // mBehavior is ParentThreadUnchangedBusyCount, it is in the compartment of
   // mWorkerPrivate's reflector (i.e. the worker object in the parent thread),
   // unless that reflector is null, in which case it's in the compartment of the
   // 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.
+  // called.  This is actually important for cases when a runnable spins a
+  // syncloop and wants everything that happens during the syncloop to happen in
+  // the compartment that runnable set up (which may, for example, be a debugger
+  // sandbox compartment!).  If aCx wasn't in a compartment to start with, 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.
+  // still in the same compartment.  PostRun implementations must NOT leave an
+  // exception on the JSContext and must not run script, because the incoming
+  // JSContext may be in the null 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.