Closed Bug 1590176 Opened 5 years ago Closed 5 years ago

Relazified scripts should not revive without updating canonical function

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

Details

Attachments

(1 file, 1 obsolete file)

While working on Bug 1589904 I stumbled on a preexisting issue where after relazification, but before the incremental-gc has swept a JSScript, we can revive it without making the canonical JSFunction non-lazy. This is a bug according to [1] and according to good sense.

This could be fixed in JSFunction::createScriptForLazilyInterpretedFunction, but we would also need to be careful that places accessing LazyScript::maybeScript() directly aren't breaking things too.

[1] https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/js/src/vm/JSFunction.cpp#1652-1653

Type: task → defect
Attached patch Test Case (obsolete) — Splinter Review

Here is a test case and an assert that matches the stated invariants.

Attachment #9103818 - Attachment is patch: true

This fixes a bug where relazification during an incremental GC may lead
to a JSScript being revived. When this happens, it was possible that
only a function clone would be delazify and the JSFunction would remain
lazy. This breaks basic invariants of the system.

The fix is to only allow this revival on canonical functions. A
non-canonical function will then need to delazify the canonical function
first and invariants are preserved. The call to JSScript::setLazyScript
is also removed since it was being guarded by a release assert ensuring
the value was already correct.

Attachment #9103818 - Attachment is obsolete: true

The fundamental issue is that at [1] we set a function to non-lazy without first ensuring the canonical function is non-lazy. Every other location we turn a JSFunction from lazy to non-lazy we are working on the canonical function.

[1] https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/js/src/vm/JSFunction.cpp#1654

Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b4e44680b99
Ensure canonical function is always delazified first r=jandem
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Regressions: 1591921
No longer regressions: 1591921
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: