1292529 - Crash [@ js::gc::IsInsideNursery]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 465d150bc8be (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --thread-count=2 --disable-oom-functions):

See attachment.


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x080e3be3 in js::gc::IsInsideNursery (cell=0xf3b498c0) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/HeapAPI.h:342
#0  0x080e3be3 in js::gc::IsInsideNursery (cell=0xf3b498c0) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/HeapAPI.h:342
#1  0x086eb6e8 in js::InternalBarrierMethods<js::NativeObject*>::isInsideNursery (v=<optimized out>) at js/src/gc/Barrier.h:274
#2  js::GCPtr<js::NativeObject*>::~GCPtr (this=0xf54e3248, __in_chrg=<optimized out>) at js/src/gc/Barrier.h:455
#3  js::ForOfPIC::Chain::~Chain (this=0xf54e3240, __in_chrg=<optimized out>) at js/src/vm/PIC.h:177
#4  js::FreeOp::delete_<js::ForOfPIC::Chain> (this=<optimized out>, p=0xf54e3240) at js/src/vm/Runtime.h:145
#5  js::ForOfPIC::Chain::sweep (fop=<optimized out>, this=<optimized out>) at js/src/vm/PIC.cpp:273
#6  ForOfPIC_finalize (fop=0xffffc57c, obj=0xf3415060) at js/src/vm/PIC.cpp:280
#7  0x08577ab3 in js::Class::doFinalize (this=<optimized out>, obj=0xf3415060, fop=0xffffc57c) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Class.h:815
#8  JSObject::finalize (this=0xf3415060, fop=0xffffc57c) at js/src/jsobjinlines.h:87
#9  0x08577e21 in js::gc::Arena::finalize<JSObject> (this=0xf3415000, fop=0xffffc57c, thingKind=js::gc::AllocKind::OBJECT2, thingSize=32) at js/src/jsgc.cpp:450
#10 0x0855226e in FinalizeTypedArenas<JSObject> (fop=0xffffc57c, src=0xffffbb38, dest=..., thingKind=js::gc::AllocKind::OBJECT2, budget=..., keepArenas=js::gc::ArenaLists::KEEP_ARENAS) at js/src/jsgc.cpp:508
#11 0x0855242e in FinalizeArenas (fop=fop@entry=0xffffc57c, src=src@entry=0xffffbb38, dest=..., thingKind=<optimized out>, budget=..., keepArenas=js::gc::ArenaLists::KEEP_ARENAS) at js/src/jsgc.cpp:542
#12 0x08552662 in js::gc::ArenaLists::forceFinalizeNow (this=this@entry=0xf79218a0, fop=fop@entry=0xffffc57c, thingKind=thingKind@entry=js::gc::AllocKind::OBJECT2, empty=0xf7921b24, keepArenas=js::gc::ArenaLists::KEEP_ARENAS) at js/src/jsgc.cpp:2715
#13 0x08552889 in js::gc::ArenaLists::finalizeNow (thingKind=js::gc::AllocKind::OBJECT2, keepArenas=js::gc::ArenaLists::KEEP_ARENAS, empty=0xf7921b24, fop=0xffffc57c, this=0xf79218a0) at js/src/jsgc.cpp:2697
#14 js::gc::ArenaLists::queueForegroundObjectsForSweep (this=0xf79218a0, fop=0xffffc57c) at js/src/jsgc.cpp:2833
#15 0x08553fb0 in js::gc::GCRuntime::beginSweepingZoneGroup (this=0xf7953318, lock=...) at js/src/jsgc.cpp:5117
#16 0x08559c4e in js::gc::GCRuntime::beginSweepPhase (this=0xf7953318, destroyingRuntime=true, lock=...) at js/src/jsgc.cpp:5205
#17 0x0855d3b0 in js::gc::GCRuntime::incrementalCollectSlice (this=0xf7953318, budget=..., reason=JS::gcreason::DESTROY_RUNTIME, lock=...) at js/src/jsgc.cpp:5903
#18 0x0855e750 in js::gc::GCRuntime::gcCycle (this=0xf7953318, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6157
#19 0x0855ed4b in js::gc::GCRuntime::collect (this=0xf7953318, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6267
#20 0x0855f065 in js::gc::GCRuntime::gc (this=0xf7953318, gckind=GC_NORMAL, reason=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6332
#21 0x0873e56d in JSRuntime::destroyRuntime (this=0xf79530e8) at js/src/vm/Runtime.cpp:423
#22 0x084f4046 in JSContext::~JSContext (this=0xf7953000, __in_chrg=<optimized out>) at js/src/jscntxt.cpp:883
#23 0x084fbad9 in js_delete_poison<JSContext> (p=0xf7953000) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/32/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Utility.h:392
#24 js::DestroyContext (cx=0xf7953000) at js/src/jscntxt.cpp:137
#25 0x08079aac in main (argc=6, argv=0xffffcda4, envp=0xffffcdc0) at js/src/shell/js.cpp:7536
eax	0xf3bffff0	-205520912
ebx	0x8c22ff4	146943988
ecx	0x0	0
edx	0xf54e3258	-179424680
esi	0xf54e3240	-179424704
edi	0xf54e3248	-179424696
ebp	0xffffb968	4294949224
esp	0xffffb960	4294949216
eip	0x80e3be3 <js::gc::IsInsideNursery(js::gc::Cell const*)+35>
=> 0x80e3be3 <js::gc::IsInsideNursery(js::gc::Cell const*)+35>:	mov    (%eax),%edx
   0x80e3be5 <js::gc::IsInsideNursery(js::gc::Cell const*)+37>:	mov    %edx,%eax


Marking s-s because this is a GC crash. I cannot reduce the attached test without making it more intermittent, up to a level where reproduction rate goes beyond 0.1%.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate test from comment

Comment 3

[Fuzzing Team]

JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment

Comment 4

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

Intermittent GC issue -> setting needinfo? from GC gurus Terrence and Jon as a start.

Comment 5

[Jon Coppeard (:jonco)]

This is probably due to my recent changes in bug 1288780.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1292529-gcptr-crash

This is just a problem with the assertions in ~GCPtr, so I don't think it's s-s.

In the previous bug I tried to assert that the wrapper pointer is not pointing into the nursery if the destructor is called off the main thread.

Unfortunately the pointer can point to a GC thing that has already been freed, so calling IsInsideNursery (which checks the chunk trailer) can cause a crash like this.

The other option of calling Nursery::isInside is also out because we may not be on the main thread and so unable to access the nursery.

I think the best bet is to remove this part of the assertion and use the 'current thread is sweeping' flag to mark the one place we can destroy these off main thread.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8778937 [details] [diff] [review]
bug1292529-gcptr-crash

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

I like it!

::: js/src/vm/Runtime.h
@@ +1747,1 @@
>                  js_delete(const_cast<T*>(ptr));

Clever!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b81703d0d0e
Fix assertion used by GCPtr destructor to not assume referent is still allocated r=terrence

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b81703d0d0e

Comment 10

[Andrew Overholt [:overholt]]

Can/should we uplift this to 50 (seems it's also affected)?

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8778937 [details] [diff] [review]
bug1292529-gcptr-crash

Approval Request Comment
[Feature/regressing bug #]: Bug 1288780.
[User impact if declined]: Possible crashes in debug builds.
[Describe test coverage new/current, TreeHerder]: On m-c since 10th August.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jon, Andrew, I tried to look at the crash-stats for the associated crash signature and I wasn't able to get any crash data on it from Socorro. Has the crash signature changed somehow? I looked at the patch and since it is debug build only, do we need to uplift this to Aurora50 or let it ride the 51 train?

Comment 13

[Andrew Overholt [:overholt]]

I hadn't noticed the required build flags when asking if we should uplift this. I'll let Jon decide.

Comment 14

[Jon Coppeard (:jonco)]

We can uplift this is you think it will be useful to reduce the possibility of crashes when running tests on debug builds.  It won't affect our users, so personally I don't think it's necessary unless there's another patch we want to uplift that depends on this.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Jon Coppeard (:jonco) from comment #14)
> We can uplift this is you think it will be useful to reduce the possibility
> of crashes when running tests on debug builds.  It won't affect our users,
> so personally I don't think it's necessary unless there's another patch we
> want to uplift that depends on this.

Thanks Jon and Andrew. I think I will let this one ride the 51 train. Thanks!

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8778937 [details] [diff] [review]
bug1292529-gcptr-crash

Normally we uplift crash fixes but this patch doesn't seem so trivial/simple. Since there are no crash reports about this one in the wild or from crash-stats, I would prefer to let this one ride the 51 train.