1791715 - Stacktraces are only generated for the first 50 "throw" usage in a realm

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P2)

Description

[Julian Descottes [:jdescottes]]

Seen while working on Bug 1770754, we stop getting stacktraces after 50 errors have been thrown on a specific window global. This only impacts stacktraces which are normally not observable from JS code, ie uncaught exceptions using throw.

So in practice, executing a script.evaluate command with an expression which throws (eg a.b.c.d with a not defined in the current global), will correctly have a stacktrace even when executed more than 50 times. But with an expression which uses the throw keyword (eg throw new Error()) the stacktrace will be missing after 50 executions.

This comes from the following logic:
https://searchfox.org/mozilla-central/rev/b1e5f2c7c96be36974262551978d54f457db2cae/js/src/vm/Realm.cpp#589-606

If the realm is a debuggee, the restriction is lifted so that errors can be inspected if the console is opened.

The first question is if we should force the same behavior for Realms monitored by BiDi, or consider that the stacktrace is optional and is implementation specific.

If we decide that we always want the stacktrace for BiDi monitored Realm, we can either add a dedicated API (similar as to what was done for async stacktraces in Bug 1775207, maybe modifying and reusing that?) or switch from makeGlobalObjectReference to addDebuggee.

Given that we want BiDi to offer debugging features in the long run, we will probably need the same features as for devtools, so I'm not sure if we will be able to avoid using addDebuggee forever. We might want to look at the performance impact of doing that in more details.

Comment 1

[Tooru Fujisawa [:arai]]

For the behavior differences that come from performance optimization/tuning (that is both this bug and async stack, where the restriction/optimization was based on the assumption that the info won't be necessary, which was not true for BiDi), in my opinion, it should be fine to add per-Realm option to lift the restriction,
so I'm in favor of adding dedicated API, and I can take this if the answer for the first question is "yes".

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Julian Descottes [:jdescottes]]

We should check how this impacts performance before enabling it, but otherwise this sounds fine.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Julian, is the performance check something easy that could be done? What specifically we would have to look out for?

Comment 5

[Julian Descottes [:jdescottes]]

I don't have a specific suggestion for a performance test. If we want to do it, it might be something like generating a lot of stacktraces and checking a profile?

Comment 6

[Julian Descottes [:jdescottes]]

Should file a dedicated bug for the platform change

Comment 7

[Julian Descottes [:jdescottes]]

Filed Bug 1834990

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1791715 - [bidi] Enable unlimited stack trace capturing for BiDi realms

Depends on D179243

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed88db4c92af
[bidi] Enable unlimited stack trace capturing for BiDi realms r=webdriver-reviewers,Sasha

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ed88db4c92af