870128 - Pause on exceptions doesn't work after reload

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

Attachment: testcase.html

STR:

Browse to the test case page.

Open the debugger.

Enable "pause on exceptions".

Reload the page to rerun the script.

You should be paused at the exception in the test case.

Reload again.

Expected results: the debugger should be paused on the at the location of the exception like before.

Actual results: the debugger displays the source where the exception ocurred, but the location of the exception is not highlighted nor does the UI indicate that we are paused.

Comment 1

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

The problem is that the error is thrown while the debugger is disabled. I wonder if this is a regression from bug 792062. Looking into it.

Comment 2

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

Actually the problem turned out to be something entirely different.

The way pause-on-exception was implemented keeps the state of the setting in the client, transmitting it to the server on resume (see bug 757282 comment 9). It turns out that there is one case where the server state is modified without the client being informed beforehand: page navigation or reload. If the page was running, then it's OK, because the setting had already been transferred and the server is going to pause on any exception that occurs.

If however execution was paused, that means that the server cleared the onExceptionUnwind hook and only the client now knows that the setting is enabled. When the user reloads the page at this point, execution resumes without the hook in place, since the client wasn't the one who initiated the resumption. Therefore, until the next time the client sends a resume packet, the server will ignore any exception that is thrown.

Now I can see 2 ways to fix this:
1. by simply storing the state of the pref in the server, whenever a resumption packet is received
2. by using a configuration packet for setting this pref as proposed in bug 757282 comment 5

I prefer option 2, because it acknowledges the stateful nature of this operation and because we've already added a "reconfigure" protocol request to support source maps. It will also fix bug 788585, no matter how unlikely it is to occur.

Option 1 seems hacky, because it will use "resume" packets for toggling thread actor configuration, something no other property of the packet currently does.

Comment 3

[Jim Blandy :jimb]

I'm kind of shocked that reloading or navigating while paused effectively does a 'resume' --- content code is running, right? --- without giving the client a chance to get a handle on things before execution begins.

I haven't read this code, but:

If the client is paused when we reload/navigate, then that means we must be in a nested event loop, which we exit. That is a resumption, not caused by a 'resume' or 'detach' packet, right? If we're not going to carry the parameters from the last resume over to this resumption, then pretty much anything else designed like pause-on-exception is going to be screwed.

I think a configuration packet to the thread actor is probably fine.

Comment 4

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

I should note that the automatic resumption is required in order to maintain some invariants in gecko code (in nsIDOMWindow or nsDocShell IIRC). The code would assert when navigating away from a paused page.

While implementing pauseOnDOMEvents I kept thinking about this issue, as you can imagine, and I'm now less concerned about the hackiness I mentioned in comment 2. If persisting state is not just a peculiarity of pauseOnExceptions, but a more general design principle in our protocol, then using a configuration packet is less enticing. pauseOnExcpetions, pauseOnDOMEvents, pauseOnXHR, pauseOnReflow and probably others down the road will make this stateful operation much easier to reason about.

Comment 5

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

There is actually one case where a configuration packet is necessary, even if the state of the flag is persisted server-side:

- with the server configured to pause on exceptions and execution paused, disable pause-on-exceptions in the client
- if the next action is a manual resume, then everything is fine. If, however, the next action is a page reload, then the client cannot transfer the new state of the flag with a resume packet and therefore needs to use a configuration packet for that purpose.

Comment 6

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

Attachment: Patch v1

This works in all cases, but needs tests.

Comment 7

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

Attachment: Patch v2

Now with a test.

Comment 8

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=7ebc773fae3b

Comment 9

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

Another try run without extra logging: https://tbpl.mozilla.org/?tree=Try&rev=6dba44abe4f6

Comment 10

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

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

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

just a few comments. I read the bits in the comments about using reconfigure and Jim's concerns about not reconfiguring properly on a reload. I'm mostly concerned with the different paths in the case of using a resume packet to change the state of the pauseOnExceptions flag. Why the inconsistency?

::: browser/devtools/debugger/test/Makefile.in
@@ +144,4 @@
>  	test-location-changes-bp.js \
>  	test-location-changes-bp.html \
>  	test-step-out.html \
> +  test-pause-exceptions-reload.html \

missed tab?

::: toolkit/devtools/client/dbg-client.jsm
@@ +1109,5 @@
>    pauseOnExceptions: function TC_pauseOnExceptions(aFlag, aOnResponse) {
>      this._pauseOnExceptions = aFlag;
> +    // If the debuggee is paused, we have to send the flag via a reconfigure
> +    // request.
> +    if (this.paused) {

do we care if this.paused or not? Why not just send the reconfigure in either case?

::: toolkit/devtools/server/actors/webbrowser.js
@@ +746,4 @@
>          this.threadActor.clearDebuggees();
>          if (this.threadActor.dbg) {
>            this.threadActor.dbg.enabled = true;
> +          this.threadActor.maybePauseOnExceptions();

mm. uncertainty. :)

Comment 11

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

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

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

Panos explained the rationale behind keeping both mechanisms in the pauseOnExceptions method. I understand the point of it, but we could be a little better with our documentation around sending configuration changes along with resume packets.

Comment 12

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

Fixed the whitespace nit locally and I will sit down with Jim soon to figure out the best way to deal with such corner cases in the protocol.

Comment 13

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

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

Comment 14

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

Attachment: Update browser_dbg_pause-exceptions-reload.js for a post-bug-887516 world

That's what I get for not running tests again before pushing. Bug 887516 changed the way object classes are displayed, so I had to make a minor update to this test.

Comment 15

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

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3195f3721d50
https://hg.mozilla.org/mozilla-central/rev/718a1d8397d9