597871 - TM: "Assertion failure: cx->enumerators == obj,"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

for (a in [0, 0, 0, 0, 0, 0, 0, 0, 0]) {
    try { (function() {
            for each(l in [false, 0, 0, 0]) {}
            h
        })()
    } catch(e) {}
}

asserts js debug shell on TM changeset dfcf5611ce02 with -m and -j at Assertion failure: cx->enumerators == obj,

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   53120:1b55ec0c7aee
user:        David Anderson
date:        Tue Sep 07 22:52:15 2010 -0700
summary:     Fix various bugs in tracer integration (bug 593532, r=dmandelin).

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This bug is really complicated to explain, so I apologize for making someone review this. I think it's responsible for the tracer integration hangs because an exn-handler could iloop. I'll test if it fixes those tomorrow.

Explanation:

1. Let's say we start at frame 1, in the method JIT.

2. ... and suddenly we want to record a trace. So we call a C++ helper, RunTracer(), from inside X. This invokes the interpreter to record. While recording, the interpreter pushes frame 2.

2. Next the trace aborts. We would like to leave the interpreter and resume in the method JIT, but we can't. We're at frame 2, and the JIT'd code below expects frame 1. So we must keep executing and wait for the stack to unwind.

3. However, we don't have to interpret. We can start a new instance of the method JIT at frame 2. So we do that. Now the call stack looks like:
 0. Method JIT for Frame 2
 1. Execute-until-unwound()
 2. RunTracer()
 3. Method JIT for Frame 1

4. While executing frame 2, an exception is thrown. No exception handler is found, so the return value from EnterMethodJIT() is false, and cx->throwing is true. The call stack looks like:
 0. Execute-until-unwound()
 1. RunTracer()
 2. Method JIT for Frame 1

5. Now, to resume executing, we must deal with the exception.

This is what the patch fixes. Before, Frame 2 was not popped before finding an exception handler. This means we could iloop by never leaving an existing handler, or accidentally removing the wrong iterator.

With this patch, Frame 2 is popped.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Also worth noting that SwallowErrors() is supposed to return true if and only if it's safe to resume executing, and false to propagate an error up. Please double check that the handling is correct, because whatever I had there before didn't look right for the case of !cx->throwing.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

This should block beta 7, since those dupe'd bugs did.

Comment 8

[Jason Orendorff [:jorendorff]]

I'll review this tomorrow, noonish.

Comment 9

[Jason Orendorff [:jorendorff]]

Check me:

Q: How can SwallowErrors be called with !cx->throwing?

A: Only when there is an uncatchable error (a hook returns false without
setting a pending exception).

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_User_Guide#Uncatchable_errors

Q: Is it really safe for InlineReturn to be called when we're not
returning but error-unwinding?

A: Yeah, it's fine. That function does exactly what the interpreter's
'inline_return:' path does: exactly those things that we need to have
happen when unwinding, plus a few harmless bits like copying
fp->returnValue() to the caller's frame.

Setting the return value is useless, but harmless, if we're
error-unwinding. And it avoids a branch.

Q: Why do we need just InlineReturn for the first frame and
js_UnwindScope for subsequent frames?

A: The comment sort of explains this. It's not just partial loop
unrolling; this is actually important for correctness. js_UnwindScope is
for leaving all the lexical scopes associated with a frame--which the
interpreter or JaegerShot will already have done for the topmost frame
we're exiting. It updates sp but not pc, and therefore it is not
idempotent. We must not call it again for that frame; but we must call
it for all subsequent frames up to and including entryFrame.

Q: Is something like RemoveExcessFrames also used when we leave trace due
to a break or return statement?

A: No, because that stuff doesn't cause frames to unroll. Even something
like for{try{break}finally{}} does not trigger anything more difficult
than a JSOP_GOSUB followed by a GOTO.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 478195 [details] [diff] [review]
fix

>+    jsbytecode *pc = NULL;
>+    bool returnOK = InlineReturn(f, cx->throwing);

I'm about 75% sure this should be InlineReturn(f, false).

The argument to InlineReturn, I think, means "error or exception" not
"exception".

(Unless something has changed, there is actually no internal engine
state indicating that an uncatchable error has occurred; it is indicated
solely by false return values and cx->throwing being false.)

>         /* If there's an exception and a handler, set the pc and leave. */
>+        if (cx->throwing) {
>+            pc = FindExceptionHandler(cx);
>+            if (pc) {
>+                cx->regs->pc = pc;
>+                break;

Move the declaration of pc into this block. Set returnOK = true; before
the break here. At the end, just return returnOK.

In SwallowErrors:
>+        returnOK = bool(js_UnwindScope(cx, 0, returnOK || cx->throwing));
>+        returnOK &= InlineReturn(f, returnOK);

To me it looks like the interpreter just does = instead of &= here.

If I understand correctly, a call hook may squelch an error or exception
as the inline frame is removed. What happens then is that the frame
returns instead of an error falling out. It looks like a plain = here
would be correct. Would that be dangerous? We can always resume safely
after a call, right?

r=me with these points addressed. Sorry if I got it all wrong-end-up; this is new code for me.

Comment 11

[Jason Orendorff [:jorendorff]]

A few things I noticed while I was looking at this:

In jsinterp.cpp, we have:
>   error:
>     JS_ASSERT(cx->regs == &regs);
> #ifdef JS_TRACER
>     if (regs.fp->hasImacropc() && cx->throwing) {
>         // Handle other exceptions as if they came from the imacro-calling pc.
>         regs.pc = regs.fp->imacropc();
>         regs.fp->clearImacropc();
>         atoms = script->atomMap.vector;
>     }
> #endif

The word "other" in that comment is nonsense now that the hardcoded
imacro exception paths are gone.

There's a redundant declaration
> static bool
> InlineReturn(VMFrame &f, JSBool ok);
in InvokeHelpers.cpp.

In SwallowErrors:
>     /* Update the VMFrame before leaving. */
>     JS_ASSERT(&f.regs == cx->regs);
> 
>     JS_ASSERT_IF(!pc || !returnOK, cx->fp() == stopFp);
>     return pc && returnOK;

But the comment is wrong--nothing is being updated here.

RemoveExcessFrames does not seem like a very good name for that
function. "Remove" makes it sound like that's all it's doing. It looks
like its job is to start from the current interpreter state (which could
be practically anything, except that we are not throwing) and run until
we reach a certain stack frame. This is a lot like GDB's 'finish'
command; so maybe "FinishExcessFrames".

SwallowErrors does not seem like a very good name either. Maybe
"UnwindExcessFrames" or "HandleErrorInExcessFrames".

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Thanks, Jason, for the in-depth review. 

comment #9's Q&A looks good.
comment #10 - yes, InlineReturn should be passed false. Thanks.
comment #11 - Took care of these nits, and used the better names.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/63066ec9dd8d

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Backed out due to mochitest failures.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Take two: http://hg.mozilla.org/tracemonkey/rev/fcf6fd216b34

> In SwallowErrors:
> >+        returnOK = bool(js_UnwindScope(cx, 0, returnOK || cx->throwing));
> >+        returnOK &= InlineReturn(f, returnOK);
> 
> To me it looks like the interpreter just does = instead of &= here.

That was okay, but the &= was needed on js_UnwindScope, like the interpreter.

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/fcf6fd216b34

Comment 18

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :(

  Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386
  New     : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5
  Change  : +10.475 (2.49% / z=2.847)
  Graph   : http://mzl.la/bZFaB3

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 19

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386
  New     : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5
  Change  : -112.809 (-5.6% / z=2.787)
  Graph   : http://mzl.la/9gFu4n

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 20

[Mozilla RelEng Bot]

A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386
  New     : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5
  Change  : -11.226 (-8.8% / z=2.659)
  Graph   : http://mzl.la/bZu5UP

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 21

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :(

  Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386
  New     : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5
  Change  : -109.155 (-5.34% / z=2.197)
  Graph   : http://mzl.la/b0dlou

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 22

[Mozilla RelEng Bot]

A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :(

  Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386
  New     : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5
  Change  : -11.749 (-9.06% / z=2.866)
  Graph   : http://mzl.la/avZij4

The regression occurred from changesets in the following range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5

The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard;
please remove it only once you have confirmed this bug is not the cause
of the regression.

Comment 24

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/jaeger/bug597871.js.