1723840 - Assertion failure: cellValue->color() >= std::min(mapColor, keyColor), at js/src/gc/WeakMap-inl.h:179

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210803-524aef2e3307 (--enable-debug build, run with --fuzzing-safe --ion-offthread-compile=off --blinterp-eager):

gcstress = 1;
Object.defineProperty(this, "", {
    value: eval
});
gczeal(11);
gczeal(6, 5);
const a = { b: 7 };
const c = [a, ""];
const d = [c];
new WeakMap(d);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555571a46b7 in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::markEntry(js::GCMarker*, js::HeapPtr<JSObject*>&, js::HeapPtr<JS::Value>&) ()
#1  0x00005555571a31df in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::markEntries(js::GCMarker*) ()
#2  0x00005555571a1fa4 in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::trace(JSTracer*) ()
#3  0x0000555557548dc3 in js::GCMarker::processMarkStackTop(js::SliceBudget&) ()
#4  0x0000555557549875 in js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#5  0x00005555574e7282 in js::gc::GCRuntime::drainMarkStack() ()
#6  0x00005555575a48a3 in js::gc::MarkingValidator::nonIncrementalMark(js::gc::AutoGCSession&) ()
#7  0x00005555574f0f42 in js::gc::GCRuntime::beginSweepPhase(JS::GCReason, js::gc::AutoGCSession&) ()
#8  0x00005555574f9b1f in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason, bool) ()
#9  0x00005555574fd04f in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#10 0x00005555574fe41b in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#11 0x0000555557503eee in js::gc::GCRuntime::runDebugGC() ()
#12 0x00005555574adcc7 in bool js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind) ()
#13 0x00005555574adb0a in JSObject* js::AllocateObject<(js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, JSClass const*, js::gc::AllocSite*) ()
#14 0x0000555556b9f0d1 in js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, js::gc::AllocSite*) ()
#15 0x0000555556c185ae in js::NewPlainObjectBaselineFallback(JSContext*, JS::Handle<js::Shape*>, js::gc::AllocKind, js::gc::AllocSite*) ()
#16 0x000033bf5882c07d in ?? ()
[...]
#50 0x0000000000000000 in ?? ()
rax	0x5555557fa4e7	93824995009767
rbx	0x0	0
rcx	0x555558120a70	93825038158448
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffaac0	140737488333504
rsp	0x7fffffffaa50	140737488333392
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x3ae9c4b00000	64775701659648
r13	0x7ffffffff000	140737488351232
r14	0x7ffff603c402	140737320829954
r15	0x7ffff603c450	140737320830032
rip	0x5555571a46b7 <js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::markEntry(js::GCMarker*, js::HeapPtr<JSObject*>&, js::HeapPtr<JS::Value>&)+1431>
=> 0x5555571a46b7 <_ZN2js7WeakMapINS_7HeapPtrIP8JSObjectEENS1_IN2JS5ValueEEEE9markEntryEPNS_8GCMarkerERS4_RS7_+1431>:	movl   $0xb3,0x0
   0x5555571a46c2 <_ZN2js7WeakMapINS_7HeapPtrIP8JSObjectEENS1_IN2JS5ValueEEEE9markEntryEPNS_8GCMarkerERS4_RS7_+1442>:	callq  0x555556b0473a <abort>

This is pretty frequent, marking as fuzzblocker. It is also pretty fragile for some reason (e.g. if you alter the name of the gcstress variable, it stops reproducing easily). Marking s-s because of that and GC involved in general.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Steve Fink [:sfink] [:s:]]

This looks like it's probably a result of the permanent atom changes, and I guess I'm not too clear on exactly how they're supposed to work.

What's happening here is that the WeakMap logic is getting to a place where it looks like it needs to mark a value, so it marks it, and then verifies that the resulting color is marked enough. The value in this case is a permanent atom.

So it first gets the current color of the value, which I would expect to be black, but it reads the mark bit directly and it is unset. Should it be set?

Then it does TraceEdge, which does checks like ShouldMark, which returns true. Should it be false? As part of that, IsOwnedByOtherRuntime returns false. Should it return true? When it actually goes to mark, it sees that it's a permanent atom and does nothing, which seems good.

Finally, it checks the color again, by looking at the mark bitmap. It's still unset, which is what triggers the assertion.

It looks like I may need to update js::gc::detail::GetEffectiveColor to match the decisions above. For a permanent atom, it looks like it's checking whether the runtime matches, and I expected it to mismatch. But I could be wrong. If that were to mismatch, then this whole series of events would be skipped at the beginning, because it would see the value's effective color as black and so not do any of this.

needinfo? jonco to answer the questions above. I probably ought to know the answers, given that I reviewed those changes.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)

So it first gets the current color of the value, which I would expect to be black, but it reads the mark bit directly and it is unset. Should it be set?

Permanent atoms should always be black. This seems like the root of the problem.

Then it does TraceEdge, which does checks like ShouldMark, which returns true. Should it be false? As part of that, IsOwnedByOtherRuntime returns false. Should it return true?

It can be either. The main runtime owns the atoms, but child (worker) runtimes do not.

The recent changes move arenas containing permanent atoms out of the arena lists and stop us marking them at the start of GC. I wonder if I didn't move all the necessary arenas.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #4)
Nope, the problem is that the incremental marking verifier clears all the mark bits, even for permanent atoms.

Not security sensitive.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1723840 - Don't unmark permanent atoms in the marking verifier r?sfink

This changes the marking verifier to only unmark arenas that are present in
each zone's arena lists, skipping the permentant atom arenas.

It also removes the unused ArenaLists::unmarkAll(), which doesn't work any more
because it doesn't handle all the places arenas may be stored in ArenasLists:

https://searchfox.org/mozilla-central/source/js/src/gc/GC-inl.h#44-48

Comment 7

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

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

Comment 8

[Bugmon [:jkratzer for issues]]

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

Start: ca9b5386b47d10cfa351da0de8705acf391a360d (20210802085148)
End: d3e0ca7e2145805b86596ce93afc4a0b4ed9b175 (20210802090128)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ca9b5386b47d10cfa351da0de8705acf391a360d&tochange=d3e0ca7e2145805b86596ce93afc4a0b4ed9b175

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66daa8395eb1
Don't unmark permanent atoms in the marking verifier r=sfink

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/66daa8395eb1

Comment 11

[Bugmon [:jkratzer for issues]]

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