1092922 - "Pause on exceptions" forces you to unwind the stack manually

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[James Long (:jlongster)]

If you enable "pause on exceptions" and throw an exception that is unhandled, for some reason it pauses on every single frame in the stack as it is popped up. You have to basically unwind the stack manually. This is pointless since you have the frames on the original pause, and you can walk up it then if you like. (as a small data point, Chrome doesn't do this)

STR:

1. http://jlongster.com/s/pause-unwind/
2. Open debugger and enable "pause on exceptions"
3. Reload and press "continue" when it's paused. You'll have to press continue several times to get out of it

I'm not entirely sure how this is implemented (or if it's intentional) but it also seems ripe for bugs about when something other JS executes and we accidentally pause it.

Comment 1

[James Long (:jlongster)]

Attachment: 1092922.patch

This was actually really easy, and I think this makes it a lot more intuitive. What do you think?

Comment 2

[James Long (:jlongster)]

Actually, that's not going to work. We might re-throw the same exception and we shouldn't key off that. We should be more strict and just check if it's a parent frame specifically.

Comment 3

[James Long (:jlongster)]

Attachment: 1092922.patch

What about something like this? I tried only storing `frame.older` and then checking if the current frame was that one stored in the map, but there appears to be some strange self-hosted frames that made it difficult to check if it was actually the direct parent frame. We're already walking the frames anyway, so this just marks all of them and in the future we won't pause if those are unwinding with the same value.

Comment 4

[James Long (:jlongster)]

Attachment: 1092922.patch

Damn, added a single bad character accidentally in the last patch.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8515776 [details] [diff] [review]
1092922.patch

Review of attachment 8515776 [details] [diff] [review]:
-----------------------------------------------------------------

The approach looks pretty good overall.

This behavior should definitely be controllable the same way ignoring caught exceptions is. That is, make it an option that we can control via reconfiguring the thread/attaching/etc, and also make a persistent pref + entry in the debugger options dropdown menu for this.

I think we need to clear the exception values anytime we actually do pause and send the packet and everything. Take this scenario: we hit an exception and pause, user hits continue, unwind the stack until we hit a catch (or a finally) block with a BP and pause because of something other than the exception unwinding, the user hits continue again and the end of the catch block re-throws the same value or the finally block is over and we continue stack unwinding. In this case, I think we /do/ want to pause on the unwind again. That is, I don't think we want to only pause once per exception, but only require one "resume" command per /consecutive/ exception unwinds, and if we interrupt frame unwinding due to some other pause, it should reset our state.

In addition, we don't need to save the exception value for every parent frame, just for the current tick of the event loop, aka the oldest frame. I think you could just define a "lastThrownException" (or something) property on the oldest frame and check against that in onExceptionUnwind. Maybe even define an `oldestFrame` getter on `ThreadActor.prototype`, similar tot he existing `youngestFrame` getter. What's nice about putting it on the oldest frame is that it gets the correct lifetime for free and we don't need to rely on WeakMaps or manually cleaning up at the appropriate time (except for the case I mention in the previous paragraph).

I think this feature requires some pretty good test coverage, because there are a lot of corner cases:

* The case I mention above where we interrupt stack unwinding with a BP/debugger-statement/etc pause:

  function doSomeStuff() {
    debugger;
  }

  try {
    somethingThatThrows();
  } finally {
    doSomeStuff();
    // We should pause at the end of this block again, because stack unwinding
    // was interrupted.
  }

* Re-throwing the same exception is ignored, and we only pause the first time:

  try {
    somethingThatThrowsInMultipleWays();
  } catch (e) {
    if (weCantHandleTheError()) {
      throw e;
    }
    handleTheError();
  }

* Catching and throwing a different value pauses again:

  function foo() { ... }
  try {
    somethingThatThrows();
  } catch (e) {
    fo(); // oops a ReferenceError; pause here
  }

* Throwing the same value on a different tick of the event loop pauses:

  const ERRNO_STUPID_ERROR = 1;
  function throwError() { throw ERRNO_STUPID_ERROR; }
  // Should pause twice:
  executeSoon(throwError);
  executeSoon(throwError);

* Your simple test case:

  function a() { b(); }
  function b() { c(); }
  function c() { throw new Error(); }
  // Only pauses once:
  a();

* What should we do with nested exceptions?

  try {
    somethingThatThrows();
  } catch (e) {
    try {
      tryAndHandleTheErrorButThrowNewError(e);
    } catch (_) {}
    // Do we pause again here? I /think/ we want to, because we would
    // pause-and-reset-error-state when we pause on the second,
    // nested exception.
  }

::: toolkit/devtools/server/actors/script.js
@@ +2200,5 @@
> +   * This is for `onExceptionUnwind`; keep track of exceptions that
> +   * we've already paused so we don't pause on every single stack
> +   * frame
> +   */
> +  _exceptionFrames: new WeakMap(),

Don't put mutable values directly in the prototype, or they will be shared by every instance. Instead, initialize the property in the constructor.

In this specific case, this wouldn't materialize any bugs because different ThreadActor instances will be using different Debugger instances and therefore get different Debugger.Object instances for the exception values. However, we have had this kind of thing result in bugs before, and it would make the careful reader stop and wonder if this was meant to be shared, and how the sharing would work, cause a bunch of confusion, etc, etc.

Comment 6

[James Long (:jlongster)]

Thanks! I agree with your points, configuration would be nice (although there are getting to by many options, I think that's a good thing, but we should whatever we can do make it easy to figure out what those options do).

I won't be able to look at this again until next week. I selfishly needed this behavior for a demo for my talk this week :) I will respond more thoroughly and provide a new patch then.

Comment 7

[Eddy Bruel [:ejpbruel]]

James, are you still working on this? The bug is currently not assigned to you.

Comment 9

[Till Schneidereit [:till]]

(In reply to James Long (:jlongster) from comment #6)
> Thanks! I agree with your points, configuration would be nice (although
> there are getting to by many options, I think that's a good thing, but we
> should whatever we can do make it easy to figure out what those options do).

As I argued in bug 1064603, I don't think it's necessary to make this configurable. The current behavior essentially boils down to single-stepping up the stack. If you really want that, you can always, well, single-step.

Comment 10

[James Long (:jlongster)]

(In reply to Eddy Bruel [:ejpbruel] from comment #7)
> James, are you still working on this? The bug is currently not assigned to
> you.

Sorry, I'm not currently working on this, no. Will likely pick it up in the future though.

Comment 11

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #10)
> (In reply to Eddy Bruel [:ejpbruel] from comment #7)
> > James, are you still working on this? The bug is currently not assigned to
> > you.
> 
> Sorry, I'm not currently working on this, no. Will likely pick it up in the
> future though.

That's ok. I just want to make sure the assigned status of these bugs is up to date in case anyone else wants to pick this up!

Comment 12

[Florent Fayolle]

Couldn't we just land the patch (with some adjustments if really necessary) and do improvements in a second step?

Because at work, I use the option "pause on exceptions" to discover exceptions… and the callstack may be *huge* (I mean, ~30 length, or even more) and resuming is just a pain. My workaround is to reopen the devtools but that's a pity.

Florent

Comment 13

[James Long (:jlongster)]

You are right, I still don't like this behave and we shouldn't have let it go for this long. I will assign it back to myself and try to work on it soon.

Comment 14

[Florent Fayolle]

Up please :(

Florent

Comment 15

[Tom Tromey :tromey]

Still relevant with the new debugger front end.

Comment 17

[Brian Hackett [Laid off!]]

Attachment: Bug 1092922 - Only pause once when unwinding a particular exception, r=loganfsmyth.

Comment 18

[Brian Hackett [Laid off!]]

The patch here is the same as James' in comment 4, except with added tests. The semantics seem good to me and it's nice to keep things simple.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc16ac6795e0
Only pause once when unwinding a particular exception, r=loganfsmyth.

Comment 20

[Natalia Csoregi [:nataliaCs]]

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