Closed
Bug 1148917
Opened 7 years ago
Closed 7 years ago
Crash [@ js::FrameIter::operator++] or Hit MOZ_CRASH(Unexpected state) at js/src/vm/Stack.cpp:719 with Debugger
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: decoder, Unassigned)
Details
(4 keywords, Whiteboard: [jsbugmon:ignore])
Crash Data
Attachments
(1 file, 1 obsolete file)
19.61 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 385840329d91 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --no-threads): var g = newGlobal(); g.eval('function f(a) { evaluate("f(" + " - 1);", {newContext: true}); }'); var dbg = new Debugger(g); var frames = []; dbg.onEnterFrame = function (frame) { frames.push(frame); for (var f of frames) f.eval('a').return }; g.f(); Backtrace: Program received signal SIGSEGV, Segmentation fault. js::FrameIter::operator++ (this=this@entry=0x7fffffeff178) at js/src/vm/Stack.cpp:719 #0 js::FrameIter::operator++ (this=this@entry=0x7fffffeff178) at js/src/vm/Stack.cpp:719 #1 0x0000000000599590 in operator++ (this=0x7fffffeff178) at js/src/vm/Stack.h:1781 #2 DebuggerFrame_eval (cx=0x7ffff69770f0, argc=<optimized out>, vp=<optimized out>) at js/src/vm/Debugger.cpp:6325 #3 0x00007ffff7ff1dce in ?? () #4 0x00007fffffeff9a8 in ?? () #5 0x00007fffffeff8d8 in ?? () #6 0x00007ffff7e59060 in ?? () #7 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7fffffeff178 140737487303032 rcx 0x7ffff5364031 140737307361329 rdx 0x0 0 rsi 0x20 32 rdi 0x7fffffeff178 140737487303032 rbp 0x7ffff69770f0 140737330508016 rsp 0x7fffffeff0f0 140737487302896 r8 0xd 13 r9 0x43c 1084 r10 0xd 13 r11 0x7ffff693c160 140737330266464 r12 0x7ffff5364239 140737307361849 r13 0x7fffffeff140 140737487302976 r14 0x7fffffeff170 140737487303024 r15 0x1 1 rip 0x621b6c <js::FrameIter::operator++()+108> => 0x621b6c <js::FrameIter::operator++()+108>: movl $0x2cf,0x0 0x621b77 <js::FrameIter::operator++()+119>: callq 0x421e20 <abort@plt> The test is intermittent despite the use of --no-threads, you might need to run it multiple times to reproduce.
Reporter | ||
Comment 1•7 years ago
|
||
Still hitting this, putting a needinfo on jimb and shu because it's related to Debugger.
Flags: needinfo?(shu)
Flags: needinfo?(jimb)
Comment 2•7 years ago
|
||
Currently, FrameIter unconditionally skips debugger eval frames by skipping everything until eifPrev, if eifPrev exists. This means that contrary to Debugger spec, D.Fs for debugger eval frames are incorrectly settled on the nearest non-debugger eval frame.
Attachment #8593671 -
Flags: review?(jimb)
Updated•7 years ago
|
Flags: needinfo?(shu)
Flags: needinfo?(jimb)
Comment 3•7 years ago
|
||
I have no idea why the fuzz test should be intermittent, but it crashes every time with --baseline-eager.
Comment 4•7 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #2) > Currently, FrameIter unconditionally skips debugger eval frames by skipping > everything until eifPrev, if eifPrev exists. Hm, FrameIter shouldn't skip eval frames, it skips the frames (if any) between the eval frame and eifPrev. What's causing the crash?
Comment 5•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > (In reply to Shu-yu Guo [:shu] from comment #2) > > Currently, FrameIter unconditionally skips debugger eval frames by skipping > > everything until eifPrev, if eifPrev exists. > > Hm, FrameIter shouldn't skip eval frames, it skips the frames (if any) > between the eval frame and eifPrev. What's causing the crash? FrameIter skips frames between the eval frame and eifPrev *if* eifPrev itself happens to not be a debugger eval frame. In this fuzz test the frame that invokes a D.F.eval was itself a debugger eval frame: it's recurring via onEnterFrame on the D.F.eval call.
Comment 6•7 years ago
|
||
Comment on attachment 8593671 [details] [diff] [review] Add a new option to FrameIter that allows stopping at debugger eval frames. Review of attachment 8593671 [details] [diff] [review]: ----------------------------------------------------------------- The code looks fine, but the ergonomics are bad. ::: js/src/vm/Stack.h @@ +1587,5 @@ > { > public: > enum SavedOption { STOP_AT_SAVED, GO_THROUGH_SAVED }; > enum ContextOption { CURRENT_CONTEXT, ALL_CONTEXTS }; > + enum DebuggerEvalOption { STOP_AT_DEBUGGER_EVAL, GO_THROUGH_DEBUGGER_EVAL }; But, this doesn't make FrameIter::operator++ "stop at debugger eval" frames at all; it continues iterating right through debugger eval frames. You mean something like IGNORE_DEBUGGER_EVAL_PREV_LINK, right? STOP_AT_SAVED, for example, does indeed terminate iteration. So something named STOP_AT_DEBUGGER_EVAL should stop iteration too. But it doesn't. @@ +1617,5 @@ > Data(const Data& other); > }; > > MOZ_IMPLICIT FrameIter(JSContext* cx, SavedOption = STOP_AT_SAVED); > + FrameIter(JSContext* cx, ContextOption, SavedOption, DebuggerEvalOption); Why not use a default argument value here? @@ +1893,5 @@ > { > public: > explicit AllFramesIter(JSContext* cx) > + : ScriptFrameIter(cx, ScriptFrameIter::ALL_CONTEXTS, ScriptFrameIter::GO_THROUGH_SAVED, > + ScriptFrameIter::STOP_AT_DEBUGGER_EVAL) Here the naming is especially perverse: it suggests that AllFramesIter is the one subclass of ScriptFrameIter that stops at debugger eval frames. That's not true; it *doesn't skip* those frames, which is consistent with the class's name.
Attachment #8593671 -
Flags: review?(jimb) → feedback+
Comment 7•7 years ago
|
||
(In reply to Jim Blandy :jimb from comment #6) > Comment on attachment 8593671 [details] [diff] [review] > Add a new option to FrameIter that allows stopping at debugger eval frames. > > Review of attachment 8593671 [details] [diff] [review]: > ----------------------------------------------------------------- > > The code looks fine, but the ergonomics are bad. > > ::: js/src/vm/Stack.h > @@ +1587,5 @@ > > { > > public: > > enum SavedOption { STOP_AT_SAVED, GO_THROUGH_SAVED }; > > enum ContextOption { CURRENT_CONTEXT, ALL_CONTEXTS }; > > + enum DebuggerEvalOption { STOP_AT_DEBUGGER_EVAL, GO_THROUGH_DEBUGGER_EVAL }; > > But, this doesn't make FrameIter::operator++ "stop at debugger eval" frames > at all; it continues iterating right through debugger eval frames. You mean > something like IGNORE_DEBUGGER_EVAL_PREV_LINK, right? > > STOP_AT_SAVED, for example, does indeed terminate iteration. So something > named STOP_AT_DEBUGGER_EVAL should stop iteration too. But it doesn't. I was using STOP in the sense of what the iter stops for after each operator++(). Indeed, the other sense of STOP is termination, so this is inconsistent here. I think IGNORE_DEBUGGER_EVAL_PREV_LINK is better abbreviated just IGNORE_DEBUGGER_EVAL or NO_IGNORE_DEBUGGER_EVAL. Ignoring eifPrev has the effect of operator++() not ignoring debugger eval frames.
Comment 8•7 years ago
|
||
A better attempt at explaining this bug. It's kind of convoluted, but I'll try my best. Consider this revised fuzz test that limits the stack depth to 3: var g = newGlobal(); g.eval('function f(a) { evaluate("f(" + " - 1);", {newContext: true}); }'); var dbg = new Debugger(g); var frames = []; dbg.onEnterFrame = function (frame) { if (frames.length == 3) return; frames.push(frame); for (var f of frames) f.eval('a').return }; g.f(); Let us consider the evolution of |frames| and the execution stack. time frames stack ---- ------ ----- 1st onEnterFrame S S 2nd onEnterFrame S, A S > A 3rd onEnterFrame S, A, B S > A > B Consider the frames (not all of them, just the ones we care about): frame debugger eval? eifPrev ----- -------------- ------- S no null A yes S B yes S Note that onEnterFrame pushes the debugger eval frames A and B in the context of S, because of the |for (var f of frames)| loop in onEnterFrame. So both A and B's eifPrev are S. Recall the optimization we did a while back where if a D.F a FrameIter is constructed lazily. This lazy construction works by walking the stack until we hit cached AbstractFramePtr. When onEnterFrame returns early on |frames.length == 3|, we unwind to B's onEnterFrame and now attempt to push a new debugger eval frame in the context of A. When we call D.F.p.eval on the D.F reflection of A, we need the FrameIter for A. The iter starts off settled on the youngest frame, B. But since B's eifPrev is S, the iter will never settle on A, the frame we're looking for. This is what causes the assertion. What the patch intends to do is to never look at eifPrev if we're stackwalking for the sake of constructing a FrameIter from a saved AbstractFramePtr inside a D.F. Does this make more sense now?
Comment 9•7 years ago
|
||
Attachment #8593671 -
Attachment is obsolete: true
Attachment #8598300 -
Flags: review?(jimb)
Comment 10•7 years ago
|
||
Comment on attachment 8598300 [details] [diff] [review] Add a new option to FrameIter that allows stopping at debugger eval frames. Review of attachment 8598300 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Just some nits: ::: js/src/vm/Debugger.cpp @@ +1878,5 @@ > } > > for (ScriptFrameIter iter(cx, ScriptFrameIter::ALL_CONTEXTS, > + ScriptFrameIter::GO_THROUGH_SAVED, > + ScriptFrameIter::DONT_IGNORE_DEBUGGER_EVAL_PREV_LINK); This is the default, and can be omitted, right? I think there's no change needed here. ::: js/src/vm/SavedStacks.cpp @@ +802,5 @@ > return true; > } > > + FrameIter iter(cx, FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED, > + FrameIter::DONT_IGNORE_DEBUGGER_EVAL_PREV_LINK); This site needn't be changed either, right? ::: js/src/vm/Stack.h @@ +1586,5 @@ > public: > enum SavedOption { STOP_AT_SAVED, GO_THROUGH_SAVED }; > enum ContextOption { CURRENT_CONTEXT, ALL_CONTEXTS }; > + enum DebuggerEvalOption { DONT_IGNORE_DEBUGGER_EVAL_PREV_LINK, > + IGNORE_DEBUGGER_EVAL_PREV_LINK }; If you'll permit me a little bikeshed here: how about FOLLOW_ instead of DONT_IGNORE_?
Attachment #8598300 -
Flags: review?(jimb) → review+
Comment 12•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1a183c3e7d8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•