Last Comment Bug 710258 - Don't allow the debugger to be open in more than one window
: Don't allow the debugger to be open in more than one window
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 16
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 753311
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-13 09:27 PST by Panos Astithas [:past]
Modified: 2012-08-03 07:11 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
v1 (21.25 KB, patch)
2012-06-19 10:19 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Review
v2 (21.52 KB, patch)
2012-06-20 05:22 PDT, Victor Porof [:vporof][:vp]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Panos Astithas [:past] 2011-12-13 09:27:52 PST
Debugging two tabs in parallel is a use case that we should support. The problem probably stems from the fact that the single debugger server has a single, non-namespaced, script cache for all its clients. Fixing bug 706506 might resolve this as well.
Comment 1 Panos Astithas [:past] 2012-06-05 06:22:57 PDT
Different tabs can no longer have the debugger frontend open, but different windows can.
Comment 2 Panos Astithas [:past] 2012-06-05 06:42:30 PDT
We should extend the approach in bug 753311 to cover different windows as well.
Comment 3 Victor Porof [:vporof][:vp] 2012-06-16 10:11:03 PDT
Heh, this is a bit tricky because every new chrome window will have its own instance of a DebuggerUI. Panos, would using a pref be ok?
Comment 4 Victor Porof [:vporof][:vp] 2012-06-16 16:06:37 PDT
(In reply to Victor Porof from comment #3)

Although I hate this idea and I don't want to go this route.
Comment 5 Panos Astithas [:past] 2012-06-18 08:49:38 PDT
(In reply to Victor Porof from comment #4)
> (In reply to Victor Porof from comment #3)
> 
> Although I hate this idea and I don't want to go this route.

Agreed, although I probably wouldn't object strongly to a pref. How about iterating over the windows and checking all DebuggerUI instances?
Comment 6 Victor Porof [:vporof][:vp] 2012-06-18 12:50:11 PDT
(In reply to Panos Astithas [:past] from comment #5)
> (In reply to Victor Porof from comment #4)
> > (In reply to Victor Porof from comment #3)
> > 
> > Although I hate this idea and I don't want to go this route.
> 
> Agreed, although I probably wouldn't object strongly to a pref. How about
> iterating over the windows and checking all DebuggerUI instances?

Still ugly, but worth a try. Should be ok, but I really don't fancy window mediator.
Comment 7 Alex Keybl [:akeybl] 2012-06-18 16:09:55 PDT
This bug is 6 months old - why should we track it for release?
Comment 8 Panos Astithas [:past] 2012-06-19 00:08:43 PDT
(In reply to Alex Keybl [:akeybl] from comment #7)
> This bug is 6 months old - why should we track it for release?

I sort of re-purposed it in comment 1, since the original issue has been covered by bug 753311. It's a serious issue, in the sense that users who open multiple debuggers in different windows can be surprised by the behavior of the nested event loops (two paused debuggees must be resumed in reverse order). This bug will make sure that the user cannot open a second debugger when one is open in another window.
Comment 9 Victor Porof [:vporof][:vp] 2012-06-19 08:18:46 PDT
(In reply to Panos Astithas [:past] from comment #8)
> (In reply to Alex Keybl [:akeybl] from comment #7)

Since it's re-purposed, maybe we should rename this bug to "Don't allow the debugger to be open in more than one window"?
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-19 09:12:25 PDT
FWIW, I don't like this fix at all.  This is a bad limitation implied by our implementation.
Comment 11 Panos Astithas [:past] 2012-06-19 09:25:50 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> FWIW, I don't like this fix at all.  This is a bad limitation implied by our
> implementation.

Well, yes and no. Yes, it's more limiting than what Firebug does, for example, but it's a limitation imposed to protect the inexperienced developer from the subtleties of nested event loops. I think the prevailing opinion in the team right now is to allow multiple open debuggers at some point, but disallow resumption in a bad order, like Firebug. It's just that this would be more work than we can currently handle, and we don't want to ship the debugger with this bug present.
Comment 12 Victor Porof [:vporof][:vp] 2012-06-19 10:19:06 PDT
Created attachment 634487 [details] [diff] [review]
v1

Fun.
Comment 13 Alex Keybl [:akeybl] 2012-06-19 11:23:17 PDT
(In reply to Panos Astithas [:past] from comment #8)
> (In reply to Alex Keybl [:akeybl] from comment #7)
> > This bug is 6 months old - why should we track it for release?
> 
> I sort of re-purposed it in comment 1, since the original issue has been
> covered by bug 753311. It's a serious issue, in the sense that users who
> open multiple debuggers in different windows can be surprised by the
> behavior of the nested event loops (two paused debuggees must be resumed in
> reverse order). This bug will make sure that the user cannot open a second
> debugger when one is open in another window.

OK - we'll track for release since the debugger is new to FF15. In the future, however, known issues critical enough to track for release should be resolved while that version of FF is on m-c.
Comment 14 Victor Porof [:vporof][:vp] 2012-06-20 02:53:55 PDT
(In reply to Victor Porof from comment #12)
> Created attachment 634487 [details] [diff] [review]
> v1
> 
> Fun.

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=68ad6ee2de5d
Comment 15 Victor Porof [:vporof][:vp] 2012-06-20 02:55:23 PDT
Bugzilla!!
Comment 16 Panos Astithas [:past] 2012-06-20 05:13:03 PDT
Comment on attachment 634487 [details] [diff] [review]
v1

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

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +53,4 @@
>     * @return DebuggerPane if the debugger is started, null if it's stopped.
>     */
>    toggleDebugger: function DUI_toggleDebugger() {
> +    let scriptDebugger = this.getDebuggerIn("navigator:browser");

Why did you choose to encumber callers with specifying the window type instead of using navigator:browser inside getDebuggerIn? I can't see any call sites with a different value.

@@ +102,5 @@
> +   *        The windows to check for a script debugger.
> +   * @return DebuggerPane | null
> +   *         The script debugger instance if it exists, null otherwise.
> +   */
> +  getDebuggerIn: function DUI_getDebuggerIn(aWindowType) {

I think findDebugger might be a better name here, the dangling In sounds funny.

::: browser/devtools/debugger/test/browser_dbg_debugger-tab-switch-window.js
@@ +8,5 @@
> +let gInitialWindow, gSecondWindow;
> +let gPane1, gPane2;
> +let gNbox;
> +
> +function test() {

Nit: a short comment to describe the purpose of this test, please.
Comment 17 Victor Porof [:vporof][:vp] 2012-06-20 05:22:07 PDT
Created attachment 634858 [details] [diff] [review]
v2

Addressed comments.
Comment 18 Panos Astithas [:past] 2012-06-20 06:50:59 PDT
https://hg.mozilla.org/integration/fx-team/rev/6cbfdf764d22
Comment 19 Panos Astithas [:past] 2012-06-21 01:06:30 PDT
https://hg.mozilla.org/mozilla-central/rev/6cbfdf764d22
Comment 20 Panos Astithas [:past] 2012-06-22 03:31:33 PDT
Comment on attachment 634858 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: developers will be able to open debuggers in separate browser windows, which can lead to confusing behavior regarding resumption from pauses. See also comment 8.
Testing completed (on m-c, etc.): On m-c and fx-team
Risk to taking this patch (and alternatives if risky): it's a small patch (besides tests) with a straight-forward refactoring, in a developer-only feature
String or UUID changes made by this patch: none
Comment 22 Ioana (away) 2012-08-03 07:10:47 PDT
Verified as fixed on:
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0

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