Closed Bug 1529991 Opened 9 months ago Closed 3 months ago

Non-relazifiable scripts break debugger script identity

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: tcampbell, Assigned: bhackett)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

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

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

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?

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

Priority: -- → P3

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.

Depends on: 1566466

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.

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.

Assignee: nobody → bhackett1024
Blocks: 1529456
No longer depends on: 1529456

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

Flags: needinfo?(bhackett1024)

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

Flags: needinfo?(bhackett1024)
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6365885e2c43
Part 1 - Ensure Debugger.Script identity for scripts that can't be relazified, r=tcampbell.
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09f20d0f43cc
Part 2 - Avoid unnecessary delazification in the Debugger, r=jorendorff.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.