Bug 1227190 part 1. change PrepareScriptEnvironmentAndInvoke to return void, not bool, to make it clearer that it reports exceptions for you. r=jorendorff

js/src/ctypes/CTypes.cpp

@@ -6932,7 +6932,7 @@ CClosure::ClosureStub(ffi_cif* cif, void* result, void** args, void* userData)
   ArgClosure argClosure(cif, result, args, static_cast<ClosureInfo*>(userData));
   JSRuntime* rt = argClosure.cinfo->rt;
   RootedObject fun(rt, argClosure.cinfo->jsfnObj);
-  (void) js::PrepareScriptEnvironmentAndInvoke(rt, fun, argClosure);
+  js::PrepareScriptEnvironmentAndInvoke(rt, fun, argClosure);
 }
 
 bool CClosure::ArgClosure::operator()(JSContext* cx)
@@ -7025,11 +7025,14 @@ bool CClosure::ArgClosure::operator()(JSContext* cx)
       size_t copySize = CType::GetSize(fninfo->mReturnType);
       MOZ_ASSERT(copySize <= rvSize);
       memcpy(result, cinfo->errResult, copySize);
+
+      // We still want to return false here, so that
+      // PrepareScriptEnvironmentAndInvoke will report the exception.
     } else {
       // Bad case: not much we can do here. The rv is already zeroed out, so we
       // just return and hope for the best.
-      return false;
     }
+    return false;
   }
 
   // Small integer types must be returned as a word-sized ffi_arg. Coerce it

js/src/jsfriendapi.cpp

@@ -1145,24 +1145,29 @@ js::detail::IdMatchesAtom(jsid id, JSAtom* atom)
     return id == INTERNED_STRING_TO_JSID(nullptr, atom);
 }
 
-JS_FRIEND_API(bool)
+JS_FRIEND_API(void)
 js::PrepareScriptEnvironmentAndInvoke(JSRuntime* rt, HandleObject scope, ScriptEnvironmentPreparer::Closure& closure)
 {
-    if (rt->scriptEnvironmentPreparer)
-        return rt->scriptEnvironmentPreparer->invoke(scope, closure);
+    if (rt->scriptEnvironmentPreparer) {
+        rt->scriptEnvironmentPreparer->invoke(scope, closure);
+        return;
+    }
 
     MOZ_ASSERT(rt->contextList.getFirst() == rt->contextList.getLast());
     JSContext* cx = rt->contextList.getFirst();
+    MOZ_ASSERT(!cx->isExceptionPending());
     JSAutoCompartment ac(cx, scope);
     bool ok = closure(cx);
 
+    MOZ_ASSERT_IF(ok, !cx->isExceptionPending());
+
     // NB: This does not affect Gecko, which has a prepareScriptEnvironment
     // callback.
-    if (JS_IsExceptionPending(cx)) {
+    if (!ok) {
         JS_ReportPendingException(cx);
     }
 
-    return ok;
+    MOZ_ASSERT(!cx->isExceptionPending());
 }
 
 JS_FRIEND_API(void)

js/src/jsfriendapi.h

@@ -2577,11 +2577,16 @@ IdToValue(jsid id)
  * PrepareScriptEnvironmentAndInvoke will call the preparer's 'invoke' method
  * with the given |closure|, with the assumption that the preparer will set up
  * any state necessary to run script in |scope|, invoke |closure| with a valid
- * JSContext*, and return.
+ * JSContext*, report any exceptions thrown from the closure, and return.
  *
  * If no preparer is registered, PrepareScriptEnvironmentAndInvoke will assert
  * that |rt| has exactly one JSContext associated with it, enter the compartment
  * of |scope| on that context, and invoke |closure|.
+ *
+ * In both cases, PrepareScriptEnvironmentAndInvoke will report any exceptions
+ * that are thrown by the closure.  Consumers who want to propagate back
+ * whether the closure succeeded should do so via members of the closure
+ * itself.
  */
 
 struct ScriptEnvironmentPreparer {
@@ -2589,10 +2594,10 @@ struct ScriptEnvironmentPreparer {
         virtual bool operator()(JSContext* cx) = 0;
     };
 
-    virtual bool invoke(JS::HandleObject scope, Closure& closure) = 0;
+    virtual void invoke(JS::HandleObject scope, Closure& closure) = 0;
 };
 
-extern JS_FRIEND_API(bool)
+extern JS_FRIEND_API(void)
 PrepareScriptEnvironmentAndInvoke(JSRuntime* rt, JS::HandleObject scope,
                                   ScriptEnvironmentPreparer::Closure& closure);
 

js/xpconnect/src/XPCJSRuntime.cpp

@@ -1328,16 +1328,25 @@ xpc::SimulateActivityCallback(bool aActive)
     XPCJSRuntime::ActivityCallback(XPCJSRuntime::Get(), aActive);
 }
 
-// static
-bool
+void
 XPCJSRuntime::EnvironmentPreparer::invoke(HandleObject scope, js::ScriptEnvironmentPreparer::Closure& closure)
 {
     MOZ_ASSERT(NS_IsMainThread());
     nsIGlobalObject* global = NativeGlobal(scope);
-    NS_ENSURE_TRUE(global && global->GetGlobalJSObject(), false);
+
+    // Not much we can do if we simply don't have a usable global here...
+    NS_ENSURE_TRUE_VOID(global && global->GetGlobalJSObject());
     AutoEntryScript aes(global, "JS-engine-initiated execution");
     aes.TakeOwnershipOfErrorReporting();
-    return closure(aes.cx());
+
+    MOZ_ASSERT(!JS_IsExceptionPending(aes.cx()));
+
+    DebugOnly<bool> ok = closure(aes.cx());
+
+    MOZ_ASSERT_IF(ok, !JS_IsExceptionPending(aes.cx()));
+
+    // The AutoEntryScript will check for JS_IsExceptionPending on the
+    // JSContext and report it as needed as it comes off the stack.
 }
 
 // static

js/xpconnect/src/xpcprivate.h

@@ -592,7 +592,7 @@ public:
     void RemoveGCCallback(xpcGCCallback cb);
 
     struct EnvironmentPreparer : public js::ScriptEnvironmentPreparer {
-        bool invoke(JS::HandleObject scope, Closure& closure) override;
+        void invoke(JS::HandleObject scope, Closure& closure) override;
     };
     EnvironmentPreparer mEnvironmentPreparer;