Closed Bug 894592 Opened 11 years ago Closed 11 years ago

Add pause on *uncaught* exception

Categories

(DevTools :: Debugger, defect, P2)

Product:
DevTools
Component:
Debugger
Version:
25 Branch
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 906963

People

(Reporter: fitzgen, Assigned: ejpbruel)

References

Details

Once again I was sure we had a bug open for this, and once again I couldn't find it in my searches. Feel free to dupe this bug with the existing one if it is filed already and my memory isn't playing tricks on me.

Anyways, we should be able to pause on uncaught exceptions. Most people probably don't care about exceptions that are caught and handled nicely, but the ones that aren't handled are the ones that really need their attention.
Are you talking about bug 882790 ? If yes, man! you only opened that :P
(I got to that bug using my Firefox awesomebar itself)
No that is just a UI button for pause on exceptions, this is to pause on exceptions that aren't caught, but NOT pause on exceptions that are caught and handled.
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> No that is just a UI button for pause on exceptions, this is to pause on
> exceptions that aren't caught, but NOT pause on exceptions that are caught
> and handled.

Then this is the first bug as far as I can search.
Every 6 months or so, I think "hey, why haven't I implemented this yet?" and jump straight into writing the code. Then I realize that I can't think of a way to make this work in a useful way, so I delete everything, until my memory fails me and I start over again.

The straightforward way to implement this is to either add a window.onerror listener, or better yet, use onExceptionUnwind and check frame.older. The problem is that both of these approaches will trigger when the exception has already propagated to the top of the stack and there is no useful stack trace left.

I don't know of a way to pause when the exception is first raised that doesn't involve visiting each parent frame and parsing the source to see if the current location is surrounded by a try/catch. I don't expect this to be very performant, and since I'm fairly confident that most users would want this pref on by default (I mean why wouldn't they?) we'd end up with a perf regression.

Needless to say I would be very happy to be proven wrong in my assumptions or to hear about another way to solve this that is both simple and fast.
(In reply to Panos Astithas [:past] from comment #4)
> The straightforward way to implement this is to either add a window.onerror
> listener, or better yet, use onExceptionUnwind and check frame.older. The
> problem is that both of these approaches will trigger when the exception has
> already propagated to the top of the stack and there is no useful stack
> trace left.
There is a way around that :-)
https://github.com/DavidBruant/usefulStackTrace/blob/83bb23b6ce9ea0e048d165eda4533eeb645be59f/lib/trackStack.js#L120-L175

Note also l.144 (with the === 'global'). Maybe that's obvious for some people  but it took me some time to find that trick so that the tool works with code executing in inline scripts, so I'm happy to share it :-)


> I don't know of a way to pause when the exception is first raised that
> doesn't involve visiting each parent frame and parsing the source to see if
> the current location is surrounded by a try/catch.
There is no need to parse to find for a try/catch. Just abort the operation if the frame pops with something else than 'throw' in its completion value (l.131). (I actually don't abort. I should re-init the frameData variable, so I have a bug if 2 errors are thrown :-s)

My code also doesn't take into account if an error is caught and a different one is thrown. I could test with === on the thrown completion value to make sure it's the same. I'll add an issue for that.


> I don't expect this to be
> very performant, and since I'm fairly confident that most users would want
> this pref on by default (I mean why wouldn't they?) we'd end up with a perf
> regression.
The pref can be off by default and they'll pay the perf regression as an opt-in.

It's not the first time that you mention that problem. I agree it's a problem.
A potential solution is to inform the user. For instance, anytime devtools are open, the first message of the web console could be in essence:
"You've enable options X, Y, and Z, each of which may degrade performance. Disable them to get your perf back.". Ideally, the console message would have clickable text (is that possible?) to disable each option individually and a button to disable them all in one click. Maybe "disable and refresh" clickable text?

In my usefulStackTrace tool, I've also added an option to the OptionPanel and warned the user about the perf degradation in the label https://github.com/DavidBruant/usefulStackTrace/blob/83bb23b6ce9ea0e048d165eda4533eeb645be59f/lib/main.js#L42

I really don't think perf degradation should be a reason not to ship a tool. I think a perf-degrading option can be shipped given:
1) it's disabled by default
2) devtools need to be opened for it to be enabled (enabling the pref isn't enough) as in https://github.com/DavidBruant/usefulStackTrace/blob/83bb23b6ce9ea0e048d165eda4533eeb645be59f/lib/main.js#L78-L79
3) the user is informed that a perf degrading option is on

What do you think?
(In reply to David Bruant from comment #5)
> (In reply to Panos Astithas [:past] from comment #4)
> > The straightforward way to implement this is to either add a window.onerror
> > listener, or better yet, use onExceptionUnwind and check frame.older. The
> > problem is that both of these approaches will trigger when the exception has
> > already propagated to the top of the stack and there is no useful stack
> > trace left.
> There is a way around that :-)
> https://github.com/DavidBruant/usefulStackTrace/blob/
> 83bb23b6ce9ea0e048d165eda4533eeb645be59f/lib/trackStack.js#L120-L175

I can't see from reading your code how you differentiate between caught and uncaught exceptions, which is the main obstacle I'm facing. I'll try to play with it tomorrow though.

Note that the debugger is not like the console where you don't want to pause execution and can collect data and discard them later if your assumptions don't pan out. The debugger needs to pause execution and present that state to the user for inspection, so by definition it can't let the exception propagate further without destroying that state.

One could argue that we could record everything (frames, locals, etc.) and if the exception does propagate uncaught, then present that state to the user, in a sort of back-in-time debugger. I'd argue in turn that this would be even slower and harder to get right compared to source parsing :-)

In regards to your comments on preffing off performance-degrading tools, I'm not really against it, as long as we have made our best effort to find a proper and fast solution and can document what it would take (from platform, management, or whatever) to implement it in the future.
(In reply to Panos Astithas [:past] from comment #6)
> (In reply to David Bruant from comment #5)
> > (In reply to Panos Astithas [:past] from comment #4)
> > > The straightforward way to implement this is to either add a window.onerror
> > > listener, or better yet, use onExceptionUnwind and check frame.older. The
> > > problem is that both of these approaches will trigger when the exception has
> > > already propagated to the top of the stack and there is no useful stack
> > > trace left.
> > There is a way around that :-)
> > https://github.com/DavidBruant/usefulStackTrace/blob/
> > 83bb23b6ce9ea0e048d165eda4533eeb645be59f/lib/trackStack.js#L120-L175
> 
> I can't see from reading your code how you differentiate between caught and
> uncaught exceptions, which is the main obstacle I'm facing. I'll try to play
> with it tomorrow though.
I used the following definition:
An exception is uncaught if the last frame on the stack has a completion value with 'throw'. That's the essence of the test L.144.

So I attach an onPop handler on *every single frame*. When it pops, I can know if it's an error thrown (L.131: if('throw' in completionValue))
Then, I capture everything I need if the next frames are exited with "'throw' in completionValue".

If the frame with ('throw' in completionValue) happens to be the last one (L.144), then it's an uncaught error.


> Note that the debugger is not like the console where you don't want to pause
> execution and can collect data and discard them later if your assumptions
> don't pan out. The debugger needs to pause execution and present that state
> to the user for inspection, so by definition it can't let the exception
> propagate further without destroying that state.
What is your definition of uncaught exception, then? oh... I guess I understand now why you need the parsing part... What you want to implement is "onWillNotBeCaughtException".

> One could argue that we could record everything (frames, locals, etc.) and
> if the exception does propagate uncaught, then present that state to the
> user, in a sort of back-in-time debugger. I'd argue in turn that this would
> be even slower and harder to get right compared to source parsing :-)
I can't see this being too slow. If you're willing to attach an onpop on every frame (which is expensive), then, the rest of the work happens only if an error if thrown which is rare enough. Work is storing a few values (only Debugger.Frame objects) for the number of traversed frames (100? 1000 in worst cases?). This seems small enough to not be slow in realistic cases.

As far as getting it right, assuming the error passes through frames and no code runs between frames, the only thing you need is keeping a reference to the Debugger.Frame objects being passed through (which is what I should do actually...), any other info can be read from these objects (no need to clone anything).
If code runs (finally block or error caught+code runs and some error re-thrown)... maybe we can just wait for a user to complain. It might just never happen :-)

Code running and re-throwing can be detected (by putting breakpoint everywhere in the frame script and cancelling the current stack info (or joining them?) if any is hit) so we can restart the stack like if a new error was thrown.
No good idea for finally blocks. Do people want data for before or after the finally block anyway? Let's give them what we have and see if anyone complains?
Perfect may be the enemy of the good here.

I'll be working on that with my addon and see how it works.
(In reply to David Bruant from comment #7)
> (In reply to Panos Astithas [:past] from comment #6)
> > Note that the debugger is not like the console where you don't want to pause
> > execution and can collect data and discard them later if your assumptions
> > don't pan out. The debugger needs to pause execution and present that state
> > to the user for inspection, so by definition it can't let the exception
> > propagate further without destroying that state.
> What is your definition of uncaught exception, then? oh... I guess I
> understand now why you need the parsing part... What you want to implement
> is "onWillNotBeCaughtException".

Exactly. This is the moment in time when the user has an opportunity to inspect the current program state and figure out why this exception was raised and what state the parent frames were in.

> > One could argue that we could record everything (frames, locals, etc.) and
> > if the exception does propagate uncaught, then present that state to the
> > user, in a sort of back-in-time debugger. I'd argue in turn that this would
> > be even slower and harder to get right compared to source parsing :-)
> I can't see this being too slow. If you're willing to attach an onpop on
> every frame (which is expensive), then, the rest of the work happens only if
> an error if thrown which is rare enough. Work is storing a few values (only
> Debugger.Frame objects) for the number of traversed frames (100? 1000 in
> worst cases?). This seems small enough to not be slow in realistic cases.
> 
> As far as getting it right, assuming the error passes through frames and no
> code runs between frames, the only thing you need is keeping a reference to
> the Debugger.Frame objects being passed through (which is what I should do
> actually...), any other info can be read from these objects (no need to
> clone anything).
> If code runs (finally block or error caught+code runs and some error
> re-thrown)... maybe we can just wait for a user to complain. It might just
> never happen :-)
> 
> Code running and re-throwing can be detected (by putting breakpoint
> everywhere in the frame script and cancelling the current stack info (or
> joining them?) if any is hit) so we can restart the stack like if a new
> error was thrown.
> No good idea for finally blocks. Do people want data for before or after the
> finally block anyway? Let's give them what we have and see if anyone
> complains?
> Perfect may be the enemy of the good here.
> 
> I'll be working on that with my addon and see how it works.

Note that what I alluded to was the need to retain the values of local variables for each frame in the stack, so that inspection can work as usual. And the main correctness issues I'm thinking of, is about letting the user interact with the state of the previous frames using watch expressions/the web console/etc., or soon even modifying the source of the parent frame to catch the exception.

These things wouldn't work without an almost full-blown back-in-time debugger, but would Just Work(TM) with the source parsing solution I mentioned before.
(In reply to Panos Astithas [:past] from comment #8)
> Note that what I alluded to was the need to retain the values of local
> variables for each frame in the stack, so that inspection can work as usual.
As I explained in my previous comment, inspection (only reading values) can work as usual (except *maybe* in the weirdo case of finally blocks and even then, it might be workable). All the info can be found starting at the popped Debugger.Frame objects.

> And the main correctness issues I'm thinking of, is about letting the user
> interact with the state of the previous frames using watch expressions/the
> web console/etc., or soon even modifying the source of the parent frame to
> catch the exception.
That wouldn't work with my solution indeed.

> These things wouldn't work without an almost full-blown back-in-time
> debugger, but would Just Work(TM) with the source parsing solution I
> mentioned before.
You might not need parsing assuming some improvements to the JS debugger API. If it was possible to know in which Debugger.Environment each frame on the stack stopped and know whether the type of environment is a try block (we currently don't have this level of accuracy) (does a try block define its own Debugger.Environment?) 
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference/Debugger.Environment#Accessor_properties
Is it worth filing dependent bugs?
(In reply to Panos Astithas [:past] from comment #8)
> And the main correctness issues I'm thinking of, is about letting the user
> interact with the state of the previous frames using watch expressions/the
> web console/etc., or soon even modifying the source of the parent frame to
> catch the exception.
If the code can change underneath your feet, how can you know whether an error will be caught or not? (the user may add a try/catch) ;-)
(In reply to David Bruant from comment #9)
> You might not need parsing assuming some improvements to the JS debugger
> API. If it was possible to know in which Debugger.Environment each frame on
> the stack stopped and know whether the type of environment is a try block
> (we currently don't have this level of accuracy) (does a try block define
> its own Debugger.Environment?) 
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/
> JS_Debugger_API_Reference/Debugger.Environment#Accessor_properties
> Is it worth filing dependent bugs?

Maybe, I'm not sure. I need to think about (and play with) it some more. In any case CCing Jim for his thoughts.

(In reply to David Bruant from comment #10)
> If the code can change underneath your feet, how can you know whether an
> error will be caught or not? (the user may add a try/catch) ;-)

Well, obviously the decision to consider an exception as uncaught would happen before pausing execution and giving the control back to the user, who would then get a chance to modify the parent stack frames :-)
Would we be creating value for our users by giving them "pause on uncaught exception, when it unwinds to the global frame" now, and replacing it with "pause on exception that won't be caught in the stack frame it is thrown in" later?
Errr, adding a comma makes this easier to parse: "pause on exception that won't be caught*,* in the stack frame it is thrown in"
(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> Would we be creating value for our users by giving them "pause on uncaught
> exception, when it unwinds to the global frame" now, and replacing it with
> ""pause on exception that won't be caught*,* in the stack frame it is thrown in""
> later?
I would believe so. Some frameworks throw and catch their own errors which makes "pause on throw" really annoying sometimes. I think it's the case of Raphael.js (or at least the version I use in one of my projects)
I imagine "pause on throw" could even be replaced by "pause on uncaught exception, when it unwinds to the global frame" (and eventually replace that with "pause on won't be caught, in the stack it's thrown in").
(In reply to David Bruant from comment #14)
> Some frameworks throw and catch their own errors which
> makes "pause on throw" really annoying sometimes.

Chrome debugging with "pause on exceptions" can be pretty frustrating as well.
(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> (In reply to David Bruant from comment #14)
> > Some frameworks throw and catch their own errors which
> > makes "pause on throw" really annoying sometimes.
> 
> Chrome debugging with "pause on exceptions" can be pretty frustrating as
> well.
Barely unusable.
oh... This reminds of a feature idea I had while frustrated: ignore files.
Basically, anytime the debugger would lead you to a file you don't care about (like a minified jQuery or code you trust to work for the time being), it would just skip the file if you "step into" a function defined on the file or even if you have breakpoints there... or if an error is thrown from there :-). I'll try to think to file it tonight.
heh heh, I'm working on it now: bug 875034
(In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> heh heh, I'm working on it now: bug 875034
AAAAAAAAAAAHHH ! I LOVE YOU SO MUCH !
(In reply to David Bruant from comment #7)
> I'll be working on that with my addon and see how it works.
Made some refactoring, added some (semi-manual) tests. The code should be easier to read now https://github.com/DavidBruant/usefulStackTrace/blob/c0d7ff3a70af81cb7569f173c0e42d4c3211d14d/lib/trackStack.js#L125-L189

(In reply to David Bruant from comment #7)
> ... the only thing you need is keeping a reference to
> the Debugger.Frame objects being passed through (which is what I should do
> actually...), any other info can be read from these objects (no need to
> clone anything).
I tried that and failed with a "frame is not live" error at some point. I imagine it's when I tried to get the offset (duh?!) which I need to report the line number.
I imagine there are some other infos that need to be fetched while the frame is live.
The essence of what I said remains though (the infos that need to be gathered can be gathered when the frame is live, but no need to deep-clone the frame)

Also, my code now takes care of when an error is caught in 2 cases:
* when an error is tracked, a frame is added to the stack (the error was caught and a function was called)
* the error value changed (meaning the error was caught and another error was thrown. I track the new error and report a stack trace for it)

If an error is caught and rethrown within the same frame, I can't detect it for now. I could (put breakpoints everywhere and see if one is hit before the next frame is popped), but I don't know if it's worth it.


> "pause on uncaught exception, when it unwinds to the global frame"
> VS
> "pause on exception that won't be caught, in the stack frame it is thrown in"
Should a different bug be filed? Which one this is one?
(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> Would we be creating value for our users by giving them "pause on uncaught
> exception, when it unwinds to the global frame" now, and replacing it with
> "pause on exception that won't be caught in the stack frame it is thrown in"
> later?

I honestly can't see any real value in this, but I'd like to hear from others that do. The user would already know that her app throws an uncaught exception from the web console logs. What's the use in pausing after the exception has propagated to the top of the stack?
(In reply to Panos Astithas [:past] from comment #20)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> > Would we be creating value for our users by giving them "pause on uncaught
> > exception, when it unwinds to the global frame" now, and replacing it with
> > "pause on exception that won't be caught in the stack frame it is thrown in"
> > later?
> 
> I honestly can't see any real value in this, but I'd like to hear from
> others that do. The user would already know that her app throws an uncaught
> exception from the web console logs.
The stack is still missing though (bug 814497 - 3 duplicates). That alone would have value. First reason I did my addon; I'd be happy to stop working on it if you show the stack trace natively :-)

> What's the use in pausing after the
> exception has propagated to the top of the stack?
Even if pausing after, the state program at time of error thrown can be shown (modulo what happened in finally blocks). That's enough of an approximation to be extremely useful for debugging.
If you want to have this level of information now, you have to "pause on throw" with the downside we explained (some libs throw and catch their own errors making "pause on throw" borderline unusable).

I guess a question to evaluate whether it's worth the effort is how far away are we from "pause on exception that won't be caught, in the stack frame it is thrown in"?
Assignee: nobody → nfitzgerald
Priority: -- → P2
Jim, apparently you have an idea about how we could make this easier from the platform side of things?
Flags: needinfo?(jimb)
My thought was that, if a Debugger.Script could tell you whether a given bytecode offset was covered by a 'catch' clause, then we could walk the stack when onExceptionUnwind is first called, and check each scripted debuggee frame to see if it might catch the exception. If we couldn't find any such, then it seems pretty safe to flag it as an uncaught exception --- and we'd still be at the point of the throw.

Obviously, this won't recognize C++ catches; nor will it recognize code like:

try {
  terribly fallible thing;
} catch (x if x instanceof CorrectibleCondition) {
  correct said condition;
}

that isn't going to catch the exception value at hand; nor catch clauses that re-throw. And it might be fooled by logging code. But it'd be a first cut.
Flags: needinfo?(jimb)
Here's the bug I filed for what one might call Debugger.Script.prototype.catchCoversOffset:

https://bugzilla.mozilla.org/show_bug.cgi?id=906963
Depends on: 906963
Would that catch:

> try {
>   functionThatThrows();
> } catch (e) {}

?
Eddy is working on this at the devtools meet up.
Assignee: nfitzgerald → ejpbruel
AFAICT the only missing piece here is adding a menu item to the UI to control the setting. Are you going to do this Eddy, or were you waiting for somebody else?
Er, disregard the above, it appears that the other bug has a patch that bounced but the bug was marked as resolved.
I'm closing this bug as all of the required work landed as part of bug 906963.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.