1597206 - Assertion failure: tag_ == TracerKindTag::WeakMarking, at js/src/gc/Marking.cpp:2618 with OOM

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 4def8673359e (build with --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

enableShellAllocationMetadataBuilder(function() {});
try {
    for (var i73 = 0; i73 < 10; ++i73) {
        var f38 = function() {
            function g99() {
                x48++;
            }
            g99();
        }
        f38();
    }
} catch (e44) {}
oomAfterAllocations(5)
gc();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::GCMarker::leaveWeakMarkingMode (this=this@entry=0x7ffff5f2a788) at js/src/gc/Marking.cpp:2616
#1  0x0000555555e969a1 in js::GCMarker::abortLinearWeakMarking (this=0x7ffff5f2a788) at js/src/gc/GCMarker.h:290
#2  js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::addWeakEntry (marker=marker@entry=0x7ffff5f2a788, key=<optimized out>, markable=...) at js/src/gc/WeakMap-inl.h:214
#3  0x0000555555e96b1f in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::markEntries (this=0x7ffff4ddd580, marker=0x7ffff5f2a788) at js/src/gc/WeakMap-inl.h:245
#4  0x0000555555e92b45 in js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value> >::trace (this=0x7ffff4ddd580, trc=0x7ffff5f2a788) at js/src/gc/WeakMap-inl.h:175
#5  0x0000555555d05102 in js::ObjectRealm::trace (this=0x7ffff471d868, trc=0x7ffff5f2a788) at js/src/vm/Realm.cpp:284
#6  0x0000555555d05197 in JS::Realm::traceRoots (this=<optimized out>, trc=<optimized out>, traceOrMark=<optimized out>) at js/src/vm/Realm.cpp:323
#7  0x00005555561e63b9 in js::gc::GCRuntime::traceRuntimeCommon (this=this@entry=0x7ffff5f29700, trc=trc@entry=0x7ffff5f2a788, traceOrMark=traceOrMark@entry=js::gc::GCRuntime::MarkRuntime) at js/src/gc/RootMarking.cpp:398
#8  0x00005555561e67e8 in js::gc::GCRuntime::traceRuntimeForMajorGC (this=this@entry=0x7ffff5f29700, trc=trc@entry=0x7ffff5f2a788, session=...) at js/src/gc/RootMarking.cpp:290
#9  0x0000555556188202 in js::gc::GCRuntime::beginMarkPhase (this=0x7ffff5f29700, reason=<optimized out>, session=...) at js/src/gc/GC.cpp:4060
#10 0x0000555556195108 in js::gc::GCRuntime::incrementalSlice (this=this@entry=0x7ffff5f29700, budget=..., gckind=..., reason=reason@entry=JS::GCReason::API, session=...) at js/src/gc/GC.cpp:6758
#11 0x0000555556195cce in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f29700, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., gckind=..., reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:7243
#12 0x000055555619629e in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f29700, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., gckindArg=..., reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:7428
#13 0x00005555561968b5 in js::gc::GCRuntime::gc (this=0x7ffff5f29700, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:7510
#14 0x0000555556196a9f in JS::NonIncrementalGC (cx=cx@entry=0x7ffff5f27000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::GCReason::API) at js/src/gc/GC.cpp:8344
#15 0x0000555555e60ee9 in GC (cx=cx@entry=0x7ffff5f27000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:509
#16 0x00005555559ce6f0 in CallJSNative (cx=0x7ffff5f27000, native=native@entry=0x555555e60e60 <GC(JSContext*, unsigned int, JS::Value*)>, reason=reason@entry=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:456
[...]
#29 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11535
rax	0x5555580befa0	93825037758368
rbx	0x7ffff5f2a788	140737319708552
rcx	0x555556f60a30	93825019546160
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffba80	140737488337536
rsp	0x7fffffffba10	140737488337424
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x8	8
r13	0x7ffff5f2a788	140737319708552
r14	0x7fffffffbb10	140737488337680
r15	0x7ffff470e988	140737294428552
rip	0x5555561dce80 <js::GCMarker::leaveWeakMarkingMode()+592>
=> 0x5555561dce80 <js::GCMarker::leaveWeakMarkingMode()+592>:	movl   $0x0,0x0
   0x5555561dce8b <js::GCMarker::leaveWeakMarkingMode()+603>:	ud2

Marking s-s until investigated because this is GC related.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jon Coppeard (:jonco)]

Possibly related to recent weak map marking changes.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/5decb743461a
user: Steve Fink
date: Fri Nov 15 16:40:44 2019 +0000
summary: Bug 1593399 - Rework how mark colors are handled in weakmap marking r=jonco

Guessing related to bug 1593399?

Comment 4

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

There are two problems here. First, it is possible to OOM during regular marking (before entering weak marking mode) when updating the weak key table, and so we need to be able to cancel the weak marking stuff (and fall back to iterative marking) at any time.

Second, we shouldn't be doing anything with the weak key table during regular marking in the first place! I must have messed that up when I merged things together for the altered color handling in bug 1593399. So I will undo that for now -- but I'll still fix abortWeakMarking() so it can be called at any time, because I'll need that for incremental weak marking anyway.

Comment 5

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

Attachment: Bug 1597206 - Refactor enterWeakMarkingMode slightly

Comment 6

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

Attachment: Bug 1597206 - Do not populate weak keys during RegularMarking yet, and allow aborting weak marking during RegularMarking

Comment 7

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

Attachment: Bug 1597206 - Refactor GCMarker state management

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/a6b2ec4c398101c0f363ec654ed499d1c75f4d5f
https://hg.mozilla.org/integration/autoland/rev/745bab598970274b9b1dc050aef274c781e3f89d
https://hg.mozilla.org/integration/autoland/rev/9c728f162f0ae034639861cbd1cb44932a4bfeed
https://hg.mozilla.org/mozilla-central/rev/a6b2ec4c3981
https://hg.mozilla.org/mozilla-central/rev/745bab598970
https://hg.mozilla.org/mozilla-central/rev/9c728f162f0a

Patches apply cleanly on beta. Do you want to uplift?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is the testcase from #c0 something suitable for landing in-tree? Also, is there a user impact which justifies Beta backport consideration for Fx72?

Comment 10

[Andrew McCreight [:mccr8]]

This should have gotten a security rating before landing. I'm marking it sec-high as it is looks like it is a couple of GC issues, and thus it should have also gotten sec-approval before landing.

Comment 11

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-high regression

Comment 12

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Is the testcase from #c0 something suitable for landing in-tree? Also, is there a user impact which justifies Beta backport consideration for Fx72?

I tried to make a more reliable test case out of #c0 and failed. That test reproduces the problem across a number of different versions, but it depends on an exact allocation count. I hoped that oomTest would remove that dependency, but I could not get it to reproduce that way.

(In reply to Andrew McCreight [:mccr8] from comment #10)

This should have gotten a security rating before landing. I'm marking it sec-high as it is looks like it is a couple of GC issues, and thus it should have also gotten sec-approval before landing.

Oh, oops. I did explicitly read through the sec-approval rules before landing, but I somehow thought that this had made it into the 72 nightly (the regressing change landed in 72).

I would probably argue for sec-moderate. Although there are 2 issues fixed here, one of them should only result in a minor perf improvement. The other is only triggerable via an OOM at an unlikely point.

I have not requested backport yet because there is still a regression in bug 1596943 that I wanted to track down. This fix is probably fine to go in, but is unlikely to resolve that regression by itself. A further fix might require modifying the same code. If it would help to backport this, I'm fine with that. I don't think we'll be seeing any crashes in practice that this would fix, other than fuzz bugs. The bug this fixes would also be very hard to exploit (not just because of the OOM targeting problem.)

Comment 13

[Julien Cristau [:jcristau]]

Not tracking based on the sec-moderate re-rating. I'll leave the 72 status as affected for now though in case we end up uplifting.

Comment 14

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

Comment on attachment 9111307 [details]
Bug 1597206 - Refactor enterWeakMarkingMode slightly

Beta/Release Uplift Approval Request

• User impact if declined: Some misbehavior on OOM that might lead to UAF. Also, one of the changes here seems to have fixed a stability problem (bug 1597827) that would likely result in some number of crashes.
• 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): Although this is tricky code, the changes here mostly remove the possibility to encounter some tricky cases and at this point have been tested for quite a while in Nightly. (The code after these patches is much simpler as well.)
• String changes made/needed: none

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9111307 [details]
Bug 1597206 - Refactor enterWeakMarkingMode slightly

gc fix, approved for 72.0b11

Comment 16

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25a180f11f4f7cd3d8f68e426cd59a18c2381049
https://hg.mozilla.org/releases/mozilla-beta/rev/184cea66f949c94800c3f5053f115f27e5f900f4
https://hg.mozilla.org/releases/mozilla-beta/rev/3923275226cbca3bd34deda94f01ae8fdcc13d5e