The attached test case examines the frame's "offset" from an onPop callback. When run with --baseline-eager, it gets the wrong result -- it reports that the offset is the offset of the "debugger" statement. However, when run without --baseline-eager, it works. pokyo. ./js/src/js --baseline-eager /tmp/fail2.js debug = 7 pop = 7 uncaught exception: 23 pokyo. ./js/src/js /tmp/fail2.js debug = 7 pop = 21

Thanks for filing and the test case, Tom! 2 bugs here. 1) We only need to manually unwind all scopes during a DebugEpilogue via forced return due to exception handling. Unexceptional calls to DebugEpilogue don't need to do anything special. 2) When unwinding all scopes for a forced return, Baseline set the pc to the first pc instead of the last pc.

I think this approach interacts badly with the patch in bug 1013219. The basic issue is that it isn't always correct to report the last PC as the frame-popping PC. Consider this test case derived from errorcolumnblame.js: function test(f) { try { f(); } catch (e) { print("column: "+ e.columnNumber); dis(f); } } test(function() { //234567890123456789 throw new Error('a'); }); The output, with both patches installed, is: pokyo. ./obj-x86_64-unknown-linux-gnu/dist/bin/js /tmp/a.js column: 0 flags: LAMBDA loc op ----- -- main: 00000: getgname "Error" 00005: undefined 00006: string "a" 00011: new 1 00014: throw 00015: retrval Source notes: ofs line pc delta desc args ---- ---- ----- ------ -------- ------ 0: 10 0 [ 0] setline lineno 12 2: 12 0 [ 0] colspan 13 4: 12 11 [ 11] xdelta 5: 12 11 [ 0] colspan 6 7: 12 15 [ 4] newline The "retrval" is the last instruction -- but it isn't where the frame is popped. That happens at PC=14, the throw. The last instruction is never executed, so reporting it as the location seem incorrect. This isn't a problem with the current tree, because the retrval inherits the most recently emitted location. However, this is incorrect in other scenarios; see the associated bugs for the gory details. In the above the correct answer is "column: 19", not "0"; with a reporting offset of 14.

(In reply to Tom Tromey :tromey from comment #2) > I think this approach interacts badly with the patch in bug 1013219. > The basic issue is that it isn't always correct to report the last PC > as the frame-popping PC. The concern you raise is valid. We never fully specced at what point should onPop be called. The ideal, the one that you want, (and after discussion with Jim), is that onPop be called right before *any* unwinding happens, scope-wise or frame-wise. Currently, onPop tends to happen after scope unwinding, but before frame unwinding. This patch is indeed wrong if we want to shoot for that ideal. I'll look at how feasible that is.

Comment on attachment 8566241 [details] [diff] [review] Fix pc when calling DebugEpilogue in Baseline during exception handling. Cancelling since we might be redesigning.

(In reply to Shu-yu Guo [:shu] from comment #3) > (In reply to Tom Tromey :tromey from comment #2) > > I think this approach interacts badly with the patch in bug 1013219. > > The basic issue is that it isn't always correct to report the last PC > > as the frame-popping PC. > > The concern you raise is valid. We never fully specced at what point should > onPop be called. The ideal, the one that you want, (and after discussion > with Jim), is that onPop be called right before *any* unwinding happens, > scope-wise or frame-wise. Currently, onPop tends to happen after scope > unwinding, but before frame unwinding. > Oh, since I didn't make it clear. Why scope unwinding affects reporting of .offset is that a script's scope contour is keyed by PC. So we can't unwind scope and not change the .offset.

> This patch is indeed wrong if we want to shoot for that ideal. I'll look at > how feasible that is. Thank you. In retrospect I am not sure that the problem I found is really a problem added by this patch. But on the other hand it will block the other patches anyhow, so maybe it's best to try to fix it all at once.

Refactoring patch to pave the way for the rewrite.

Refactoring patch to pave the way for rewrite. Jan, I tried to think of a reason why it'd be preferable to check for wrapping errors every time we want to know if the pending exception is JS_GENERATOR_CLOSING and couldn't think of one. If the exception is not JS_GENERATOR_CLOSING, then the wrapping error will occur regardless of propagating through generator frames or not. If the exception is JS_GENERATOR_CLOSING, then wrapping is infallible since it's not a GC thing.

Comment on attachment 8567454 [details] [diff] [review] Refactor JS_GENERATOR_CLOSED checking. Review of attachment 8567454 [details] [diff] [review]: ----------------------------------------------------------------- Ah, much nicer!

This is to avoid extra logic to avoid calling onLeaveFrame multiple times. Seems logical to unset debuggee-ness once the Debugger doesn't track the frame anymore.

Comment on attachment 8567544 [details] [diff] [review] Update docs for new Debugger.Frame.onPop spec. Review of attachment 8567544 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/doc/Debugger/Debugger.Frame.md @@ +224,5 @@ > value. The function should return a [resumption value][rv] indicating > how execution should proceed. On newly created frames, this property's > value is `undefined`. > > + When this handle is called, its frame is settled on the location that "handler" I think you want to actually spell out what's going on, rather than use the verb "settled", which nothing in this file has ever used before. Perhaps: "When this handler is called, this frame's current execution location, as reflected in its `offset` and `environment` properties, is the operation which caused it to be unwound."

Comment on attachment 8567543 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT. Logic is wrong.

Comment on attachment 8567542 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter. Logic is wrong.

Comment on attachment 8567541 [details] [diff] [review] Make Debugger::onLeaveFrame unmark frames as debuggee. Review of attachment 8567541 [details] [diff] [review]: ----------------------------------------------------------------- I'm a little confused by the changes to the assertions. The intent there is to accommodate the transitional state after the call to onPop but before the frame is really removed, right? Debugger::inFrameMaps(f) is true if f has any Debugger.Frame instances referring to it. A frame running a script with breakpoints set must be a debuggee frame, even though it may not have any D.Fs. So this change weakens the assertion conditions too much, as it will permit us to call unsetIsDebuggee on frames that must be debuggees because they're running debuggee scripts, just because those frames have no D.Fs. I can't think of any better way to flag frames as "pop already reported to Debugger" than clearing their isDebuggee flags, though. Perhaps a separate bit, parallel to isDebuggee, on all three frame representations, with all the AbstractFramePtr dynamic dispatch gunk, whose only purpose is to provide a fast rejection in Debugger::onLeaveFrame? What a pain. If we do make this change, I would think the comments in jscompartment.h talking about when frames must be debuggees would need to be updated. Now it's not the case that a frame running a debuggee script must always be a debuggee frame; we have this special transient state. ::: js/src/vm/Debugger.cpp @@ +671,5 @@ > removeFromFrameMapsAndClearBreakpointsIn(cx, frame); > > + // Unset the frame as a debuggee, now that all its Debugger.Frame > + // instances are removed. This prevents onLeaveFrame from being called > + // multiple times (e.g., due to exception handling or forced return). I read your changes to Debugger::onLeaveFrame's assertions to mean that that function might indeed see frames that have had their debuggee flag cleared here previously. So it's not true that Debugger::onLeaveFrame isn't called multiple times on a single frame. Isn't it more accurate to say that Debugger::slowPathOnLeaveFrame doesn't get called multiple times?

(In reply to Jim Blandy :jimb from comment #18) > Comment on attachment 8567541 [details] [diff] [review] > Make Debugger::onLeaveFrame unmark frames as debuggee. > > Review of attachment 8567541 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm a little confused by the changes to the assertions. The intent there is > to accommodate the transitional state after the call to onPop but before the > frame is really removed, right? > > Debugger::inFrameMaps(f) is true if f has any Debugger.Frame instances > referring to it. A frame running a script with breakpoints set must be a > debuggee frame, even though it may not have any D.Fs. So this change weakens > the assertion conditions too much, as it will permit us to call > unsetIsDebuggee on frames that must be debuggees because they're running > debuggee scripts, just because those frames have no D.Fs. > Good catch, I didn't think about the breakpoint case. Hm... the weakening was needed for the first version of the patch. I wonder if it's possible to restructure to code such that the weakening of the assertion isn't necessary and makes this entire patch unneeded.

Do the slower, but much easier to reason about thing.

Thanks for the edits, Jim! Ever the wordsmith.

This rewrites the trynotes code, which was necessary to get more desirable behavior out of Debugger.Frame.onPop. This could use a careful review, aka, a Luke-quality review.

Mirrors the logic from the interpreter patch into Baseline.

Comment on attachment 8567453 [details] [diff] [review] Rename assertNotInFrameMaps to inFrameMaps. Review of attachment 8567453 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8567740 [details] [diff] [review] Update docs for new Debugger.Frame.onPop spec. Review of attachment 8567740 [details] [diff] [review]: ----------------------------------------------------------------- One little nit, but otherwise good. ::: js/src/doc/Debugger/Debugger.Frame.md @@ +226,5 @@ > value is `undefined`. > > + When this handler is called, this frame's current execution location, as > + reflected in its `offset` and `environment` properties, is the operation > + which caused it to be unwound. In frames that are returning or threw an I think it should read "...returning or throwing an exception..."

Comment on attachment 8567540 [details] [diff] [review] Update tests to reflect new specced behavior on Debugger.Frame.onPop. Review of attachment 8567540 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, in particular I think it is the logical consequence of the other patches.

Comment on attachment 8567742 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter. Review of attachment 8567742 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really familiar with the debugger invariants being maintained by this patch; probably better to have jimb to review here. One comment, though: ::: js/src/vm/Interpreter.h @@ +262,5 @@ > RootedScript script; /* TryNotIter is always stack allocated. */ > uint32_t pcOffset; > JSTryNote *tn, *tnEnd; > > + virtual uint32_t stackDepth() const = 0; Instead of this and the awkward requirement on settle() stated below, how about template <class StackDepthOp> class TryNoteIter { StackDepthOp getStackDepth_; TryNoteIter(StackDepthOp getStackDepth) : getStackDepth_(getStackDepth) {} .. where TryNoteIter calls "getStackDepth_()" when it wants the stack depth? ::: js/src/vm/Stack.cpp @@ +416,4 @@ > > /*****************************************************************************/ > > // Unlike the other methods of this calss, this method is defined here so that pre-existing: s/calss/class/

(In reply to Luke Wagner [:luke] from comment #28) > Comment on attachment 8567742 [details] [diff] [review] > Call Debugger::onPop at the point that caused the frame to pop before any > unwinding in the interpreter. > > Review of attachment 8567742 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not really familiar with the debugger invariants being maintained by > this patch; probably better to have jimb to review here. > Okay, very well. > Instead of this and the awkward requirement on settle() stated below, how > about > > template <class StackDepthOp> > class TryNoteIter { > StackDepthOp getStackDepth_; > TryNoteIter(StackDepthOp getStackDepth) : getStackDepth_(getStackDepth) {} > .. Good idea. I had tried CRTP and inheritance, this is probably simpler.

Switch to Jim on Luke's request.

Comment on attachment 8568909 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter. Review of attachment 8568909 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Interpreter.cpp @@ +983,5 @@ > js::UnwindScopeToTryPc(JSScript *script, JSTryNote *tn) > { > + jsbytecode *pc = script->main() + tn->start; > + if (tn->kind == JSTRY_CATCH || tn->kind == JSTRY_FINALLY) > + pc -= JSOP_TRY_LENGTH; Drive-by comment: MOZ_ASSERT(*pc == JSOP_TRY); here.

Comment on attachment 8568910 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT. Review of attachment 8568910 [details] [diff] [review]: ----------------------------------------------------------------- Nice refactoring but some issues below. ::: js/src/jit/JitFrames.cpp @@ +575,5 @@ > + for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) { > + JSTryNote *tn = *tni; > + > + switch (tn->kind) { > + case JSTRY_CATCH: { This case assumes cx->isExceptionPending() right? But UnwindIteratorForException in the JSTRY_ITER case can make that false I think. @@ +643,4 @@ > RootedScript script(cx, frame.baselineFrame()->script()); > > + if (cx->isExceptionPending()) { > + if(cx->compartment()->isDebuggee() && !cx->isClosingGenerator()) { Nit: add space after "if(" @@ +647,5 @@ > + switch (Debugger::onExceptionUnwind(cx, frame.baselineFrame())) { > + case JSTRAP_ERROR: > + // Uncatchable exception. > + MOZ_ASSERT(!cx->isExceptionPending()); > + break; ProcessTryNotes assumes cx->isExceptionPending(), AFAICS (it should assert that if true), so in this case we don't want the code below calling ProcessTryNotes. @@ +693,5 @@ > } > + > + if (script->hasTrynotes()) > + ProcessTryNotes(cx, frame, si.ref(), rfe, &pc); > + frameOk = HandleClosingGeneratorReturn(cx, frame.baselineFrame(), frameOk); Questions: (1) ProcessTryNotes can make us enter a finally-block when we are closing a generator, right? Is it OK to call HandleClosingGeneratorReturn in that case? (2) The if-branch above calls HandleClosingGeneratorReturn and we will call it here too. Even if that's fine, it's confusing and it'd be nice to restructure the code to avoid this.

Comment on attachment 8568910 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT. Review of attachment 8568910 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitFrames.cpp @@ +575,5 @@ > + for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) { > + JSTryNote *tn = *tni; > + > + switch (tn->kind) { > + case JSTRY_CATCH: { Good catch, you're right. @@ +647,5 @@ > + switch (Debugger::onExceptionUnwind(cx, frame.baselineFrame())) { > + case JSTRAP_ERROR: > + // Uncatchable exception. > + MOZ_ASSERT(!cx->isExceptionPending()); > + break; Yes, also true. @@ +693,5 @@ > } > + > + if (script->hasTrynotes()) > + ProcessTryNotes(cx, frame, si.ref(), rfe, &pc); > + frameOk = HandleClosingGeneratorReturn(cx, frame.baselineFrame(), frameOk); 1) Yeah, this is tricky, but yeah this is a bug here. 2) This is actually ok but is pretty hard to restructure. I can duplicate a bit of code and split the isDebuggee path off completely, maybe.

Comment on attachment 8567739 [details] [diff] [review] Don't call Debugger::slowPathOnLeaveFrame on frames no longer in Debugger frame maps. Review of attachment 8567739 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +604,5 @@ > + // The onPop handler and associated clean up logic should not run multiple > + // times on the same frame. If slowPathOnLeaveFrame has already been > + // called, the frame will not be present in the Debugger frame maps. > + if (!inFrameMaps(frame)) > + return frameOk; Actually, I did have one thing to suggest: If I remember, inFrameMaps is implemented by creating a FrameRange, and then checking if it's empty. Building a FrameRange isn't expensive, but it's not super-cheap either. We're probably about to create a FrameRange in the loop below ("Build a list of the recipients"). Instead of calling inFrameMaps, would it be too ugly to create a FrameRange here, check if it's empty for the early return, and then use it in the loop?

(In reply to Jim Blandy :jimb from comment #35) > Comment on attachment 8567739 [details] [diff] [review] > Don't call Debugger::slowPathOnLeaveFrame on frames no longer in Debugger > frame maps. > > Review of attachment 8567739 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/Debugger.cpp > @@ +604,5 @@ > > + // The onPop handler and associated clean up logic should not run multiple > > + // times on the same frame. If slowPathOnLeaveFrame has already been > > + // called, the frame will not be present in the Debugger frame maps. > > + if (!inFrameMaps(frame)) > > + return frameOk; > > Actually, I did have one thing to suggest: > > If I remember, inFrameMaps is implemented by creating a FrameRange, and then > checking if it's empty. Building a FrameRange isn't expensive, but it's not > super-cheap either. > > We're probably about to create a FrameRange in the loop below ("Build a list > of the recipients"). Instead of calling inFrameMaps, would it be too ugly to > create a FrameRange here, check if it's empty for the early return, and then > use it in the loop? Fine with me.

Address comments. They're all basically fixed by adding an 'again:' label like in the interpreter's HandleError. Personally I think this use of goto is more readable than wrapping the entire thing in a loop and setting some control variable.

Comment on attachment 8570814 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the JIT. Review of attachment 8570814 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but these changes are really hard to review, so many cases to consider.. ::: js/src/jit/JitFrames.cpp @@ +590,5 @@ > + > + for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) { > + JSTryNote *tn = *tni; > + > + switch (tn->kind) { MOZ_ASSERT(cx->isExceptionPending()); above this line. @@ +646,3 @@ > static void > HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFromException *rfe, > + jsbytecode *pc) Nice that we can get rid of the unwoundScopeToPc and calledDebugEpilogue gunk. @@ +661,4 @@ > RootedScript script(cx, frame.baselineFrame()->script()); > > +again: > + if (cx->isExceptionPending()) { This makes the interpreter and Baseline exception handlers more similar, I think. Maybe we could even have a shared exception handler that takes an AbstractFramePtr? Please compare them and try to make them look the same as much as possible.

Comment on attachment 8571565 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter. Review of attachment 8571565 [details] [diff] [review]: ----------------------------------------------------------------- I'm concerned about the changes to HandleError. It seems to me that we essentially make three near copies of the try note handling path: 1) frame isDebuggee() and CoveredByCatchOrFinally. This copy is incomplete, as it only needs to process the next note and resume. 2) frame isDebuggee() and not CoveredByCatchOrFinally. This copy calls the onLeaveFrame hook early. 3) frame !isDebuggee(). This skips the onLeaveFrame call. It seems to me that the only reason for this variation is that JSTRY_ITER notes modify REGS.pc and REGS.sp. If JSTRY_ITER left those alone, then we could simply loop over try notes, returning early as needed for catch and finally clauses, and call onLeaveFrame only after the loop runs to completion. When leaving a frame with a 'finally' clause, onPop should see the pc at the end of the 'finally', and indeed that's where the proposed loop would leave REGS.pc. And I don't see that JSTRY_ITER has any real need to adjust REGS.pc and REGS.sp. The JSTRY_ITER note specifies the slot containing the iterator, so we can find it. Aside from legacy iterators, no JS runs when we process a TRY_ITER (and if those legacy iterators get stack traces with a less-than-perfect pc, we can live with that). ::: js/src/vm/Interpreter.cpp @@ +1020,5 @@ > + regs.pc = regs.fp()->script()->main() + tn->start + tn->length; > + regs.sp = regs.spForStackDepth(tn->stackDepth); > +} > + > +class InterpreterFrameStackDepthOp This could be a class internal to TryNoteIterInterpreter, named merely "StackDepthOp". ::: js/src/vm/Interpreter.h @@ +260,3 @@ > class TryNoteIter > { > + RootedScript script_; /* TryNotIter is always stack allocated. */ If this is the case, let's put MOZ_STACK_CLASS on TryNoteIter. Also: "TryNotIter"

I haven't forgotten about this, but won't get cycles to work on it for a bit. I hope to address review in the next 2 weeks sometime.

(In reply to Jim Blandy :jimb from comment #40) > Comment on attachment 8571565 [details] [diff] [review] > Call Debugger::onPop at the point that caused the frame to pop before any > unwinding in the interpreter. > > Review of attachment 8571565 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/Interpreter.cpp > @@ +1020,5 @@ > > + regs.pc = regs.fp()->script()->main() + tn->start + tn->length; > > + regs.sp = regs.spForStackDepth(tn->stackDepth); > > +} > > + > > +class InterpreterFrameStackDepthOp > > This could be a class internal to TryNoteIterInterpreter, named merely > "StackDepthOp". I can't, because TryNoteIterInterpreter inherits from TryNoteIter<T>, and T can't be an inner class to TryNoteIterInterpreter. I could virtualize the functor, but I don't think it's a big deal to not be able to define the StackDepthOp as an internal class. > > ::: js/src/vm/Interpreter.h > @@ +260,3 @@ > > class TryNoteIter > > { > > + RootedScript script_; /* TryNotIter is always stack allocated. */ > > If this is the case, let's put MOZ_STACK_CLASS on TryNoteIter. > > Also: "TryNotIter"

(In reply to Shu-yu Guo [:shu] from comment #42) > (In reply to Jim Blandy :jimb from comment #40) > > Comment on attachment 8571565 [details] [diff] [review] > > Call Debugger::onPop at the point that caused the frame to pop before any > > unwinding in the interpreter. > > > > Review of attachment 8571565 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: js/src/vm/Interpreter.cpp > > @@ +1020,5 @@ > > > + regs.pc = regs.fp()->script()->main() + tn->start + tn->length; > > > + regs.sp = regs.spForStackDepth(tn->stackDepth); > > > +} > > > + > > > +class InterpreterFrameStackDepthOp > > > > This could be a class internal to TryNoteIterInterpreter, named merely > > "StackDepthOp". > > I can't, because TryNoteIterInterpreter inherits from TryNoteIter<T>, and T > can't be an inner class to TryNoteIterInterpreter. I could virtualize the > functor, but I don't think it's a big deal to not be able to define the > StackDepthOp as an internal class. Oh, hmm. Yes, it was just a suggestion. InterpreterFrameStackDepthOp just seems like internal structure to me, so I was hoping we could narrow its scope and shorten its name and all that good stuff.

Addressed comments and simplified logic. JSTRY_FOR_IN notes no longer changes the pc and sp of the frame.

Updated the JIT part per the simplified logic of the interpreter part. Carrying r=jandem; not sure if warrants re-review.

Comment on attachment 8582135 [details] [diff] [review] Call Debugger::onPop at the point that caused the frame to pop before any unwinding in the interpreter. Review of attachment 8582135 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Interpreter.cpp @@ +995,5 @@ > js::UnwindScopeToTryPc(JSScript *script, JSTryNote *tn) > { > + jsbytecode *pc = script->main() + tn->start; > + if (tn->kind == JSTRY_CATCH || tn->kind == JSTRY_FINALLY) > + pc -= JSOP_TRY_LENGTH; Wouldn't Jandem want a MOZ_ASSERT(*pc == JSOP_TRY) here? @@ +1027,5 @@ > + /* > + * Set pc to the first bytecode after the the try note to point > + * to the beginning of catch or finally or to [enditer] closing > + * the for-in loop. > + */ I understand that you're just moving code around here, but could we not mix // and /* */ comments so close together? @@ +1164,2 @@ > > + switch (ProcessTryNotes(cx, si, regs)) { Since this is now the only call to ProcessTryNotes, wouldn't it make sense to inline it here, and have one less switch in total? @@ +1174,4 @@ > } > > + ok = HandleClosingGeneratorReturn(cx, regs.fp(), ok); > + ok = Debugger::onLeaveFrame(cx, regs.fp(), ok); And this is our backstop, making sure that no matter what, we eventually call the hook, right?

(In reply to Jim Blandy :jimb from comment #46) > Comment on attachment 8582135 [details] [diff] [review] > Call Debugger::onPop at the point that caused the frame to pop before any > unwinding in the interpreter. > > Review of attachment 8582135 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/Interpreter.cpp > > @@ +1164,2 @@ > > > > + switch (ProcessTryNotes(cx, si, regs)) { > > Since this is now the only call to ProcessTryNotes, wouldn't it make sense > to inline it here, and have one less switch in total? > Exception handling code is hard to read, I think it's worth it for readability to keep it in a separate function. > @@ +1174,4 @@ > > } > > > > + ok = HandleClosingGeneratorReturn(cx, regs.fp(), ok); > > + ok = Debugger::onLeaveFrame(cx, regs.fp(), ok); > > And this is our backstop, making sure that no matter what, we eventually > call the hook, right? Yeah.

Comment on attachment 8587575 [details] [diff] [review] Fix up tests now that onPop and onExceptionUnwind may be called at different locations than previously. Review of attachment 8587575 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.

https://hg.mozilla.org/mozilla-central/rev/b9a87aa39cbd https://hg.mozilla.org/mozilla-central/rev/dccb0b37351a https://hg.mozilla.org/mozilla-central/rev/2ae5caa9f204 https://hg.mozilla.org/mozilla-central/rev/f2fc5733b0c4 https://hg.mozilla.org/mozilla-central/rev/91f4274da14e https://hg.mozilla.org/mozilla-central/rev/6ea2d6612cae https://hg.mozilla.org/mozilla-central/rev/6efecddff450 https://hg.mozilla.org/mozilla-central/rev/94bdad2b4c3c