Crash [@ js::gc::IsInsideNursery]

RESOLVED FIXED in Firefox 51



JavaScript Engine
a year ago
a year ago


(Reporter: decoder, Assigned: jonco)


(Blocks: 1 bug, {crash, regression, testcase})

crash, regression, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed)


(Whiteboard: [jsbugmon:], crash signature)


(2 attachments)



a year ago
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.


 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

a year ago
Created attachment 8778237 [details]


a year ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 2

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


a year ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]

Comment 3

a year ago
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Intermittent GC issue -> setting needinfo? from GC gurus Terrence and Jon as a start.
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)

Comment 5

a year ago
This is probably due to my recent changes in bug 1288780.
Assignee: nobody → jcoppeard
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)

Comment 6

a year ago
Created attachment 8778937 [details] [diff] [review]

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.
Attachment #8778937 - Flags: review?(terrence)


a year ago
Group: javascript-core-security
Comment on attachment 8778937 [details] [diff] [review]

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

I like it!

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

Attachment #8778937 - Flags: review?(terrence) → review+

Comment 8

a year ago
Pushed by
Fix assertion used by GCPtr destructor to not assume referent is still allocated r=terrence

Comment 9

a year ago
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Can/should we uplift this to 50 (seems it's also affected)?
Flags: needinfo?(jcoppeard)


a year ago
Blocks: 1288780
Flags: needinfo?(jcoppeard)

Comment 11

a year ago
Comment on attachment 8778937 [details] [diff] [review]

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.
Attachment #8778937 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(overholt)
Flags: needinfo?(jcoppeard)
I hadn't noticed the required build flags when asking if we should uplift this. I'll let Jon decide.
Flags: needinfo?(overholt)

Comment 14

a year ago
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.
Flags: needinfo?(jcoppeard)
(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 on attachment 8778937 [details] [diff] [review]

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.
Attachment #8778937 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-


a year ago
status-firefox50: affected → wontfix
You need to log in before you can comment on or make changes to this bug.