2036256 - Assertion failure: zone->needsMarkingBarrier(), at checkouts/gecko/js/src/gc/Marking.cpp:951

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Tyson Smith [:tsmith]]

Found with m-c 20260430-3d6e8c604cf2 (--enable-debug)

This was found by visiting a live website with a debug build. Marking as s-s as a precaution until reviewed.

STR:

• Launch browser and visit site

This issue was triggered by visiting http://www.icloud.com/keynote/0hmu8sUZRot_9XJACva_hdnKA#DocGroup%5FLabeling%5Fin%5FQuantum%5FDOM.

Assertion failure: zone->needsMarkingBarrier(), at checkouts/gecko/js/src/gc/Marking.cpp:951

0|0|xul.dll|js::gc::PerformIncrementalReadBarrier(js::gc::TenuredCell*)|git:github.com/mozilla-firefox/firefox:js/src/gc/Marking.cpp:fcf41d17dfad8cf127eaccab48a783899ee01d4c|951|0x199
0|1|xul.dll|js::jit::WeakMapValueReadBarrier(js::gc::TenuredCell*, JS::Zone*)|git:github.com/mozilla-firefox/firefox:js/src/jit/VMFunctions.cpp:fcf41d17dfad8cf127eaccab48a783899ee01d4c|3460|0x80
0|2|||||
0|3|xul.dll|js::CheckContextLocal::check() const|git:github.com/mozilla-firefox/firefox:js/src/threading/ProtectedData.cpp:fcf41d17dfad8cf127eaccab48a783899ee01d4c|51|0x67
0|4|||||
0|5|xul.dll|js::jit::AssertPropertyLookup(js::NativeObject*, JS::PropertyKey, unsigned int)|git:github.com/mozilla-firefox/firefox:js/src/jit/VMFunctions.cpp:fcf41d17dfad8cf127eaccab48a783899ee01d4c|3446|0x8b

Comment 1

[Jan de Mooij [:jandem]]

Test case:

// Assertion failure: zone->needsMarkingBarrier(), at gc/Marking.cpp:951
//
// visitWeakMapGetObject in Ion hardcodes the compilation zone as an immediate
// for the WeakMapValueReadBarrier call. If the stored value is a Symbol (atoms
// zone) and a zone-specific incremental GC runs, the compilation zone needs a
// marking barrier but the symbol's zone (atoms zone) does not, causing the
// assertion in PerformIncrementalReadBarrier to fire.
//
// Run: js --fast-warmup --no-threads test.js
let wm = new WeakMap();
let key = {};
wm.set(key, Symbol());
function get() {
  return wm.get(key);
}
function main() {
  get();
  schedulezone(key);
  startgc(1);
  while (gcstate() !== "NotActive") {
    get();
    gcslice(100);
  }
}
main();

Comment 3

[Iain Ireland [:iain]]

Attachment: (secure)

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 2036256: Add test r=jonco

I'm not sure how bad the bug is, so I'm putting the test in a separate patch just in case.

Comment 5

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/NAe03TX5fadL_mTTRZREVw/index.html

Sorry it was delayed coming in, but since I already had the session I might as well attach it.

Comment 6

[Andrew McCreight [:mccr8]]

Is this a security issue, Jon? It sounds like we're doing a read barrier on something that doesn't need one? Thanks.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
The problem is we can miss the read barrier in a zone GC because the code currently checks the wrong zone. This means we can leave a symbol or marked gray incorrectly, creating a black to gray edge.

Comment 8

[Andrew McCreight [:mccr8]]

Thanks. I think I've been calling black to gray edges sec-moderate. They can lead to a "use after unlink" which could result in a use after free if an unlink method is doing something weird.

Comment 9

[Iain Ireland [:iain]]

Comment on attachment 9575910 [details]
(secure)

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Potential for creating a black->gray edge
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple local fix. Clearly more correct than existing code.
• String changes made/needed:
• Is Android affected?: Yes

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/634574618127
https://hg.mozilla.org/integration/autoland/rev/86630c751f07
Disambiguate zones r=jonco

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/86630c751f07

Comment 12

[Dianna Smith [:diannaS]]

Comment on attachment 9575910 [details]
(secure)

Approved for 150.0.2 dot release and 151.0b7

Comment 13

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/c0eb0e81c0fd
https://hg.mozilla.org/releases/mozilla-release/rev/2de0c75612bc

Comment 14

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/8b0cd9b99576
https://hg.mozilla.org/releases/mozilla-beta/rev/32164f806375

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1d36b3661a35
https://hg.mozilla.org/integration/autoland/rev/f2f190de32c3
Add test r=jonco

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3b1a6fe2d5fe
https://hg.mozilla.org/integration/autoland/rev/a3952b8fada0
Revert "Bug 2036256: Add test r=jonco" for causing sm bustages in bug2036256.js.

Comment 17

[Serban Stanca [:SerbanS]]

Reverted this because it was causing sm bustages in bug2036256.js

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/gc/bug2036256.js | /builds/worker/checkouts/gecko/js/src/jit-test/tests/gc/bug2036256.js:12:10 Error: Incremental GC already in progress (code 3, args "--fast-warmup --no-threads") [0.1 s]

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/cf52112a2b7f
https://hg.mozilla.org/integration/autoland/rev/f624a5e13e8a
Add test r=jonco

Comment 19

[amarc]

https://hg.mozilla.org/mozilla-central/rev/f624a5e13e8a