2044836 - Assertion failure: hasMarkingWork(), at js/src/gc/Sweeping.cpp:1336

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20260603-14f381ab3699 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

grayRoot()
function a() {
  enqueueMark("yield")
  enqueueMark("drain")
  gcslice(100000)
}
for (b of [, , ])
  for (c of [, , , 'otherzone'])
    a();

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555575bdf15 in js::gc::GCRuntime::markGray(JS::GCContext*, JS::SliceBudget&) ()
#0  0x00005555575bdf15 in js::gc::GCRuntime::markGray(JS::GCContext*, JS::SliceBudget&) ()
#1  0x00005555575fec6c in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#2  0x00005555575e8359 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#3  0x00005555575ccd14 in js::gc::GCRuntime::sweepPhase(JS::SliceBudget&) ()
#4  0x000055555751c251 in js::gc::GCRuntime::incrementalSlice(JS::SliceBudget&, JS::GCReason, bool) ()
#5  0x0000555557520002 in js::gc::GCRuntime::gcCycle(bool, JS::SliceBudget const&, JS::GCReason) ()
#6  0x0000555557521975 in js::gc::GCRuntime::collect(bool, JS::SliceBudget const&, JS::GCReason) ()
#7  0x00005555571fe42f in GCSlice(JSContext*, unsigned int, JS::Value*) ()
#8  0x0000555557dfea75 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
[...]
#20 0x0000555556eadb4e in main ()
rax	0x0	0
rbx	0x7fffee2387b8	140737188693944
rcx	0x538	1336
rdx	0x7ffff7804563	140737345766755
rsi	0x0	0
rdi	0x7ffff7805700	140737345771264
rbp	0x7fffffffc630	140737488340528
rsp	0x7fffffffc5a0	140737488340384
r8	0x0	0
r9	0x3	3
r10	0x2	2
r11	0x293	659
r12	0x1	1
r13	0x7fffffffc5d0	140737488340432
r14	0x7fffee238918	140737188694296
r15	0x7fffffffc8a0	140737488341152
rip	0x5555575bdf15 <js::gc::GCRuntime::markGray(JS::GCContext*, JS::SliceBudget&)+645>
=> 0x5555575bdf15 <_ZN2js2gc9GCRuntime8markGrayEPN2JS9GCContextERNS2_11SliceBudgetE+645>:	mov    %rcx,(%rax)
   0x5555575bdf18 <_ZN2js2gc9GCRuntime8markGrayEPN2JS9GCContextERNS2_11SliceBudgetE+648>:	call   0x555556f51a70 <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20260603204655-14f381ab3699.
The bug appears to have been introduced in the following build range:

Start: 6087fd009558cbb64f7b2804eaa4cf377fc4da5e (20260311191918)
End: a3a56502097a58707759d7893cd0d6a95730a746 (20260311202658)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6087fd009558cbb64f7b2804eaa4cf377fc4da5e&tochange=a3a56502097a58707759d7893cd0d6a95730a746

Comment 4

[Andrew McCreight [:mccr8]]

Bug 2022017 maybe?

Comment 5

[Jan de Mooij [:jandem]]

sm-audit found this too. Below is the write-up, in case it's helpful.

Root cause

GCRuntime::markSynchronously (GC.cpp:3588) can return NotFinished for a
reason unrelated to remaining marking work: processTestMarkQueue()
returning QueueYielded (GC.cpp:3605). The test mark queue's "drain"
directive (GC.cpp:3773) drains the main marker completely via
markUntilBudgetExhausted(unlimited); a following "yield" directive then
returns QueueYielded during an incremental GC. At that point
markSynchronously returns NotFinished while every marker is drained and
there is no delayed marking.

Commit 1b46536f11c3 (Bug 1693542 Part 11, the audited commit) introduced
the pattern of ignoring markSynchronously's return value and consulting
hasMarkingWork() instead — markPhase (GC.cpp:3569) and
markDuringSweeping (Sweeping.cpp:1977) both do this, which silently
swallows a queue yield when the markers are drained. The follow-up commit
4303413ab050 (Bug 2022017 Part 2) added gray-marking support on this
pattern in GCRuntime::markGray (Sweeping.cpp:1325) but instead asserted
the implication that NotFinished => hasMarkingWork():

if (markSynchronously(mainThreadBudget, useParallelMarking) == NotFinished) {
  MOZ_ASSERT(hasMarkingWork());   // <-- fails
  MOZ_ASSERT(isIncremental);
  MOZ_ASSERT(safeToYield);
  ...
}

That implication does not hold on the QueueYielded path.

Why the queue survives until markGray

The subtlety is that markDuringSweeping runs at the start of every sweep
slice (Sweeping.cpp:2767) and would normally consume the queue's
gray-segment first (harmlessly, since it ignores the return value). The
repro keeps the queue suspended until markGray runs by placing an object
from a second zone B after "set-color-gray":

• During black marking, processTestMarkQueue suspends at
  "set-color-gray" because state() != State::Sweep (GC.cpp:3779).
• During sweep slices before zone B's sweep group begins gray marking, the
  queue suspends at the zone-B object because the queue color is gray and
  B is still isGCMarkingBlackOnly() (GC.cpp:3728).
• When B's sweep group reaches beginMarkingSweepGroup /
  markGrayRootsInCurrentGroup / markGray (action list, Sweeping.cpp:2654),
  B becomes BlackAndGray, gray roots are pushed (so the marker is not
  drained and markGray does not early-return), and the queue finally
  processes: object marked, "drain" empties all markers, "yield"
  returns QueueYielded -> NotFinished -> assert fails.

Expected vs. observed

• Expected: a "yield" directive in the test mark queue ends the slice and
  marking resumes in the next slice (or, if treated as spurious, is
  ignored without crashing).
• Observed: debug assertion failure / SEGV (abort) at Sweeping.cpp:1336.

Reachability

enqueueMark, grayRoot, newGlobal, startgc, gcslice are all
available under --fuzzing-safe in debug builds (enqueueMark/grayRoot
are #ifdef DEBUG but in the fuzzing-safe TestingFunctions array).
Fuzzers exercise random mark-queue directive sequences (cf. the regression
test js/src/jit-test/tests/gc/bug-2042774.js, which randomly combines
set-color-gray/drain/yield), so this is fuzzer-reachable. Severity is
low (testing-only mechanism, debug-only assert), but it is a real
inconsistency in the markSynchronously contract: callers that ignore the
result paper over the queue yield; the one caller that asserts the old
contract crashes.

Comment 6

[Jon Coppeard (:jonco)]

This is an interaction with the test mark queue which is not present in release builds. Not security sensitive.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 2044836 - Remove assertion that isn't necessarily true when using the test marking queue r?sfink

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d5572ed6925c
https://hg.mozilla.org/integration/autoland/rev/1fc85ab28073
Remove assertion that isn't necessarily true when using the test marking queue r=sfink

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ecc0f592efb2
https://hg.mozilla.org/integration/autoland/rev/f944431c330c
Revert "Bug 2044836 - Remove assertion that isn't necessarily true when using the test marking queue r=sfink" for causing spidermonkey failures

Comment 10

[rperta]

Backed out for causing spidermonkey failures
Backout link
Push with failures
Failure log(s)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/394f5c0570d7
https://hg.mozilla.org/integration/autoland/rev/1fec05e95ae5
Remove assertion that isn't necessarily true when using the test marking queue r=sfink

Comment 12

[BugBot [:suhaib / :marco]]

Based on comment #3, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:jonco, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/1fec05e95ae5

Comment 14

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20260615211149-0b0f01b9eaad.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.