Backed out 6 changesets (bug 1072144) for test_recursion.html failures
authorWes Kocher <wkocher@mozilla.com>
Tue, 01 Mar 2016 15:41:24 -0800
changeset 286341 93c0e8939efab7801d053e5721ee758963a632dd
parent 286340 9716b46f3b6067c33e7b407bd3ffcbb533703df4
child 286342 80549d9e752805da90e2c294d6bb68abcccafe38
push id72733
push userkwierso@gmail.com
push dateTue, 01 Mar 2016 23:42:27 +0000
treeherdermozilla-inbound@93c0e8939efa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1072144
milestone47.0a1
backs outac15fe0e71fda9ccbe20732221eedbbbe6b54837
b666d48a267d6b4fd990938d56c1313820447f3d
0f0464ef08edad8766273ab2dad09cb1bd6230bf
7ba5f3b95022fee5057be24ffb4c2df558aee07c
1912f838fcaa5008ce9d75d610dddf8d6edebb8e
145c9bb59b97c62d5e14e4613c6383c68223069e
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
Backed out 6 changesets (bug 1072144) for test_recursion.html failures Backed out changeset ac15fe0e71fd (bug 1072144) Backed out changeset b666d48a267d (bug 1072144) Backed out changeset 0f0464ef08ed (bug 1072144) Backed out changeset 7ba5f3b95022 (bug 1072144) Backed out changeset 1912f838fcaa (bug 1072144) Backed out changeset 145c9bb59b97 (bug 1072144) MozReview-Commit-ID: 7cl4RtpHSfl
dom/workers/RuntimeService.cpp
dom/workers/ScriptLoader.cpp
dom/workers/ServiceWorkerPrivate.cpp
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerRunnable.cpp
dom/workers/WorkerRunnable.h
--- a/dom/workers/RuntimeService.cpp
+++ b/dom/workers/RuntimeService.cpp
@@ -1661,19 +1661,16 @@ RuntimeService::UnregisterWorker(JSConte
 
     if (windowArray->IsEmpty()) {
       mWindowMap.Remove(window);
     }
   }
 
   if (queuedWorker && !ScheduleWorker(aCx, queuedWorker)) {
     UnregisterWorker(aCx, queuedWorker);
-    // There's nowhere sane to report the exception from ScheduleWorker, if any,
-    // here.
-    JS_ClearPendingException(aCx);
   }
 }
 
 bool
 RuntimeService::ScheduleWorker(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
 {
   if (!aWorkerPrivate->Start()) {
     // This is ok, means that we didn't need to make a thread for this worker.
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -302,19 +302,16 @@ public:
 private:
   ~ScriptExecutorRunnable()
   { }
 
   virtual bool
   IsDebuggerRunnable() const override;
 
   virtual bool
-  PreRun(WorkerPrivate* aWorkerPrivate) override;
-
-  virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override;
 
   virtual void
   PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
           override;
 
   NS_DECL_NSICANCELABLERUNNABLE
 
@@ -1722,51 +1719,16 @@ ScriptExecutorRunnable::IsDebuggerRunnab
 {
   // ScriptExecutorRunnable is used to execute both worker and debugger scripts.
   // In the latter case, the runnable needs to be dispatched to the debugger
   // queue.
   return mScriptLoader.mWorkerScriptType == DebuggerScript;
 }
 
 bool
-ScriptExecutorRunnable::PreRun(WorkerPrivate* aWorkerPrivate)
-{
-  aWorkerPrivate->AssertIsOnWorkerThread();
-
-  if (!mIsWorkerScript) {
-    return true;
-  }
-
-  if (!aWorkerPrivate->GetJSContext()) {
-    return false;
-  }
-
-  MOZ_ASSERT(mFirstIndex == 0);
-  MOZ_ASSERT(!mScriptLoader.mRv.Failed());
-
-  AutoJSAPI jsapi;
-  jsapi.Init();
-  jsapi.TakeOwnershipOfErrorReporting();
-
-  WorkerGlobalScope* globalScope =
-    aWorkerPrivate->GetOrCreateGlobalScope(jsapi.cx());
-  if (NS_WARN_IF(!globalScope)) {
-    NS_WARNING("Failed to make global!");
-    // There's no way to report the exception on jsapi right now, because there
-    // is no way to even enter a compartment on this thread anymore.  Just clear
-    // the exception.  We'll report some sort of error to our caller thread in
-    // ShutdownScriptLoader.
-    jsapi.ClearException();
-    return false;
-  }
-
-  return true;
-}
-
-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++) {
@@ -1778,21 +1740,42 @@ ScriptExecutorRunnable::WorkerRun(JSCont
     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?");
 
-  // Slightly icky action at a distance, but there's no better place to stash
-  // this value, really.
-  JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
+  JS::Rooted<JSObject*> global(aCx);
+
+  if (mIsWorkerScript) {
+    WorkerGlobalScope* globalScope =
+      aWorkerPrivate->GetOrCreateGlobalScope(aCx);
+    if (NS_WARN_IF(!globalScope)) {
+      NS_WARNING("Failed to make global!");
+      // There's no way to report the exception on aCx right now, because there
+      // is no way to even enter a compartment on this thread anymore.  Just
+      // clear the exception.  We'll report some sort of error to our caller
+      // thread in ShutdownScriptLoader.
+      JS_ClearPendingException(aCx);
+      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?");
--- a/dom/workers/ServiceWorkerPrivate.cpp
+++ b/dom/workers/ServiceWorkerPrivate.cpp
@@ -271,17 +271,20 @@ public:
       result.SuppressException();
       return;
     }
 
     RefPtr<Promise> waitUntilPromise = aEvent->GetPromise();
     if (!waitUntilPromise) {
       waitUntilPromise =
         Promise::Resolve(sgo, aCx, JS::UndefinedHandleValue, result);
-      MOZ_RELEASE_ASSERT(!result.Failed());
+      if (NS_WARN_IF(result.Failed())) {
+        result.SuppressException();
+        return;
+      }
     }
 
     MOZ_ASSERT(waitUntilPromise);
     RefPtr<KeepAliveHandler> keepAliveHandler =
       new KeepAliveHandler(mKeepAliveToken);
     waitUntilPromise->AppendNativeHandler(keepAliveHandler);
 
     if (aWaitUntilPromise) {
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -493,39 +493,35 @@ class CompileScriptRunnable final : publ
 public:
   explicit CompileScriptRunnable(WorkerPrivate* aWorkerPrivate,
                                  const nsAString& aScriptURL)
   : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount),
     mScriptURL(aScriptURL)
   { }
 
 private:
-  // We can't implement PreRun effectively, because at the point when that would
-  // run we have not yet done our load so don't know things like our final
-  // principal and whatnot.
-
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
     aWorkerPrivate->AssertIsOnWorkerThread();
 
     ErrorResult rv;
     scriptloader::LoadMainScript(aWorkerPrivate, mScriptURL, WorkerScript, rv);
     rv.WouldReportJSException();
     // 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 they will get
-    // reported after we return.  We do this for all failures on rv, because now
-    // we're using rv to track all the state we care about.
+    // 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,
@@ -573,19 +569,19 @@ private:
     // 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 they will get
-    // reported after we return.  We do this for all failures on rv, because now
-    // we're using rv to track all the state we care about.
+    // 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.
     if (rv.MaybeSetPendingException(aCx)) {
       return false;
     }
 
     return true;
   }
 };
 
@@ -1230,18 +1226,18 @@ private:
 
   virtual bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
     JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
     JS::Rooted<JS::Value> callable(aCx, JS::ObjectValue(*mHandler->Callable()));
     JS::HandleValueArray args = JS::HandleValueArray::empty();
     JS::Rooted<JS::Value> rval(aCx);
-    if (!JS_CallFunctionValue(aCx, global, callable, args, &rval)) {
-      // Just return false; WorkerRunnable::Run will report the exception.
+    if (!JS_CallFunctionValue(aCx, global, callable, args, &rval) &&
+        !JS_ReportPendingException(aCx)) {
       return false;
     }
 
     return true;
   }
 };
 
 void
--- a/dom/workers/WorkerRunnable.cpp
+++ b/dom/workers/WorkerRunnable.cpp
@@ -150,22 +150,16 @@ WorkerRunnable::PostDispatch(WorkerPriva
 
   if (!aDispatchResult) {
     if (mBehavior == WorkerThreadModifyBusyCount) {
       aWorkerPrivate->ModifyBusyCount(false);
     }
   }
 }
 
-bool
-WorkerRunnable::PreRun(WorkerPrivate* aWorkerPrivate)
-{
-  return true;
-}
-
 void
 WorkerRunnable::PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate,
                         bool aRunResult)
 {
   MOZ_ASSERT(aCx);
   MOZ_ASSERT(aWorkerPrivate);
 
 #ifdef DEBUG
@@ -185,16 +179,20 @@ WorkerRunnable::PostRun(JSContext* aCx, 
     default:
       MOZ_ASSERT_UNREACHABLE("Unknown behavior!");
   }
 #endif
 
   if (mBehavior == WorkerThreadModifyBusyCount) {
     aWorkerPrivate->ModifyBusyCountFromWorker(false);
   }
+
+  if (!aRunResult) {
+    JS_ReportPendingException(aCx);
+  }
 }
 
 // static
 WorkerRunnable*
 WorkerRunnable::FromRunnable(nsIRunnable* aRunnable)
 {
   MOZ_ASSERT(aRunnable);
 
@@ -257,83 +255,62 @@ WorkerRunnable::Run()
     MOZ_ASSERT(mCallingCancelWithinRun);
     mCallingCancelWithinRun = false;
 
     MOZ_ASSERT(IsCanceled(), "Subclass Cancel() didn't set IsCanceled()!");
 
     return NS_OK;
   }
 
-  bool result = PreRun(mWorkerPrivate);
-  if (!result) {
-    MOZ_ASSERT(targetIsWorkerThread,
-               "The only PreRun implementation that can fail is "
-               "ScriptExecutorRunnable");
-    mWorkerPrivate->AssertIsOnWorkerThread();
-    MOZ_ASSERT(!JS_IsExceptionPending(mWorkerPrivate->GetJSContext()));
-    // We can't enter a useful compartment on the JSContext here; just pass it
-    // in as-is.
-    PostRun(mWorkerPrivate->GetJSContext(), mWorkerPrivate, false);
-    return NS_ERROR_FAILURE;
-  }
-
-  // Track down the appropriate global, if any, to use for the AutoEntryScript.
+  // Track down the appropriate global to use for the AutoJSAPI/AutoEntryScript.
   nsCOMPtr<nsIGlobalObject> globalObject;
   bool isMainThread = !targetIsWorkerThread && !mWorkerPrivate->GetParent();
   MOZ_ASSERT(isMainThread == NS_IsMainThread());
   RefPtr<WorkerPrivate> kungFuDeathGrip;
   if (targetIsWorkerThread) {
     JSContext* cx = GetCurrentThreadJSContext();
     if (NS_WARN_IF(!cx)) {
       return NS_ERROR_FAILURE;
     }
 
     JSObject* global = JS::CurrentGlobalOrNull(cx);
     if (global) {
       globalObject = GetGlobalObjectForGlobal(global);
     } else {
       globalObject = DefaultGlobalObject();
     }
-
-    // We may still not have a globalObject here: in the case of
-    // CompileScriptRunnable, we don't actually create the global object until
-    // we have the script data, which happens in a syncloop under
-    // CompileScriptRunnable::WorkerRun, so we can't assert that it got created
-    // in the PreRun call above.
   } else {
     kungFuDeathGrip = mWorkerPrivate;
     if (isMainThread) {
       globalObject = nsGlobalWindow::Cast(mWorkerPrivate->GetWindow());
     } else {
       globalObject = mWorkerPrivate->GetParent()->GlobalScope();
     }
   }
 
   // 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.
-  Maybe<mozilla::dom::AutoJSAPI> maybeJSAPI;
+  // 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.
+  mozilla::dom::AutoJSAPI jsapi;
   Maybe<mozilla::dom::AutoEntryScript> aes;
   JSContext* cx;
-  AutoJSAPI* jsapi;
   if (globalObject) {
     aes.emplace(globalObject, "Worker runnable",
                 isMainThread,
                 isMainThread ? nullptr : GetCurrentThreadJSContext());
-    jsapi = aes.ptr();
     cx = aes->cx();
   } else {
-    maybeJSAPI.emplace();
-    maybeJSAPI->Init();
-    jsapi = maybeJSAPI.ptr();
-    cx = jsapi->cx();
+    jsapi.Init();
+    cx = jsapi.cx();
   }
-  jsapi->TakeOwnershipOfErrorReporting();
 
   // Note that we can't assert anything about mWorkerPrivate->GetWrapper()
   // existing, since it may in fact have been GCed (and we may be one of the
   // runnables cleaning up the worker as a result).
 
   // If we are on the parent thread and that thread is not the main thread,
   // then we must be a dedicated worker (because there are no
   // Shared/ServiceWorkers whose parent is itself a worker) and then we
@@ -366,43 +343,27 @@ WorkerRunnable::Run()
                js::GetObjectCompartment(mWorkerPrivate->GetWrapper()) ==
                  js::GetContextCompartment(cx),
                "Must either be in the null compartment or in our reflector "
                "compartment");
 
     ac.emplace(cx, mWorkerPrivate->GetWrapper());
   }
 
-  result = WorkerRun(cx, mWorkerPrivate);
-  MOZ_ASSERT_IF(result, !jsapi->HasException());
-  jsapi->ReportException();
-
-  // We can't even assert that this didn't create our global, since in the case
-  // of CompileScriptRunnable it _does_.
+  bool result = WorkerRun(cx, mWorkerPrivate);
 
-  // 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.
+  // 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();
+  }
+
   PostRun(cx, mWorkerPrivate, result);
-  MOZ_ASSERT(!jsapi->HasException());
 
   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
@@ -117,57 +117,41 @@ protected:
   virtual bool
   PreDispatch(WorkerPrivate* aWorkerPrivate);
 
   // By default asserts that Dispatch() is being called on the right thread
   // (ParentThread if |mTarget| is WorkerThread, or WorkerThread otherwise).
   virtual void
   PostDispatch(WorkerPrivate* aWorkerPrivate, bool aDispatchResult);
 
-  // May be implemented by subclasses if desired if they need to do some sort of
-  // setup before we try to set up our JSContext and compartment for real.
-  // Typically the only thing that should go in here is creation of the worker's
-  // global.
-  //
-  // If false is returned, WorkerRun will not be called at all.  PostRun will
-  // still be called, with false passed for aRunResult.
-  virtual bool
-  PreRun(WorkerPrivate* aWorkerPrivate);
-
   // Must be implemented by subclasses. Called on the target thread.  The return
   // value will be passed to PostRun().  The JSContext passed in here comes from
   // 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.  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.
+  // 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.
   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.  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.
+  // still in the same compartment.  If aRunResult is false, any failures on
+  // aCx are reported.  Otherwise failures are left to linger on the JSContext
+  // and maim later code (XXXbz: Aiming to fix that in bug 1072144).
   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.