725360 - Closing the tab when the debugger is paused should cleanly exit the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

Currently the debugger doesn't handle gracefully being closed while in a paused state. One idea would be to resume in the TabClose handler, after disabling all debugger hooks and then close the debugger and tab.

Comment 1

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

Attachment: [in-fx-team] Working patch

This patch does the trick.

Comment 2

[Dave Camp (:dcamp)]

Comment on attachment 597079 [details] [diff] [review]
[in-fx-team] Working patch

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +123,5 @@
> +    if (this._state == "paused") {
> +      // Disable debugger hooks and then resume, in order to guarantee there
> +      // will be no more pauses until actor exit.
> +      this.dbg.enabled = false;
> +      this.onResume();

We set this.dbg.enabled to false a few lines down, is that redundant/should it be changed?

Comment 3

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

(In reply to Dave Camp (:dcamp) from comment #2)
> Comment on attachment 597079 [details] [diff] [review]
> Working patch
> 
> Review of attachment 597079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +123,5 @@
> > +    if (this._state == "paused") {
> > +      // Disable debugger hooks and then resume, in order to guarantee there
> > +      // will be no more pauses until actor exit.
> > +      this.dbg.enabled = false;
> > +      this.onResume();
> 
> We set this.dbg.enabled to false a few lines down, is that redundant/should
> it be changed?

I was worried about getting paused again between resuming and disabling debugging, but now that I rethink of it, I don't see a way for the debugger to be preempted in this code path.

Comment 4

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

Comment on attachment 597079 [details] [diff] [review]
[in-fx-team] Working patch

+      this.dbg.enabled = false;
+      this.onResume();
+    }

I guess we don't care about the resume packet here?

Comment 5

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

(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> Comment on attachment 597079 [details] [diff] [review]
> Working patch
> 
> +      this.dbg.enabled = false;
> +      this.onResume();
> +    }
> 
> I guess we don't care about the resume packet here?

Right, this is a forced resumption. Also, callers that are protocol request handlers, like onDetach, respond with their own return packet anyway.

Comment 6

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

Landed in fx-team with the redundant line removed, as suggested by Dave:

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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