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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Created attachment 597079 [details] [diff] [review]
[in-fx-team] Working patch

This patch does the trick.
Assignee: nobody → past
Status: NEW → ASSIGNED
Attachment #597079 - Flags: review?(rcampbell)

Comment 2

6 years ago
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?
(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 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?
Attachment #597079 - Flags: review?(rcampbell) → review+
(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.
Landed in fx-team with the redundant line removed, as suggested by Dave:

https://hg.mozilla.org/integration/fx-team/rev/d08878c56f15
Whiteboard: [fixed-in-fx-team]
Attachment #597079 - Attachment description: Working patch → [in-fx-team] Working patch
https://hg.mozilla.org/mozilla-central/rev/d08878c56f15
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13

Updated

6 years ago
Depends on: 728606
You need to log in before you can comment on or make changes to this bug.