Closed Bug 1791715 Opened 2 years ago Closed 1 year ago

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

Categories

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

defect
Points:
2

Tracking

(firefox115 fixed)

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [webdriver:m7][webdriver:relnote])

Attachments

(1 file)

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.

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".

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(hskupin)

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

Severity: -- → S3
Priority: -- → P3

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

Flags: needinfo?(hskupin)
Flags: needinfo?(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?

Flags: needinfo?(jdescottes)

Should file a dedicated bug for the platform change

Points: --- → 2
Flags: needinfo?(jdescottes)
Priority: P3 → P2
Whiteboard: [webdriver:m7]
Depends on: 1834990
Flags: needinfo?(jdescottes)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ed88db4c92af [bidi] Enable unlimited stack trace capturing for BiDi realms r=webdriver-reviewers,Sasha
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
Whiteboard: [webdriver:m7] → [webdriver:m7][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: