Remove ReportPendingException call in EvaluateString

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
This looks like it's might take some effort.
(Assignee)

Comment 1

4 years ago
Created attachment 8524937 [details] [diff] [review]
evaluatestring_1-v0.diff

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.
Attachment #8524937 - Flags: review?(bobbyholley)
(Assignee)

Comment 2

4 years ago
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!
Attachment #8524937 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 8527052 [details] [diff] [review]
evaluatestring_2-v1.diff

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
Attachment #8527052 - Flags: review?(bobbyholley)
(Assignee)

Comment 5

4 years ago
Created attachment 8527055 [details] [diff] [review]
evaluatestring_3-v0.diff

Pushes AutoEntryScript down through EvaluateScript. Quite trivial as all the callers already create one on stack.
Attachment #8527055 - Flags: review?(bobbyholley)
(Assignee)

Comment 6

4 years ago
Created attachment 8527065 [details] [diff] [review]
evaluatestring_4-v0.diff

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.
Attachment #8527065 - Flags: review?(bobbyholley)
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.
Attachment #8527052 - Flags: review?(bobbyholley) → review+
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.
Attachment #8527055 - Flags: review?(bobbyholley) → review+
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.
Attachment #8527052 - Flags: review+ → review-
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.
Attachment #8527065 - Flags: review?(bobbyholley) → review+
> 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
(Assignee)

Comment 13

4 years ago
Comment on attachment 8524937 [details] [diff] [review]
evaluatestring_1-v0.diff

In the meantime, it looks like this has already been done.
Attachment #8524937 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Comment on attachment 8527065 [details] [diff] [review]
evaluatestring_4-v0.diff

This is also done.
Attachment #8527065 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Yeah, this got fixed in bug 1174486.  I'm sorry I'd forgotten about this wip when doing that.  :(
Depends on: 1174486
You need to log in before you can comment on or make changes to this bug.