1529991 - Non-relazifiable scripts break debugger script identity

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Ted Campbell [:tcampbell]]

If I take test [1] and change the functions to contain |eval("")| the test unexpectedly fails. This is due to the function now being marked as non-relazifiable and doesn't store a WeakRef<LazyScript*> pointer anymore. The result is the debugger mixes up script identity and thinks the DebuggerScript is different. This issue also occurs if an inner function is used.

If Bug 1529456 is able to successfully merge JSScript and LazyScript instances, then bug should go away on it's own (as well as a lot of debugger code being removed)

[1] https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/debug/Debugger-findScripts-06.js

Comment 1

[Jason Orendorff [:jorendorff]]

(In reply to Ted Campbell [:tcampbell] from comment #0)

If Bug 1529456 is able to successfully merge JSScript and LazyScript instances, then bug should go away on it's own (as well as a lot of debugger code being removed)

...which is the outcome ardently to be hoped for. But if not, the fix would be to notify Debugger when creating a JSScript with no LazyScript back-pointer. Any existing Debugger.Script objects need to be removed from the table and re-added with a different key.

Comment 2

[Jim Blandy :jimb]

If the pointer to the lazy script was a WeakRef, and Debugger was relying on that to get script identity right, does that mean that Debugger's behavior was affected by GC?

Comment 3

[Jason Orendorff [:jorendorff]]

I don't think it's actually a WeakRef. Here's the field declaration.

Comment 4

[Ted Campbell [:tcampbell]]

Correct, the JSScript -> LazyScript is a strong reference. The LazyScript -> JSScript ref is a WeakRef but that shouldn't affect identity.

I don't expect GC non-determinism here, but rather an issue of when delazification happens. Eg. taking a DebugObject<Script> before first triggering delazification.

Comment 5

[Brian Hackett [Laid off!]]

While working on bug 1556813 I noticed this problem and have a fix waiting for review in https://phabricator.services.mozilla.com/D34962. That patch allows Debugger users to more readily access scripts without triggering delazification (in particular, the script's child hierarchy can be crawled without delazifying), which makes it easier to trigger this bug. The fix adds a flag on LazyScripts, WrappedByDebugger, which is set when a debugger wraps a LazyScript. If the script is delazified later, the resulting script will have a pointer to the LazyScript, whether it is relazifiable or not, so the debugger can consistently find the canonical LazyScript to wrap. By unnecessarily entraining the LazyScript this costs a bit of memory when debugging but that doesn't seem too horrible. While bug 1529456 would be a great way to fix this, it might be nice to get the problem taken care now because it is visible to Debugger users.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Bug 1529991 Part 1 - Ensure Debugger.Script identity for scripts that can't be relazified, r=tcampbell.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Bug 1529991 Part 2 - Avoid unnecessary delazification in the Debugger, r=jorendorff.

Comment 8

[Brian Hackett [Laid off!]]

Looking at the patch again, it would make more sense in this bug than as part of bug 1556813. This fix isn't necessary for bug 1556813 and doesn't depend on the other patches there, but by itself is a pretty significant performance improvement when recording/replaying complex sites which benefit from lazy parsing.

Comment 9

[Ted Campbell [:tcampbell]]

Brian are there any issues left preventing landing these? They also are solving some test failures I'm hitting on a local wip branch.

Comment 10

[Brian Hackett [Laid off!]]

Oops, I'll try to land these patches today.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6365885e2c43
Part 1 - Ensure Debugger.Script identity for scripts that can't be relazified, r=tcampbell.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09f20d0f43cc
Part 2 - Avoid unnecessary delazification in the Debugger, r=jorendorff.

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6365885e2c43
https://hg.mozilla.org/mozilla-central/rev/09f20d0f43cc

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6365885e2c43
https://hg.mozilla.org/releases/mozilla-beta/rev/09f20d0f43cc