1791363 - Assertion failure: !hasBlackEntries(), at gc/Marking.cpp:2039

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

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

enqueueMark('set-color-gray');
enqueueMark(newGlobal());
enqueueMark('set-color-black');
enqueueMark(newGlobal());
gcparam("markStackLimit", 1);
gczeal(8, 1);
throw 1;

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555576141a2 in js::GCMarker::setMarkColor(js::gc::MarkColor) ()
#1  0x0000555557616d1d in js::GCMarker::markDelayedChildren(js::gc::Arena*, js::gc::MarkColor) ()
#2  0x0000555557617269 in js::GCMarker::processDelayedMarkingList(js::gc::MarkColor, js::SliceBudget&) ()
#3  0x000055555761470f in js::GCMarker::markAllDelayedChildren(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#4  0x0000555557613e76 in js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#5  0x00005555575d182f in js::gc::GCRuntime::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#6  0x0000555557660909 in js::gc::GCRuntime::markGray(JS::GCContext*, js::SliceBudget&) ()
#7  0x0000555557683141 in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) ()
#8  0x0000555557678e63 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) ()
#9  0x000055555766848c in js::gc::GCRuntime::performSweepActions(js::SliceBudget&) ()
#10 0x00005555575d4d7a in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, JS::GCReason, bool) ()
#11 0x00005555575d80d4 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, JS::GCReason) ()
#12 0x00005555575d9286 in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, JS::GCReason) ()
#13 0x00005555575dc189 in js::gc::GCRuntime::runDebugGC() ()
#14 0x00005555575a3b17 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) ()
#15 0x00005555575a5c0d in js::gc::TenuredCell* js::gc::detail::AllocateTenuredImpl<(js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long) ()
#16 0x00005555570a4638 in js::SharedShape::getInitialShape(JSContext*, JSClass const*, JS::Realm*, js::TaggedProto, unsigned long, js::EnumFlags<js::ObjectFlag>) ()
#17 0x0000555556f58623 in NewObject(JSContext*, JSClass const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind) ()
#18 0x0000555556ed28ef in js::GlobalObject::createBlankPrototype(JSContext*, JS::Handle<js::GlobalObject*>, JSClass const*) ()
#19 0x0000555556ecf17c in js::GlobalObject::resolveConstructor(JSContext*, JS::Handle<js::GlobalObject*>, JSProtoKey, js::GlobalObject::IfClassIsDisabled) ()
#20 0x0000555557073c72 in js::GlobalObject::getOrCreateSavedFramePrototype(JSContext*, JS::Handle<js::GlobalObject*>) ()
#21 0x00005555570738ee in js::SavedFrame::create(JSContext*) ()
#22 0x000055555707caa5 in js::SavedStacks::createFrameFromLookup(JSContext*, JS::Handle<js::SavedFrame::Lookup>) ()
#23 0x000055555707c51e in js::SavedStacks::getOrCreateSavedFrame(JSContext*, JS::Handle<js::SavedFrame::Lookup>) ()
#24 0x0000555557079949 in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#25 0x0000555557078801 in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#26 0x000055555729b645 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#27 0x00005555572a056e in js::CaptureStack(JSContext*, JS::MutableHandle<JSObject*>) ()
#28 0x0000555556f21c12 in JSContext::setPendingException(JS::Handle<JS::Value>, js::ShouldCaptureStack) ()
#29 0x0000555556d4c19b in js::ThrowOperation(JSContext*, JS::Handle<JS::Value>) ()
#30 0x0000555556d4307d in Interpret(JSContext*, js::RunState&) ()
[...]
#39 0x0000555556b9f654 in main ()
rax	0x555555887d51	93824995589457
rbx	0x7ffff6019328	140737320686376
rcx	0x5555582cdae8	93825039915752
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffaaa0	140737488333472
rsp	0x7fffffffaa80	140737488333440
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99800	140737353717760
r10	0x0	0
r11	0x0	0
r12	0x1	1
r13	0x2	2
r14	0x1	1
r15	0x1	1
rip	0x5555576141a2 <js::GCMarker::setMarkColor(js::gc::MarkColor)+354>
=> 0x5555576141a2 <_ZN2js8GCMarker12setMarkColorENS_2gc9MarkColorE+354>:	movl   $0x7f7,0x0
   0x5555576141ad <_ZN2js8GCMarker12setMarkColorENS_2gc9MarkColorE+365>:	callq  0x555556c349f4 <abort>

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20220919105141-d42e0ca4bb3e.
The bug appears to have been introduced in the following build range:

Start: a27f2f698323860d680f0d042d0a833411935057 (20220202172237)
End: 26438f963a5ffab579ede7738679bc2ae34102e2 (20220202173131)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a27f2f698323860d680f0d042d0a833411935057&tochange=26438f963a5ffab579ede7738679bc2ae34102e2

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1791363 - Process newly added marking work after performing delayed marking r?sfink

Delayed marking may push more (normal, non-delayed) marking work. We need to do
this before switch mark color from black to gray since we cannot add gray
marking work while there is black marking work on the stack.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1791363 - Rename GCMarker::color to avoid confusion with local variables r?sfink

It's pretty confusing that this name is sometimes used for local variables and
sometimes refers to the current mark color.

Depends on D157733

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is there a user-facing impact from this bug?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/882c2a4dde59
Process newly added marking work after performing delayed marking r=sfink
https://hg.mozilla.org/integration/autoland/rev/c0fe58fee455
Rename GCMarker::color to avoid confusion with local variables r=sfink

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/882c2a4dde59
https://hg.mozilla.org/mozilla-central/rev/c0fe58fee455

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
It's possible that this problem could cause a crash if we hit OOM expanding the mark stack during gray marking and then added new black marking work via a barrier. As such I'll mark this bug as sec-moderate as I think this would be difficult to arrange.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox106 to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #11)
I don't think this is likely to be causing problems for users so let's let this ride the trains.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Generally we uplift sec-moderates unless there's a strong reason not to, though :). These patches graft cleanly to both Beta & ESR102, FWIW.

Comment 14

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220921214338-7c0a787fe65a.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 9295430 [details]
Bug 1791363 - Process newly added marking work after performing delayed marking r?sfink

Beta/Release Uplift Approval Request

• User impact if declined: Possible crash / security vulnerability following OOM condition.
• 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): This is a straightforward change and has baked on central for over a week.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9295430 [details]
Bug 1791363 - Process newly added marking work after performing delayed marking r?sfink

Approved for 106.0b7, thanks.

Comment 17

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/146647978a94
https://hg.mozilla.org/releases/mozilla-beta/rev/88b817c0fa5a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9295430 [details]
Bug 1791363 - Process newly added marking work after performing delayed marking r?sfink

Approved for 102.4esr also.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/7d6ec2f15021
https://hg.mozilla.org/releases/mozilla-esr102/rev/aed0e9bb26e0