1148917 - Crash [@ js::FrameIter::operator++] or Hit MOZ_CRASH(Unexpected state) at js/src/vm/Stack.cpp:719 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Still hitting this, putting a needinfo on jimb and shu because it's related to Debugger.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Add a new option to FrameIter that allows stopping at debugger eval frames.

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.

Comment 3

[Shu-yu Guo [:shu]]

I have no idea why the fuzz test should be intermittent, but it crashes every time with --baseline-eager.

Comment 4

[Jan de Mooij [:jandem] (away Nov 23-27)]

(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

[Shu-yu Guo [:shu]]

(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

[Jim Blandy :jimb]

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.

Comment 7

[Shu-yu Guo [:shu]]

(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

[Shu-yu Guo [:shu]]

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

[Shu-yu Guo [:shu]]

Attachment: Add a new option to FrameIter that allows stopping at debugger eval frames.

Comment 10

[Jim Blandy :jimb]

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_?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a183c3e7d8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1a183c3e7d8