Don't allow the debugger to be open in more than one window

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: vporof)

Tracking

Trunk
Firefox 16
Points:
---

Firefox Tracking Flags

(firefox15 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Priority: -- → P3
Depends on: 753311
No longer blocks: 676586
Priority: P3 → P1
Summary: When open in two tabs, the debugger UI shows the scripts of both tabs in each page → When open in two windows, the debugger UI shows the scripts of both pages
Different tabs can no longer have the debugger frontend open, but different windows can.
We should extend the approach in bug 753311 to cover different windows as well.
tracking-firefox15: --- → ?
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Comment 4

5 years ago
(In reply to Victor Porof from comment #3)

Although I hate this idea and I don't want to go this route.
(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?
(Assignee)

Comment 6

5 years ago
(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

5 years ago
This bug is 6 months old - why should we track it for release?
(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.
(Assignee)

Comment 9

5 years ago
(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"?
Summary: When open in two windows, the debugger UI shows the scripts of both pages → Don't allow the debugger to be open in more than one window
FWIW, I don't like this fix at all.  This is a bad limitation implied by our implementation.
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 634487 [details] [diff] [review]
v1

Fun.
Attachment #634487 - Flags: review?(past)
(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.
tracking-firefox15: ? → +
(Assignee)

Comment 14

5 years ago
(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
tracking-firefox15: + → ?
(Assignee)

Comment 15

5 years ago
Bugzilla!!
tracking-firefox15: ? → ---
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.
Attachment #634487 - Flags: review?(past) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 634858 [details] [diff] [review]
v2

Addressed comments.
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/6cbfdf764d22
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
tracking-firefox15: --- → +
https://hg.mozilla.org/mozilla-central/rev/6cbfdf764d22
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
Attachment #634487 - Attachment is obsolete: true
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
Attachment #634858 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #634858 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/43a9a6e3e68f
status-firefox15: --- → fixed
tracking-firefox15: + → ---

Comment 22

5 years ago
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

Updated

5 years ago
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.