757282 - Pause when an exception is hit

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Thaddee Tyl [:espadrine]]

The debugger currently does not have a feature to break (pause JS execution)
as soon as an exception is thrown.

That feature is available on all other major browser.
It is very useful to get the whole context of an uncaught exception.

Comment 1

[Rob Campbell [:rc] (:robcee)]

spidermonkey API requirement? Possibly be able to either install an onError or Debugger object in the browser to invoke the debugger.

Need to investigate, may not be able to get for version 1.

Comment 2

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

There is Debugger.onExceptionUnwind which does what we need here. We may also use Debugger.onError for other errors, but I'm not sure if it's ready yet.

Comment 3

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

Attachment: WIP

Made some progress in both frontend and backend.

Comment 4

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

I'll base this on top of the scope refactoring patch, in order to avoid rebasing.

Comment 5

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

Attachment: Patch v2

This is working, but has no tests and I need some feedback on UI and protocol additions.

I've used a checkbox toolbar button with the same configuration icon we use in the page inspector. My plan was to make it work the same as the inspector, a menu with various options. But I don't know if it would be better to keep it as a separate button as long as we only have one option. I also recall that Paul spent quite some time trying to get l10n and a11y right with that. In case we keep it as a button and not a menu, we will need shorlander to provide us with a new icon (webkit has a cross between a stop sign and a pause button, which seems nice).

Jim, I added another protocol operation to let the client configure the server actor:

{
  "to": "conn0.context710",
  "type": "configure",
  "pauseOnExceptions": true
}
{
  "from": "conn0.context710"
}

I imagine us adding more options in that packet in the future, although I can't think of one right now.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 627257 [details] [diff] [review]
Patch v2

I think this approach looks ok (without having actually building the patch).

One question: The Debugger UI has to be "up" in order for this option to take effect, right? If you set that thing, the debugger won't popup on any page with a JS Exception? That's probably the right thing to do as it could get pretty annoying hitting exceptions on pages you didn't mean to debug, but it does require a bit of forethought.

As for the "gear" icon vs. a checkbox, I think a single checkbox is enough. When we get more options, we can add the gear menu.

Comment 7

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> Comment on attachment 627257 [details] [diff] [review]
> Patch v2
> 
> I think this approach looks ok (without having actually building the patch).
> 
> One question: The Debugger UI has to be "up" in order for this option to
> take effect, right? If you set that thing, the debugger won't popup on any
> page with a JS Exception? That's probably the right thing to do as it could
> get pretty annoying hitting exceptions on pages you didn't mean to debug,
> but it does require a bit of forethought.

Right, the Debugger server has to be running first.

> As for the "gear" icon vs. a checkbox, I think a single checkbox is enough.
> When we get more options, we can add the gear menu.

OK, but where do I put the checkbox? Right in the toolbar?

Comment 8

[Rob Campbell [:rc] (:robcee)]

I would say yes, for now.

Comment 9

[Jim Blandy :jimb]

I think adding a "pauseOnException":true property to the "type":"resume" packet is a better way to go here.

This approach avoids adding more long-lived server-side state. Certainly, we need to establish an onExceptionUnwind handler until the next pause, but that's the same kind of handling required for the 'next' and 'step' operations, so the onExceptionUnwind handler addition/removal should just slot right in to that existing code. And doesn't require any additional packets to set/clear/query the state.

Having the client set a "pauseOnException" property on the resume packet keeps the modal state in the client, where the UI is, so that seems preferable.

Comment 10

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

Attachment: Patch v3

Changed the protocol to use the 'resume' packet for transferring the pauseOnExceptions flag and used a checkbox for the UI. Added lots of tests.

Comment 11

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

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=87ca368fc263

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 628418 [details] [diff] [review]
Patch v3

   /**
+   * Inform the debugger client whether the debuggee should be paused whenever
+   * an exception is thrown.
+   */
+  updatePauseOnExceptions: function SF_updatePauseOnExceptions(aFlag) {

should have a @param entry. (I know...)

+        // Special additions to the innermost scope.
+        if (env == frame.environment) {
+          // Add any thrown exception.
+          if (aDepth == 0 && this.exception) {
+            let excVar = scope.addVar("<exception>");
+            if (typeof this.exception == "object") {

this is getting pretty nesty. OK.

Can this.exception be of any type other than "object"? Presumably since you added an "else".

tested this out locally and it works well, at least for my fairly limited test code.

Comment 13

[Rob Campbell [:rc] (:robcee)]

one other thing: we're not saving the state of that checkbox. We might want to do that in a follow-up.

Comment 14

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

Attachment: Patch v4

(In reply to Rob Campbell [:rc] (:robcee) from comment #12)
>    /**
> +   * Inform the debugger client whether the debuggee should be paused
> whenever
> +   * an exception is thrown.
> +   */
> +  updatePauseOnExceptions: function SF_updatePauseOnExceptions(aFlag) {
> 
> should have a @param entry. (I know...)

Added.

> +        // Special additions to the innermost scope.
> +        if (env == frame.environment) {
> +          // Add any thrown exception.
> +          if (aDepth == 0 && this.exception) {
> +            let excVar = scope.addVar("<exception>");
> +            if (typeof this.exception == "object") {
> 
> this is getting pretty nesty. OK.
> 
> Can this.exception be of any type other than "object"? Presumably since you
> added an "else".

Yes, this is the case for 'throw "error";' or 'throw 42;'.

(In reply to Rob Campbell [:rc] (:robcee) from comment #13)
> one other thing: we're not saving the state of that checkbox. We might want
> to do that in a follow-up.

Indeed, filed bug 760564.

Comment 15

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=10460200df94

Comment 16

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 629217 [details] [diff] [review]
Patch v4

+  pauseOnExceptions: function TC_pauseOnExceptions(aFlag, aOnResponse) {
+    this._pauseOnExceptions = aFlag;
+    // If the debuggee is paused, the value of the flag will be communicated in
+    // the next resumption. Otherwise we have to force a pause in order to send
+    // the flag.

good, as per jimb's request in irc, I think.

Comment 17

[Dave Camp (:dcamp)]

Try run failed with:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pause-exceptions.js | Should have the right property name for the exception. - Got this, expected <exception>
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_pause-exceptions.js | Should have the right property value for the exception. - Got [object HTMLButtonElement], expected [object Error]

Comment 18

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

Attachment: Patch v5

I think this should fix the test failures. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=b83d9cd7db3e

Comment 19

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

Attachment: Patch v6

The previous change wasn't enough, but I'm confident about this one:
https://tbpl.mozilla.org/?tree=Try&rev=ffbdf53b8eee

The patches I've been sending to try have been showing a decreasing failure rate in an increasing number of runs:
50%
21%
14%

Maybe this one will get us to 0%.

Comment 20

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

Attachment: Patch v7

OK, fine, the previous version didn't make a dent (16% failure rate, only in debug builds), but this one seems to have fixed it locally.

The problem was that I was clearing the exception in SF__afterFramesCleared which is triggered by a timeout, after the 'framescleared' event is received. In the test, we get in some cases SF__afterFramesCleared called from the previous resumption to happen after the 'framesadded' event for the next pause happened. Instead of just tweaking the test to accommodate that case, I moved the clearing of the stored exception to happen in the 'framescleared' handler, not the timeout event handler it triggers.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2f710b7ac280

Comment 21

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

Attachment: v6-v7 diff

This is the actual change for easier review. Victor can you verify?

Comment 22

[Victor Porof [:vporof][:vp]]

Comment on attachment 629585 [details] [diff] [review]
v6-v7 diff

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

Lol. Ok!

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 629584 [details] [diff] [review]
Patch v7

sequencing is hard.

Comment 24

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

https://hg.mozilla.org/integration/fx-team/rev/e60ac3f6119d

Comment 25

[Rob Campbell [:rc] (:robcee)]

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