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,

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).

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.

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.

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

I'll review this tomorrow, noonish.

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 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.

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".

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.

Backed out due to mochitest failures.

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.

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.

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.

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.

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.

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.

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