The following testcase crashes on mozilla-central revision e15383656900 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions --baseline-eager --ion-eager --nursery-size=64): g = newGlobal(); dbg = Debugger(g); hits = 0; dbg.onNewScript = function () hits++; assertEq(g.eval("eval('2 + 3')"), 5); this.gczeal(hits,1); dbg = Debugger(g); g.h = function () { var env = dbg.getNewestFrame().environment; dbg = 0; assertThrowsInstanceOf; } g.eval("h()"); Backtrace: Program received signal SIGSEGV, Segmentation fault. js::Debugger::fromChildJSObject (obj=obj@entry=0xe5e5e5e5e5e5e5e5) at js/src/vm/Debugger.cpp:584 #0 js::Debugger::fromChildJSObject (obj=obj@entry=0xe5e5e5e5e5e5e5e5) at js/src/vm/Debugger.cpp:584 #1 0x00000000007f6bea in js::Debugger::slowPathOnLeaveFrame (cx=cx@entry=0x7ffff6907800, frame=..., pc=pc@entry=0x7ffff21845c6 ":", frameOk=frameOk@entry=false) at js/src/vm/Debugger.cpp:782 #2 0x00000000005aabe3 in onLeaveFrame (ok=false, pc=0x7ffff21845c6 ":", frame=..., cx=0x7ffff6907800) at js/src/vm/Debugger-inl.h:24 #3 HandleExceptionBaseline (pc=0x7ffff21845c6 ":", rfe=0x7fffffffbf40, frame=..., cx=0x7ffff6907800) at js/src/jit/JitFrames.cpp:691 #4 js::jit::HandleException (rfe=0x7fffffffbf40) at js/src/jit/JitFrames.cpp:837 #5 0x00007ffff7fe815d in ?? () #6 0x00007fffffffbfc8 in ?? () #7 0x00007fffffffbf40 in ?? () #8 0x0000000000000000 in ?? () rax 0x7fffffffc100 140737488339200 rbx 0x7fffffffb8e8 140737488337128 rcx 0x7fffffffb790 140737488336784 rdx 0x2 2 rsi 0x7fffffffb880 140737488337024 rdi 0xe5e5e5e5e5e5e5e5 -1880844493789993499 rbp 0x7fffffffb790 140737488336784 rsp 0x7fffffffb6e8 140737488336616 r8 0x20 32 r9 0x7ffff7e703a0 140737352500128 r10 0x1e 30 r11 0x3b 59 r12 0x7ffff6907800 140737330051072 r13 0x7fffffffb880 140737488337024 r14 0xe5e5e5e5e5e5e5e5 -1880844493789993499 r15 0xe5e5e5e5e5e5e5e5 -1880844493789993499 rip 0x7ddfc0 <js::Debugger::fromChildJSObject(JSObject*)> => 0x7ddfc0 <js::Debugger::fromChildJSObject(JSObject*)>: mov 0x8(%rdi),%rax 0x7ddfc4 <js::Debugger::fromChildJSObject(JSObject*)+4>: mov 0x10(%rax),%eax

another poison pattern, another bug for our gc folks

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/6efecddff450 user: Shu-yu Guo date: Thu Apr 02 17:28:02 2015 -0700 summary: Bug 1134198 - Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT. (r=jandem) This iteration took 143.709 seconds to run.

Shu-yu, is bug 1134198 a likely regressor?

I put a proposal for further simplification in bug 1254439.

(In reply to Jim Blandy :jimb from comment #5) > I put a proposal for further simplification in bug 1254439. It is a fine simplification, but I have no cycles for the rest of the week. I'll be happy to do it if we're fine with letting this fuzzbug sit unfixed until next week. Does anyone care?

Comment on attachment 8727658 [details] [diff] [review] Fix unsafe use of FrameRange in slowPathOnLeaveFrame. Review of attachment 8727658 [details] [diff] [review]: ----------------------------------------------------------------- You said you'd be happy to implement the suggestion I made in bug 1254439, which would further simplify this. Given that, and the questions I raise below, I think this patch should be revised. ::: js/src/vm/Debugger.cpp @@ +753,5 @@ > // This path can be hit via unwinding the stack due to over-recursion or > // OOM. In those cases, don't fire the frames' onPop handlers, because > // invoking JS will only trigger the same condition. See > // slowPathOnExceptionUnwind. > + bool callOnPop = !cx->isThrowingOverRecursed() && !cx->isThrowingOutOfMemory(); I don't understand why we need to split the original `if` into two, and place the call to resultToCompletion right in the middle of it. We should just move that to the top, and then keep the single `if` structure.

Forgot to address the slowPathOnLeaveFrame comment from the earlier review.

Comment on attachment 8733581 [details] [diff] [review] Remove FrameRange cray cray in favor of using GCVectors. Review of attachment 8733581 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Thanks for the rewrite. Also, I am contractually obliged to approve any patch with "cray cray" in the title. ::: js/src/vm/Debugger.cpp @@ +643,5 @@ > > auto frameMapsGuard = MakeScopeExit([&] { > // Clean up all Debugger.Frame instances. This call creates a fresh > + // DebuggerFrameVector, as one debugger's onPop handler could have > + // caused another debugger to create its own Debugger.Frame instance. I think the whole comment starting with "This call creates" can be deleted, since there's nothing interesting in scope at this point that it could conceivably re-use. @@ +668,4 @@ > } > > + if (frames.empty()) > + return frameOk; Consider the case where we are handling over-recursion or OOM. In the original code, if there are no frames, we return immediately. In this new code, if there are no frames, we call resultToCompletion to convert (cx, frameOk, frame.returnValue()) into (status, value), and then do a switch at the bottom to convert (status, value) back to its original form as (cx, frame return value, return bool). And then we call removeFromFrameMapsAndClearBreakpointsIn, which should do nothing. This change should be transparent, so I think it's okay. I just wanted to make sure that's your understanding as well. In the case where we're *not* handling over-recursion or OOM, we return after calling getDebuggerFrames, which seems equivalent to the original.

Shu says don't uplift. WONTFIX 47.