906963 - [jsdbg2] predicting uncaught exceptions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jim Blandy :jimb]

The DevTools team has been making an effort to talk with real-life JS developers to find out what they really miss in our developer tools, to identify features which would be very valuable to devs but would be relatively easy to implement. It's already yielded some good results, like the "black-boxing" feature (described here, but not necessary to read for this bug: https://hacks.mozilla.org/2013/08/new-features-of-firefox-developer-tools-episode-25/)

Another thing developers really want is the ability to pause the page in the debugger when an exception is thrown that's not going to be caught. This is hard for us to implement perfectly, as sometimes it's C++ that decides whether to propagate or catch an exception, but even an 80% solution would be very valuable.

We can pause the page when *any* exception is thrown, but it turns out that jQuery generates a bunch of exceptions when it's initializing itself: it adapts itself to the browser it's running in by trying a bunch of operations and seeing which ones throw. So a simple "pause on throw" feature requires the developer to sit there and hit "continue" a half-dozen times every time they reload their page. Not a pleasant experience.

So here's the 80% approximation we'd like to try: when an exception is thrown, we want to scan the JS frames on the stack to see if any of them are executing within the scope of a 'catch' clause. This is a matter of identifying each frame that is running JS, getting the frame's JSScript and the bytecode offset within that script, and then looking up that offset in the JSScripts' "source notes" data structure to see if a catch clause covers it.

Debugger can already do all of this, except that last step: looking up the offset in the source notes looking for a catch clause. This would be simple to add as a method on Debugger.Script.prototype, but it requires someone who is familiar with the source notes.

Comment 1

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

P1 because it is blocking work on bug 894592.

Comment 2

[David Bruant]

Attachment: try clause define a lexical environment

Comment 3

[David Bruant]

(In reply to Jim Blandy :jimb from comment #0)
> So here's the 80% approximation we'd like to try: when an exception is
> thrown, we want to scan the JS frames on the stack to see if any of them are
> executing within the scope of a 'catch' clause.
Do you mean 'try' clause?
An error may be thrown from a catch clause and be of interest:

  try{
      throw new Error('yo');
  }
  catch(e){
      throw new Error('what?');
  }

If nothing catches 'what?', we want to know about it.


> This is a matter of
> identifying each frame that is running JS, getting the frame's JSScript and
> the bytecode offset within that script, and then looking up that offset in
> the JSScripts' "source notes" data structure to see if a catch clause covers
> it.
> 
> Debugger can already do all of this, except that last step: looking up the
> offset in the source notes looking for a catch clause. This would be simple
> to add as a method on Debugger.Script.prototype, but it requires someone who
> is familiar with the source notes.
I might be misunderstanding the Debugger.Script abstraction, but I feels that "is an error thrown from this frame going to be caught?" is better answered at the Debugger.Environment level as it's a very static/source-level information. On the other hand, some part of a given Debugger.Script may be covered by try{}, but not some other part, that's why I'm doubtful about using Debugger.Script.

Also, a try block defines a Debugger.Environment (lexical environment for let variables, see attachment) or at least it should. So it could make sense to add a 'try' boolean to Debugger.Environment instances just to know whether they're a try/catch block.
Instead of a new boolean, we may want to split up the "declarative" type value into more accurate informations ('eval', 'block', 'try', 'catch', etc.). This is a breaking change from the current Debugger API, but a reasonable one, I believe.

From a frame, one would do:
  if(myFrame.environment.type === 'try'){
    // an error thrown by the frame will be caught
  }
  else{
    // an error won't be caught by that frame.
  }

Gathering the same info on all frames provides the answer we're looking for (whether a thrown information will be caught somewhere in the stack)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fwiw, the C++ side of this has three kinds of callers:

1)  Will report the exception as soon as you return from JS to C++.  Your approach does the Right Thing here.

2)  Will rethrow the exception to some calling JS.  There's only one of these that I know of: NodeFilter.

3)  Will save the exception object and report it via non-exception mechanisms. Again, only one: Promises.

If we cared, we could even tell the JS engine about the fact that Promises and NodeFilter plan to do something interesting with the exception...

Comment 5

[Jim Blandy :jimb]

(In reply to David Bruant from comment #3)
> (In reply to Jim Blandy :jimb from comment #0)
> > So here's the 80% approximation we'd like to try: when an exception is
> > thrown, we want to scan the JS frames on the stack to see if any of them are
> > executing within the scope of a 'catch' clause.
> Do you mean 'try' clause?

You and I are using 'scope' in different ways. By "within the scope of a 'catch' clause", I meant, "within a 'try' block that has a 'catch' clause"; I should have said that in the first place. Certainly exceptions thrown within a 'catch' or 'finally' clause are of interest.

> I might be misunderstanding the Debugger.Script abstraction, but I feels
> that "is an error thrown from this frame going to be caught?" is better
> answered at the Debugger.Environment level as it's a very
> static/source-level information. On the other hand, some part of a given
> Debugger.Script may be covered by try{}, but not some other part, that's why
> I'm doubtful about using Debugger.Script.

You're misunderstanding. Indeed, which code locations within a script lie within a 'try' block with a 'catch' clause is static; Debugger.Script is exactly the representation of the static aspects of the program. So Given a script 's', one would ask, 's.coveredByCatch(offset)', where 'offset' is a bytecode offset within the script.

> Also, a try block defines a Debugger.Environment (lexical environment for
> let variables, see attachment) or at least it should. So it could make sense
> to add a 'try' boolean to Debugger.Environment instances just to know
> whether they're a try/catch block.

Yeah, that's just not the right approach at all. Catch clauses are control flow structures, not variable scoping structures.

I think the spec actually has most of the characteristics you've asked for in this bug; you're just not seeing it yet.

Comment 6

[Brandon Benvie [:benvie]]

(In reply to Jim Blandy :jimb from comment #5)
> Yeah, that's just not the right approach at all. Catch clauses are control
> flow structures, not variable scoping structures.

They do introduce a new scope though:

> try {
>   throw null;
> } catch (e) {
>   let foo = 10;
> }
> foo; // ReferenceError

Comment 7

[David Bruant]

(In reply to Jim Blandy :jimb from comment #5)
> Catch clauses are control
> flow structures, not variable scoping structures.
> 
> I think the spec actually has most of the characteristics you've asked for
> in this bug; you're just not seeing it yet.
Ok. I think I see it now. And Boris points make sense.

(In reply to Boris Zbarsky [:bz] from comment #4)
> Fwiw, the C++ side of this has three kinds of callers:
> 
> 1)  Will report the exception as soon as you return from JS to C++.  Your
> approach does the Right Thing here.
> 
> 2)  Will rethrow the exception to some calling JS.  There's only one of
> these that I know of: NodeFilter.
> 
> 3)  Will save the exception object and report it via non-exception
> mechanisms. Again, only one: Promises.

4?) .dispatchEvent swallows exceptions thrown from event listeners... Actually, we may want to be aware of these as they are uncaught by "userland JS".

> If we cared, we could even tell the JS engine about the fact that Promises
> and NodeFilter plan to do something interesting with the exception...
Probably out of scope for an 80% solution (like what I said for .dispatchEvent). The kind of things that can wait until people ask for it (if that ever happens), I guess.

> Debugger.Script is
> exactly the representation of the static aspects of the program. So Given a
> script 's', one would ask, 's.coveredByCatch(offset)', where 'offset' is a
> bytecode offset within the script.
Just sharing thoughts for a future where we'd take care of how host functions affect error handling:
host functions don't have a related Debugger.Script. Maybe the information of how they take care of error handling can be put on the related Debugger.Object instance?

If it's not more expensive (runtime and developement time), it could be interesting if the s.coveredByCatch(offset) provided more than a boolean. For instance, the offsets where the execution may resume after the try block like the catch block first offset, finally block first offset and after catch/finally first offset.
But if that makes your life any more complicated Eddy, don't bother.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 4?) .dispatchEvent swallows exceptions thrown from event listeners...

No, it doesn't.  It reports them to window.onerror and whatnot, at the point when you return from JS to C++.  So any exception in an event listener that's not inside a try statement is in fact uncaught.

> Actually, we may want to be aware of these as they are uncaught by "userland JS".

Exactly.

Comment 9

[Eddy Bruel [:ejpbruel]]

(In reply to Jim Blandy :jimb from comment #0)
> Debugger can already do all of this, except that last step: looking up the
> offset in the source notes looking for a catch clause. This would be simple
> to add as a method on Debugger.Script.prototype, but it requires someone who
> is familiar with the source notes.

I believe there may be a better solution. As it turns out the emitter also emits something called try notes. This is what the interpreter uses internally to find catch and/or finally blocks for a given bytecode offset.

I'm going to look into this tomorrow.

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Detect whether a bytecode offset is within the scope of a catch statement.

Here's the promised patch. This implements the hard part, which is detecting whether a given bytecode offset is within scope of a catch statement. The rest is just hooking everything up with the server/client/ui, etc.

Comment 11

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

Eddy, just a note, we have bug 894592 open for the debugger integration with the new platform API. Feel free to fix it all here and then close that bug when this one lands, if that is easier for you.

Comment 12

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #9)
> I believe there may be a better solution. As it turns out the emitter also
> emits something called try notes. This is what the interpreter uses
> internally to find catch and/or finally blocks for a given bytecode offset.

Ah, right; I think it used to be saved in the srcnotes, and then got separated out into its own structure. (But I could be be imagining that...)

Comment 13

[Jim Blandy :jimb]

... I'm wrong. Trynotes and srcnotes have always been separate. And try notes are definitely the right thing to use.

Comment 14

[Jim Blandy :jimb]

Comment on attachment 796246 [details] [diff] [review]
Detect whether a bytecode offset is within the scope of a catch statement.

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

Looks great! Some suggestions for two or three more tests:

::: js/src/jit-test/tests/debug/Script-isInCatchScope.js
@@ +26,5 @@
> +// Should correctly detect catch blocks
> +test("throw new Error();", [false]);
> +test("try { throw new Error(); } catch (e) {}", [true]);
> +test("try { throw new Error(); } finally {}", [false, false]);
> +test("try { throw new Error(); } catch (e) {} finally {}", [true]);

How about both 'catch' and 'finally'?

How about a throw just before and just after a try { } block? That is, we'd like to try the offsets right next to, but not within, the try note's range.

::: js/src/vm/Debugger.cpp
@@ +3552,5 @@
> +    if (script->hasTrynotes()) {
> +        JSTryNote* tnBegin = script->trynotes()->vector;
> +        JSTryNote* tnEnd = tnBegin + script->trynotes()->length;
> +        while (tnBegin != tnEnd) {
> +            if (offset - tnBegin->start < tnBegin->length &&

I understand the 'unsigned' trick you're using here, and that's great to use in generated machine code, but let's just say "tnBegin->start <= offset && offset < tnBegin->start + tnBegin->length"; that's what people expect to see (and actually, the compiler knows about the unsigned comparison trick, and may apply it here anyway).

Comment 15

[Eddy Bruel [:ejpbruel]]

Attachment: Add a flag to pauseOnExceptions to optionally ignore caught exceptions.

Comment 16

[Eddy Bruel [:ejpbruel]]

Comment on attachment 796622 [details] [diff] [review]
Add a flag to pauseOnExceptions to optionally ignore caught exceptions.

Reassigning to dcamp because past is too busy.

Comment 17

[Eddy Bruel [:ejpbruel]]

Attachment: Add a flag to pauseOnExceptions to optionally ignore caught exceptions.

New patch, now with 100% more tests.

Comment 18

[Dave Camp (:dcamp)]

Comment on attachment 796628 [details] [diff] [review]
Add a flag to pauseOnExceptions to optionally ignore caught exceptions.

Actually, can you update the callers of pauseOnExceptions in this patch please?

Comment 19

[Dave Camp (:dcamp)]

Talked to Eddy, no callers need updating (none use the callback argument).

Comment 20

[Eddy Bruel [:ejpbruel]]

Attachment: Add a "ignore caught exceptions" checkbox to the UI

Comment 21

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/581d27f7d811

Comment 22

[Eddy Bruel [:ejpbruel]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbaea63ac14

Comment 23

[Eddy Bruel [:ejpbruel]]

Changed the default setting for 'ignore caught exceptions' to true as per fitzgen's request:

https://hg.mozilla.org/integration/mozilla-inbound/rev/63be67166a2b

Comment 24

[Ryan VanderMeulen [:RyanVM]]

1.) This landed with the wrong bug #.
2.) It was causing mochitest failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=27220614&tree=Mozilla-Inbound

Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d1fa974bfd

I'll resist the urge to leave a comment on your blog post ;)

Comment 25

[Eddy Bruel [:ejpbruel]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> 1.) This landed with the wrong bug #.
> 2.) It was causing mochitest failures.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27220614&tree=Mozilla-
> Inbound
> 
> Backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/02d1fa974bfd
> 
> I'll resist the urge to leave a comment on your blog post ;)

Thanks. I had a green try run for this, but looks like the last minute change of the default setting broke an invariant in one of the tests.

Comment 26

[Eddy Bruel [:ejpbruel]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> 1.) This landed with the wrong bug #.
> 2.) It was causing mochitest failures.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27220614&tree=Mozilla-
> Inbound
> 
> Backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/02d1fa974bfd
> 
> I'll resist the urge to leave a comment on your blog post ;)

Actually, it looks like you backed it out with the wrong number.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

*facepalm* qbackout uses the bug number from the original commit...
"Bug 906693 - Add a "ignore caught exceptions" checkbox to the UI"

Comment 28

[Eddy Bruel [:ejpbruel]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> *facepalm* qbackout uses the bug number from the original commit...
> "Bug 906693 - Add a "ignore caught exceptions" checkbox to the UI"

Damnit. You're right, I was looking at the wrong commit in my history.

I'm just going to stop talking now.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/581d27f7d811
https://hg.mozilla.org/mozilla-central/rev/5dbaea63ac14

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Looks like we still need to land the third patch that bounced.

Comment 31

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77841a0ee530

Double checked the bug number and ensured a green try run before pushing:
https://tbpl.mozilla.org/?tree=Try&rev=05fc3b1f860c

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77841a0ee530

Should this have been [leave open] still? Please remove the whiteboard status and resolve the bug if not.

Comment 33

[Eddy Bruel [:ejpbruel]]

I've added some additional tests and other review suggestions from jimb that I forgot to add to the previous patches before landing them:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae62dbb782

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ae62dbb782b