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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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 (he/him) [:past] (please ni?)]

Different tabs can no longer have the debugger frontend open, but different windows can.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

We should extend the approach in bug 753311 to cover different windows as well.

Comment 3

[Victor Porof [:vporof][:vp]]

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]]

(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 (he/him) [:past] (please ni?)]

(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]]

(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]]

This bug is 6 months old - why should we track it for release?

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(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]]

(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

[(no longer active)]

FWIW, I don't like this fix at all.  This is a bad limitation implied by our implementation.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(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]]

Attachment: v1

Fun.

Comment 13

[Alex Keybl [:akeybl]]

(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]]

(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]]

Bugzilla!!

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

Attachment: v2

Addressed comments.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/6cbfdf764d22

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/6cbfdf764d22

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

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 21

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43a9a6e3e68f

Comment 22

[Ioana (away)]

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