Closed Bug 1756567 Opened 3 years ago Closed 3 years ago

Use a WeakMap to hold cross-zone wrappers for FinalizationRecordObjects

Categories

(Core :: JavaScript: GC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox97 --- unaffected
firefox98 --- unaffected
firefox99 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Regression)

Details

(Keywords: regression, sec-high, Whiteboard: [sec-survey])

Crash Data

Attachments

(4 files)

In bug 1749298 we added a map to count cross-zone wrappers between FinalizationRegistry targets and FinalizationRecordObjects so that we could calculate sweep groups correctly.

If we change this to put the wrappers in a WeakMap (used as a weak set) we will get this behaviour automatically and also preserve the wrappers. This will allow us to remove the non-standard tracing code for the record objects which traces cross-zone wrappers and has to have a comment explaining why this is safe when it normally wouldn't be.

Further, using the approach for WeakRefs will fix bug 1755725.

Summary: Use a WeakMap to arrange sweep groups for FinalizationRegistries → Use a WeakMap to hold cross-zone wrappers for FinalizationRecordObjects

The non-standard tracing code turns out to be a problem for other reasons too. Marking s-s before duping bug 1756590 to this one.

Group: javascript-core-security

This has two effects:

  • it keeps the wrapper alive as long as the record is alive (because it's a
    WeakMap key delegate)
  • it ensures the target zone and registry zone are swept in the same sweep
    group.

This allows us to remove the special tracing of cross compartment wrappers for
records and that caused problems in bug 1755874. This becomes normal tracing of
finalization record objects from their own global.

It also allows us to remove the sweep group handling code from
FinalizationRegistry.cpp because the WeakMap will now handle this for us by
adding sweep group edges which are the reverse of those of the
cross-compartment wrappers.

Depends on D139397

Crash Signature: [@ js::gc::TenuredCell::zone() const]
Keywords: sec-high
Regressed by: 1749298

I'm going to mark this sec-high because the dupe had a giant pile of GC-related crashes, so maybe something in there is bad.

Severity: -- → N/A
Priority: -- → P1

Set release status flags based on info from the regressing bug 1749298

Has Regression Range: --- → yes
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(jcoppeard)
Whiteboard: [sec-survey]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Group: core-security-release
Type: task → defect
Flags: needinfo?(jcoppeard)
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: