1455599 - Assertion failure: !JS::CurrentThreadIsHeapMinorCollecting(), at js/src/vm/TypeInference.cpp:4191 with wasm

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision cc0d7de218cb (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions):

for (var i = 0; i < 2000000; ++i, WebAssembly.Instance) {
    var o = {};
    o = Object.create(o);
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000c99d90 in AssertGCStateForSweep (zone=<optimized out>) at js/src/vm/TypeInference.cpp:4191
#0  0x0000000000c99d90 in AssertGCStateForSweep (zone=<optimized out>) at js/src/vm/TypeInference.cpp:4191
#1  0x0000000000cb286c in js::ObjectGroup::sweep (this=this@entry=0x7ffff5889310, oom=<optimized out>, oom@entry=0x0) at js/src/vm/TypeInference.cpp:4332
#2  0x00000000004d6e0f in js::ObjectGroup::maybeSweep (oom=0x0, this=0x7ffff5889310) at js/src/vm/ObjectGroup-inl.h:27
#3  js::ObjectGroup::flags (this=0x7ffff5889310) at js/src/vm/ObjectGroup-inl.h:33
#4  js::ObjectGroup::unknownProperties (this=0x7ffff5889310) at js/src/vm/ObjectGroup-inl.h:68
#5  0x0000000000f2f5c1 in js::ObjectGroup::canPreTenure (this=0x7ffff5889310) at js/src/vm/ObjectGroup-inl.h:82
#6  js::Nursery::collect (this=0x7ffff5f1bd28, reason=reason@entry=JS::gcreason::FULL_CELL_PTR_BUFFER) at js/src/gc/Nursery.cpp:758
#7  0x0000000000ed6704 in js::gc::GCRuntime::minorGC (this=0x7ffff5f19700, reason=JS::gcreason::FULL_CELL_PTR_BUFFER, phase=<optimized out>) at js/src/gc/GC.cpp:7825
#8  0x0000000000ee8dbd in js::gc::GCRuntime::gcIfRequested (this=this@entry=0x7ffff5f19700) at js/src/gc/GC.cpp:7870
#9  0x0000000000eea4b8 in js::gc::GCRuntime::gcIfNeededAtAllocation (this=0x7ffff5f19700, cx=cx@entry=0x7ffff5f15000) at js/src/gc/Allocator.cpp:316
#10 0x0000000000f16628 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ffff5f15000, kind=<optimized out>) at js/src/gc/Allocator.cpp:271
#11 0x0000000000f1683f in js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff5f15000, kind=kind@entry=js::gc::AllocKind::OBJECT4_BACKGROUND, nDynamicSlots=nDynamicSlots@entry=0, heap=heap@entry=js::gc::DefaultHeap, clasp=clasp@entry=0x2071aa0 <js::PlainObject::class_>) at js/src/gc/Allocator.cpp:52
#12 0x0000000000aad52f in js::NativeObject::create (cx=0x7ffff5f15000, kind=<optimized out>, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/vm/NativeObject-inl.h:539
#13 0x0000000000be5463 in NewObject (cx=<optimized out>, group=..., kind=<optimized out>, newKind=js::GenericObject, initialShapeFlags=<optimized out>) at js/src/vm/JSObject.cpp:731
#14 0x0000000000be583d in js::NewObjectWithGivenTaggedProto (cx=<optimized out>, cx@entry=0x7ffff5f15000, clasp=clasp@entry=0x2071aa0 <js::PlainObject::class_>, proto=..., allocKind=js::gc::AllocKind::OBJECT4_BACKGROUND, allocKind@entry=js::gc::AllocKind::OBJECT4, newKind=newKind@entry=js::GenericObject, initialShapeFlags=initialShapeFlags@entry=0) at js/src/vm/JSObject.cpp:792
#15 0x00000000005fb28f in js::NewObjectWithGivenProto<js::PlainObject> (newKind=js::GenericObject, allocKind=js::gc::AllocKind::OBJECT4, proto=..., cx=0x7ffff5f15000) at js/src/vm/JSObject-inl.h:678
#16 js::ObjectCreateImpl (cx=0x7ffff5f15000, proto=..., proto@entry=..., newKind=newKind@entry=js::GenericObject, group=..., group@entry=...) at js/src/builtin/Object.cpp:973
#17 0x000000000060bb5f in js::obj_create (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Object.cpp:1062
#18 0x000012c7a80b810f in ?? ()
[...]
#22 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff5889310	140737312756496
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbed0	140737488338640
rsp	0x7fffffffbed0	140737488338640
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff55ec000	140737310015488
r13	0x7ffff55ec000	140737310015488
r14	0x7ffff5889310	140737312756496
r15	0x7ffff5f15000	140737319620608
rip	0xc99d90 <AssertGCStateForSweep(JS::Zone*)+96>
=> 0xc99d90 <AssertGCStateForSweep(JS::Zone*)+96>:	movl   $0x0,0x0
   0xc99d9b <AssertGCStateForSweep(JS::Zone*)+107>:	ud2


Marking s-s because this is a GC assertion.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ebfd78786947
user:        Steve Fink
date:        Thu Apr 05 15:46:59 2018 -0700
summary:     Bug 1446693 - Include discardJitCode in AutoTraceSession for minor GC, r=jonco

This iteration took 279.587 seconds to run.

Comment 2

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

Steve, is bug 1446693 a likely regressor?

Comment 3

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

Yes it is. Also for bug 1454925. I was going to look at that today; I'll do it now.

Comment 4

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

Attachment: Redo bug 1446693, creating separate AutoTraceSessions instead of expanding one

I don't *think* there's any reason not to end a trace session and then restart it.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8969837 [details] [diff] [review]
Redo bug 1446693, creating separate AutoTraceSessions instead of expanding one

Review of attachment 8969837 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine.

I'm not clear why we don't end the session sooner though.  Is it required for anything after the pretenuring block?  Ideally we could hoist the shouldPretenure check and then dispense with the Maybe.

Comment 8

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

(In reply to Jon Coppeard (:jonco) from comment #6)
> Comment on attachment 8969837 [details] [diff] [review]
> Redo bug 1446693, creating separate AutoTraceSessions instead of expanding
> one
> 
> Review of attachment 8969837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine.
> 
> I'm not clear why we don't end the session sooner though.  Is it required
> for anything after the pretenuring block?  Ideally we could hoist the
> shouldPretenure check and then dispense with the Maybe.

Oops, there's no reason not to end it sooner. I think it was a holdover from bug 1446693, where it felt a little cleaner to wrap the entire minor GC instead of ending it slightly sooner -- but with this new version, it's just weird to have some extra stuff within it.

The shouldPretenure check is kind of a silly reason to use Maybe, although even if I changed that, then we could easily have a common case of not doing any string pretenuring even though shouldPretenure is true, when everything's already pretenured (so zone->allocNurseryStrings always returns false.)

For the current state and planned future changes, I think leaving it as a Maybe is simplest. Though I'm not sure whether it's better or worse than constructing the session within the innermost if; it's probably fine to be starting and stopping a lot.

Comment 10

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 26e53729a109).

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe59f468c0b74e5d44f81489bd129161b7bd2e9
https://hg.mozilla.org/mozilla-central/rev/cfe59f468c0b

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.