Closed Bug 1667913 Opened 4 years ago Closed 3 years ago

Enable incremental weakmap marking in the browser

Categories

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

defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox-esr78 --- disabled
firefox-esr91 --- disabled
firefox83 --- disabled
firefox94 --- disabled
firefox95 --- disabled
firefox96 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression, sec-other, Whiteboard: [post-critsmash-triage])

Attachments

(4 files, 1 obsolete file)

As I was horrified to discover, bug 1633176 did not enable incremental weakmap marking in the browser after all. It only enabled it in the shell. I did not update the browser's prefs file, so because it's a boolean pref it defaulted to false.

I also found one bug when enabling it in the browser.

Group: core-security

Also weaken an overly strong assertion.

Assignee: nobody → sphink
Status: NEW → ASSIGNED

Also note that with this bug and bug 1667912 fixed, I don't get any more errors when swapping the order of weak/gray marking, whether with incremental or non-incremental weakmap marking.

It seems safest to land bug 1667912 first, to fix the current non-incremental weakmap marking. At any point thereafter, the two patches here might then be landed either separately or together. One fixes incremental weakmap marking, the other enables it, but I believe there is still at least one other bug still remaining in the incremental version (bug 1658654).

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:sfink, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(sphink)

I think this should probably be a sec-other. The current situation, where weakmap marking is still nonincremental, is a bug but it's safer and currently more correct. At the same time, I would rather not have it exposed that this is what's going on.

Before landing this bug, any remaining known issues in the incremental mode of weakmap marking should be resolved.

I'll try to set dependencies.

Depends on: 1658654
Flags: needinfo?(sphink)
Keywords: sec-highsec-other
Attachment #9178338 - Attachment is obsolete: true
Blocks: 1735591

The assertion failure I looked at turned out to be kind of nasty. (But I could reproduce in Pernosco, which really helped.)

We mark a map in zone 1 and find an unmarked CCW key to a delegate in zone 2, so add a <weakmap, delegate> -> key edge to the ephemeron table. Later in the same incremental GC, zone 1 gets nuked. Then we enter sweeping, and compute sweep groups. Zone 1 is put into sweep group 3, zone 2 is sweep group 4, because at this point in time there's no edge between the zones since the CCW was nuked. We finish sweep group 3, then when marking the delegate in sweep group 4 we try to mark the key while its zone is no longer marking.

Because nuking a CCW will conservatively mark the key already, these ephemeron edges are unneeded. I thought they were harmless, but this shows that they are at least going to cause a DEBUG assertion failure. It's pretty easy to erase them, though.

Blocks: 1507440
Group: javascript-core-security → core-security-release
Flags: needinfo?(sphink)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Has Regression Range: --- → yes
Keywords: regression
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: