1770477 - Support handling of Javascript errors for "script.evaluate" command

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, enhancement, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

With bug 1742979 we will get basic support for script.evaluate. This bug covers the addition of handling Javascript errors and the creation of appropriate webdriver and maybe xpcshell tests. Further we should make sure to also include async stack trace support (see bug 1747061 for the same feature but for log.entryAdded).

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1770477 - [bidi] Handle javascript errors in script.evaluate

Depends on D149412

Comment 2

[Julian Descottes [:jdescottes]]

Paraphrasing my comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1745440#c1, handling async errors needs discussion.

Async call stacks are only reported for Debuggees (ie globals on which addDebuggee was used).

We can lift this restriction by flipping the preference javascript.options.asyncstack_capture_debuggee_only to false, after what async stack traces will always be collected.

It's not clear yet what is the best approach here, between calling addDebuggee on all inspected globals, or flipping the preference. We probably want to pick the solution which has the least impact on the browser overall. Using addDebuggee would only impact the globals BiDi is currently interacting with. However it might have other consequences on the behavior/performance of that global. See usage of IsDebuggee at https://searchfox.org/mozilla-central/search?q=symbol:_ZNK2JS5Realm10isDebuggeeEv&redirect=false

Comment 3

[Julian Descottes [:jdescottes]]

Hi Alex, since we need to handle async errors in BiDi (for script evaluate, but also for error messages in general see https://bugzilla.mozilla.org/show_bug.cgi?id=1745440#c1), we should either go ahead and use addDebuggee on all relevant globals, or flip the javascript.options.asyncstack_capture_debuggee_only preference.

Do you have any advice on what might be the least impactful for the browser?

I tend to think that using addDebuggee is more fine grained and would be better. It's also something which will be required whenever we want to start supporting actual debugging scenarios with BiDi. We could take a conservative approach and use addDebuggee only when the user actually starts monitoring errors (ie starts listening to log.entryAdded events for a given global) or wants to evaluate an async script.

What do you think?

Comment 4

[Julian Descottes [:jdescottes]]

In this bug we will also have to make sure the base URL for the script is correctly set as discussed at https://phabricator.services.mozilla.com/D148907#inline-822780.

Unless we have realm support this should probably be the base URL for the document, ie the same thing as what we return from the windowglobal/browsingContext _getBaseURL command: this.messageHandler.window.document.baseURI.

We could either call the browsingContext command from the script module or directly reuse the snippet.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #2)

Paraphrasing my comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1745440#c1, handling async errors needs discussion.

Async call stacks are only reported for Debuggees (ie globals on which addDebuggee was used).

We can lift this restriction by flipping the preference javascript.options.asyncstack_capture_debuggee_only to false, after what async stack traces will always be collected.

It's not clear yet what is the best approach here, between calling addDebuggee on all inspected globals, or flipping the preference. We probably want to pick the solution which has the least impact on the browser overall. Using addDebuggee would only impact the globals BiDi is currently interacting with. However it might have other consequences on the behavior/performance of that global. See usage of IsDebuggee at https://searchfox.org/mozilla-central/search?q=symbol:_ZNK2JS5Realm10isDebuggeeEv&redirect=false

Hi arai! I'm trying to assess the performance impact of calling addDebuggee on a given global, compared to makeGlobalObjectReference. We use this in order to evaluate JS in that global, using executeInGlobal. Looking at the searchfox link above, it seems that using addDebuggee will lead to opt out of a few optimisations. But since it comes with a very useful feature for us (inspecting async stack traces) I would like to measure the actual performance impact.

Do you have any suggestion about JavaScript snippets which should perform worse if I use addDebuggee instead of makeGlobalObjectReference? So far I've tried to run heavily jit-ed code, but I wasn't able to measure any difference.

Comment 6

[Tooru Fujisawa [:arai]]

If there's issue about making the target global a debuggee, we could look into adding yet another flag to temporarily record the resolution site information for given global or context.

The related code is the following:

https://searchfox.org/mozilla-central/rev/70504e4c6fe61c027675c792d328ee476b6b1c1f/js/src/builtin/Promise.cpp#477-484

static void setResolutionInfo(JSContext* cx, Handle<PromiseObject*> promise,
                              Handle<SavedFrame*> unwrappedRejectionStack) {
  MOZ_ASSERT_IF(unwrappedRejectionStack,
                promise->state() == JS::PromiseState::Rejected);

  if (!JS::IsAsyncStackCaptureEnabledForRealm(cx)) {
    return;
  }

https://searchfox.org/mozilla-central/rev/70504e4c6fe61c027675c792d328ee476b6b1c1f/js/src/jsapi.cpp#4581-4588

JS_PUBLIC_API bool JS::IsAsyncStackCaptureEnabledForRealm(JSContext* cx) {
  if (!cx->options().asyncStack()) {
    return false;
  }

  return !cx->options().asyncStackCaptureDebuggeeOnly() ||
         cx->realm()->isDebuggee();
}

Currently, asyncStackCaptureDebuggeeOnly() is true there, and that requires the global to be debuggee.
We could add yet another flag that's turned on when start evaluating the code, and turned off when the evaluation finishes, so that no other debuggee-specific de-optimization are applied.

The target of the capturing is limited to debuggee because capturing the info is costly (see bug 1152893 etc), and the information was necessary mostly when the global is debuggee. So, basically there's no problem enabling it for some short time span for specific context or global if necessary.

About the perf impact and snippets, I'll look into it today.

Comment 7

[Julian Descottes [:jdescottes]]

Thanks for looking into this!

So, basically there's no problem enabling it for some short time span for specific context or global if necessary.

We can use that approach when we evaluate scripts, but there's at least another spot where we wanted to provide async stack traces which will be harder to handle this way: page errors observed using the log.entryAdded event. For those we would have no other choice than to enable the behavior during the whole lifetime of the global, since we don't know when an error might occur.

Although, something we discussed yesterday with the team, if monitoring async stack traces really comes with a performance hit, we can make that behavior optional and clearly document that enabling it can affect page performance.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1770477 - [bidi] Return the innerWindowId as realm id for script.evaluate

Depends on D149413

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1770477 - [wdspec] Add tests for errors and exceptions

depends on D150245

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6709036fd19c
[bidi] Handle javascript errors in script.evaluate r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/8b228fea81bf
[bidi] Return the innerWindowId as realm id for script.evaluate r=webdriver-reviewers,jgraham,whimboo

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6709036fd19c
https://hg.mozilla.org/mozilla-central/rev/8b228fea81bf