This looks like it's might take some effort.

This is going to take a few steps. Here's part 1: removing CompileOptions::reportUncaught -- we can just take it as true now. The next step is going to eagerize error handling here: passing a |bool ok| through multiple levels of if and branch and conditionally doing different bits of work in the function body is, uh, not okay.

I'm not sure which of the paths I modified later in the stack can be used from workers. We assert in TakeOwnershipOfErrorReporting, so normal testing should show up any problems. None of the workers tests fail locally, so maybe none? Hopefully, this Try run will suss out any issues. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=da6f6436e1e2

Comment on attachment 8524937 [details] [diff] [review] evaluatestring_1-v0.diff Review of attachment 8524937 [details] [diff] [review]: ----------------------------------------------------------------- Youpie!

This patch eagerizes error handling in EvaluateString: the prior threaded-bool was infuriating to read and understand. Laid out like this, it is much clearer that an error at each API call does indeed call report before returning, that the rval gets cleared in the right places, and that the error return is correct. My initial patch used non-ok returns in the exception-was-pending case because that is what we do in spidermonkey. This triggered multiple test failures because we fail to fire various observers if the method fails. It seems like the return status is more like "succeeded in giving the script to SM to do something with" rather than anything to do with the script itself; a sensible enough meaning that's much clearer with the new layout. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=da7c6ed0c6e3

Pushes AutoEntryScript down through EvaluateScript. Quite trivial as all the callers already create one on stack.

Replaces the ReportPendingException calls with TakeOwnershipOfError reporting. I manually verified that this path is working as expected in a case that failed with the earlier bad patch for part 2. I'm a bit leery here because I have no idea how to figure out if any of these paths are usable from workers other than throwing it at TBPL and waiting for failures. The one saving grace here is that TakeOwnership asserts immediately, not just in the error path. Given that the worker tests pass, I'm guessing that workers can't reach EvaluateScript, but I'd prefer a more reliable answer, if possible.

Comment on attachment 8527052 [details] [diff] [review] evaluatestring_2-v1.diff Review of attachment 8527052 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that important piece fixed. ::: dom/base/nsJSUtils.cpp @@ +269,1 @@ > } You need to do something with |str| here, otherwise the coercing is a no-op.

Comment on attachment 8527055 [details] [diff] [review] evaluatestring_3-v0.diff Review of attachment 8527055 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed. ::: dom/base/nsJSUtils.h @@ +82,5 @@ > > // aEvaluationGlobal is the global to evaluate in. The return value > // will then be wrapped back into the compartment aCx is in when > // this function is called. > + static nsresult EvaluateString(mozilla::dom::AutoJSAPI& jsapi, These all should take the more-concrete AutoEntryScript, because EvaluateString may (and in fact is guaranteed to) execute script.

Comment on attachment 8527052 [details] [diff] [review] evaluatestring_2-v1.diff Review of attachment 8527052 [details] [diff] [review]: ----------------------------------------------------------------- Hm actually no, I think this isn't the right API. If callers want to ignore the failure code they can, but we should return NS_ERROR_FAILURE if the JS threw (this seems to be important in nsJSProtocolHandler.cpp, for example). So the right thing to do here is to return failure and fix up the oranges in the callers.

Comment on attachment 8527065 [details] [diff] [review] evaluatestring_4-v0.diff Review of attachment 8527065 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSUtils.cpp @@ -230,5 @@ > for (size_t i = 0; i < aEvaluateOptions.scopeChain.length(); ++i) { > JS::ExposeObjectToActiveJS(aEvaluateOptions.scopeChain[i]); > scopeChain.infallibleAppend(aEvaluateOptions.scopeChain[i]); > if (!JS_WrapObject(cx, scopeChain[i])) { > - ReportPendingException(cx); Please assert OwnsErrorReporting() at the top of this method. ::: dom/jsurl/nsJSProtocolHandler.cpp @@ +248,5 @@ > // New script entry point required, due to the "Create a script" step of > // http://www.whatwg.org/specs/web-apps/current-work/#javascript-protocol > AutoEntryScript entryScript(innerGlobal, true, > scriptContext->GetNativeContext()); > + entryScript.TakeOwnershipOfErrorReporting(); Please remove the subsequent JS_ReportPendingException call further down.

> the prior threaded-bool was infuriating to read and understand. Fwiw, it was that way originally because there were some trailing function calls to restore some state which weren't using an RAII helpe. But those have been gone for a while.

Comment on attachment 8527055 [details] [diff] [review] evaluatestring_3-v0.diff Review of attachment 8527055 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSUtils.cpp @@ +163,5 @@ > return NS_OK; > } > > nsresult > +nsJSUtils::EvaluateString(mozilla::dom::AutoJSAPI& jsapi, a- prefix @@ +186,5 @@ > const EvaluateOptions& aEvaluateOptions, > JS::MutableHandle<JS::Value> aRetValue, > void **aOffThreadToken) > { > + JSContext *cx = jsapi.cx(); * goes on the left

Comment on attachment 8524937 [details] [diff] [review] evaluatestring_1-v0.diff In the meantime, it looks like this has already been done.

Comment on attachment 8527065 [details] [diff] [review] evaluatestring_4-v0.diff This is also done.

Looks like the major pieces of this bug have all landed. There's still the removal of |ok| and passing the AutoJSAPI directly, but those seem like they'd be more appropriate in different bugs.

Yeah, this got fixed in bug 1174486. I'm sorry I'd forgotten about this wip when doing that. :(