Closed Bug 1252464 Opened 8 years ago Closed 8 years ago

Crash [@ js::Debugger::fromChildJSObject] with Debugger

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: decoder, Assigned: shu)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files, 1 obsolete file)

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
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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?
Blocks: 1134198
Flags: needinfo?(shu)
Flags: needinfo?(shu)
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.
Attachment #8727658 - Flags: review?(jimb) → review-
Attachment #8733157 - Flags: review?(jimb)
Forgot to address the slowPathOnLeaveFrame comment from the earlier review.
Attachment #8733581 - Flags: review?(jimb)
Attachment #8733157 - Attachment is obsolete: true
Attachment #8733157 - Flags: review?(jimb)
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.
Attachment #8733581 - Flags: review?(jimb) → review+
Assignee: nobody → shu
https://hg.mozilla.org/mozilla-central/rev/27e149c2ecdc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Shu says don't uplift. WONTFIX 47.
You need to log in before you can comment on or make changes to this bug.