Closed Bug 710258 Opened 10 years ago Closed 10 years ago

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

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox15 verified)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox15 --- verified

People

(Reporter: past, Assigned: vporof)

References

Details

Attachments

(1 file, 1 obsolete file)

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: minotaur
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.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
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?
(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?
(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.
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.
(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.
Attached patch v1 (obsolete) — Splinter Review
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.
(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
Bugzilla!!
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+
Attached patch v2Splinter Review
Addressed comments.
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/6cbfdf764d22
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6cbfdf764d22
Status: ASSIGNED → RESOLVED
Closed: 10 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?
Attachment #634858 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.