The following testcase crashes on mozilla-central revision 3c26bef95d54 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off): setGCCallback({ action: "majorGC", }); oomAfterAllocations(50); Backtrace: Program terminated with signal SIGSEGV, Segmentation fault. #0 0x00000000008121c2 in mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back (this=<optimized out>) at ../../dist/include/mozilla/Vector.h:452 To enable execution of this file add add-auto-load-safe-path /home/ubuntu/mozilla-central/js/src/debug64/dist/bin/js-gdb.py line to your configuration file "/home/ubuntu/.gdbinit". To completely disable this security protection add set auto-load safe-path / line to your configuration file "/home/ubuntu/.gdbinit". For more information about this security protection see the "Auto-loading safe path" section in the GDB manual. E.g., run from the shell: info "(gdb)Auto-loading safe path" #0 0x00000000008121c2 in mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back (this=<optimized out>) at ../../dist/include/mozilla/Vector.h:452 #1 0x00000000007f618c in js::gcstats::Statistics::endSlice (this=0x7f067203d6d8) at js/src/gc/Statistics.cpp:956 #2 0x0000000000b477af in ~AutoGCSlice (this=0x7fffb74a4020, __in_chrg=<optimized out>) at js/src/gc/Statistics.h:341 #3 js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6148 #4 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #5 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #6 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #7 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #8 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #9 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #10 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #11 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #12 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #13 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #14 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #15 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #16 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #17 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #18 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #19 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #20 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #21 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #22 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #23 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #24 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #25 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #26 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #27 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #28 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #29 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #30 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #31 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #32 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #33 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #34 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #35 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:6159 #36 0x0000000000b48299 in gc (reason=JS::gcreason::API, gckind=GC_NORMAL, this=<optimized out>) at js/src/jsgc.cpp:6224 #37 JS::GCForReason (rt=rt@entry=0x7f0672037000, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::API) at js/src/jsgc.cpp:7056 #38 0x0000000000477191 in gcCallback::majorGC (rt=0x7f0672037000, status=<optimized out>, data=0x7f0672089200) at js/src/shell/js.cpp:1560 #39 0x0000000000b47853 in js::gc::GCRuntime::collect (this=this@entry=0x7f0672037350, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6159 #40 0x0000000000b479b0 in js::gc::GCRuntime::gc (this=this@entry=0x7f0672037350, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6224 #41 0x0000000000a8b5b3 in js::DestroyContext (cx=0x7f067201b330, mode=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:185 #42 0x0000000000a8b92e in JS_DestroyContext (cx=<optimized out>) at js/src/jsapi.cpp:738 #43 0x00000000004725f2 in DestroyContext (withGC=true, cx=0x7f067201b330) at js/src/shell/js.cpp:5698 #44 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6494 rax 0x0 0 rbx 0x7f067203d6d8 139665659385560 rcx 0x7f067236888d 139665662707853 rdx 0x0 0 rsi 0x7f067263d9d0 139665665677776 rdi 0x7f067263c1c0 139665665671616 rbp 0x7fffb74a3f40 140736268484416 rsp 0x7fffb74a3f40 140736268484416 r8 0x7f06736ad780 139665682913152 r9 0x6c697a6f6d2f6564 7811909647642617188 r10 0x7f0672639be0 139665665661920 r11 0x0 0 r12 0x7f067203d710 139665659385616 r13 0x0 0 r14 0x0 0 r15 0x0 0 rip 0x8121c2 <mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back()+66> => 0x8121c2 <mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back()+66>: movl $0x1c4,0x0 0x8121cd <mozilla::VectorBase<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy, js::Vector<js::gcstats::Statistics::SliceData, 8ul, js::SystemAllocPolicy> >::back()+77>: callq 0x4941f0 <abort()>

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/4f64a711181d user: Steve Fink date: Tue Jan 13 14:01:42 2015 -0800 summary: Bug 1117768 - Fix assertion in AutoStopVerifyingBarriers and add tests, r=terrence This iteration took 553.749 seconds to run.

So there is actually already a gcDepth counter in the Statistics object, for taking into account nested GCs. Let's just do the same thing for "aborted", making it a counter rather than a boolean. Also, I've realized that gcDepth wouldn't be incremented if aborted is set to true, but it still could be decremented later, leading to a false count. I made increment and decrement dependent upon aborted, so as to ensure it's more correct.

Comment on attachment 8670314 [details] [diff] [review] aborted.patch Review of attachment 8670314 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I don't think this is the right fix for the problem here. We need to stop the begin callback from running a GC (this will never happen in practice). > Also, I've realized that gcDepth wouldn't be incremented if aborted is set to true, but it still could be decremented later, leading to a false count. This sounds like a real problem however!

So something like this, instead?

Comment on attachment 8670386 [details] [diff] [review] gccallback.patch Review of attachment 8670386 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test for this. ::: js/src/jsgc.cpp @@ +6326,5 @@ > return; > > + // The GC callback shouldn't call collect() > + if (callingGCCallback) > + return; Can you move this to checkIfGCAllowedInCurrentState()? Also, comment needs a fullstop at the end. @@ +6351,5 @@ > poked = false; > bool wasReset = gcCycle(nonincrementalByAPI, budget, reason); > > if (!isIncrementalGCInProgress()) { > + AutoSetCallingGCCallback scgc(*this); I think this will fail as the cycle collector does trigger GC from here. We can just remove this line though.

Thanks for the review! However, the test case embedded in the patch fails if we don't put the RAII class in the second branch... The issue shows up if the gc callback is called in the BEGIN or in the END position. If the CC wants to call collect() at the END position, then the first patch looks more valid, what do you think? If not, feel free to take over this bug :)

After discussing it, Jon said IRL he'd take over the bug.

I'm sorry, I didn't understand this bug at first and what I told you about how to fix it was wrong. This patch is more like your original patch, but instead of making |aborted| a counter, we only clear it at the end of the outermost GC. I moved the increment/decrement of gcDepth so that it is always > 0 when we are inside a GC, and added an assert that it doesn't go negative which could happen previously if we git OOM and returned early from beginSlice().

Comment on attachment 8673026 [details] [diff] [review] bug1175755-stats-crash Review of attachment 8673026 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, that's simpler indeed. ::: js/src/jit-test/tests/gc/bug-1175755.js @@ +1,4 @@ > +setGCCallback({ > + action: "majorGC", > +}); > +oomAfterAllocations(50); Do we need to test for oomAfterAllocations, and wrap the code in a try/catch to ignore the OOM message?

(In reply to Benjamin Bouvier [:bbouvier] from comment #9) > Do we need to test for oomAfterAllocations and wrap the code in a try/catch to ignore the OOM message? Yes, indeed we do.