Last Comment Bug 725360 - Closing the tab when the debugger is paused should cleanly exit the debugger
: Closing the tab when the debugger is paused should cleanly exit the debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on: 728606
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-08 08:25 PST by Panos Astithas [:past]
Modified: 2012-02-18 14:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[in-fx-team] Working patch (3.37 KB, patch)
2012-02-14 10:29 PST, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Review

Description Panos Astithas [:past] 2012-02-08 08:25:54 PST
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 [:past] 2012-02-14 10:29:39 PST
Created attachment 597079 [details] [diff] [review]
[in-fx-team] Working patch

This patch does the trick.
Comment 2 Dave Camp (:dcamp) 2012-02-14 12:14:39 PST
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 [:past] 2012-02-15 02:58:10 PST
(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) 2012-02-16 12:43:12 PST
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 [:past] 2012-02-16 23:34:17 PST
(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 [:past] 2012-02-17 00:20:43 PST
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] 2012-02-17 17:07:56 PST
https://hg.mozilla.org/mozilla-central/rev/d08878c56f15

Note You need to log in before you can comment on or make changes to this bug.