816569 - Browser Debugger should freeze debuggee execution when paused

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

Content debugging freezes page execution while debugging, but chrome debugging does not. We should fix that.

Comment 1

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

Attachment: Patch v1

I finally figured out the right incantations. This freezes all browser windows, not just the current one, but doesn't freeze other kinds of windows, like the Error Console for example. I wish I knew how to do that.

Comment 2

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

Comment on attachment 686648 [details] [diff] [review]
Patch v1

It suffers from the same problem we found in bug 808527. I need to look into it some more.

Comment 3

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

Attachment: Patch v2

Fixed the getter problems from bug 808527. Now the STR in that bug don't add any unexpected frames to the stack. I had to pretty much inline the selectedBrowser getter from tabbrowser.xml and the contentWindow getter from browser.xml.

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 687046 [details] [diff] [review]
Patch v2

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

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +240,5 @@
>     * Prepare to enter a nested event loop by disabling debuggee events.
>     */
>    preNest: function BRA_preNest() {
> +    // Disable events in all open browser windows.
> +    let e = windowMediator.getEnumerator("navigator:browser");

you mention that this won't pause non-browser windows in a comment. If you're interested in pausing Error Consoles, Scratchpads, Download Managers, etc, you might consider being greedier and passing in null to getEnumerator.

Then again, this is dbg-browser-actors.js. Maybe we'd be better off implementing dbg-global-actors.js if we wanted to go all-in.

@@ +259,5 @@
> +      let windowUtils = chromeWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIDOMWindowUtils);
> +      windowUtils.suppressEventHandling(true);
> +      windowUtils.suspendTimeouts();
> +    }

I wrote a bunch of stuff in this method trying to pick apart why you have to QI to the moon to make this work. Then I started pulling out pieces. It's Rather Amazing! I'm a little shocked you need to start this process inside the content window.

It seems to work, but I'd like someone who knows the inner workings of suppressEventHandling to weigh in on it.

@@ +285,5 @@
> +                          .chromeEventHandler.ownerDocument.defaultView;
> +      let windowUtils = chromeWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIDOMWindowUtils);
> +      windowUtils.resumeTimeouts();
> +      windowUtils.suppressEventHandling(false);

ditto.

Comment 5

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> ::: toolkit/devtools/debugger/server/dbg-browser-actors.js
> @@ +240,5 @@
> >     * Prepare to enter a nested event loop by disabling debuggee events.
> >     */
> >    preNest: function BRA_preNest() {
> > +    // Disable events in all open browser windows.
> > +    let e = windowMediator.getEnumerator("navigator:browser");
> 
> you mention that this won't pause non-browser windows in a comment. If
> you're interested in pausing Error Consoles, Scratchpads, Download Managers,
> etc, you might consider being greedier and passing in null to getEnumerator.
>
> Then again, this is dbg-browser-actors.js. Maybe we'd be better off
> implementing dbg-global-actors.js if we wanted to go all-in.

I tried it, but other windows don't have a gBrowser property (or a getBrowser() method) and this strategy breaks. I would like to make this part apply more broadly, but I don't know how! (From a terminology standpoint, dbg-browser-actors was meant to convey the pertinence of these actors to the browser application as a whole).

> @@ +259,5 @@
> > +      let windowUtils = chromeWin.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                 .getInterface(Ci.nsIDOMWindowUtils);
> > +      windowUtils.suppressEventHandling(true);
> > +      windowUtils.suspendTimeouts();
> > +    }
> 
> I wrote a bunch of stuff in this method trying to pick apart why you have to
> QI to the moon to make this work. Then I started pulling out pieces. It's
> Rather Amazing! I'm a little shocked you need to start this process inside
> the content window.
> 
> It seems to work, but I'd like someone who knows the inner workings of
> suppressEventHandling to weigh in on it.
> 
> @@ +285,5 @@
> > +                          .chromeEventHandler.ownerDocument.defaultView;
> > +      let windowUtils = chromeWin.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                 .getInterface(Ci.nsIDOMWindowUtils);
> > +      windowUtils.resumeTimeouts();
> > +      windowUtils.suppressEventHandling(false);
> 
> ditto.

Agreed.

Comment 6

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

Comment on attachment 687046 [details] [diff] [review]
Patch v2

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

I'll go straight to the experts.

Boris, is there a shorter way of getting a reference to nsIDOMWindowUtils to freeze the chrome window while debugging the browser?

Gavin, is there a way to grab gBrowser from Firefox windows that don't have that property set (like the Error Console, View Source, Scratchpad, etc.)?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Boris, is there a shorter way of getting a reference to nsIDOMWindowUtils

Looking at the patch, you're enumerating the window mediator, right?  That's handing you the nsIDOMWindow, I believe.  So why does this not work?

  let win = e.getNext();
  let windowUtils = win.QueryInterface(Ci.nsIInterfaceRequestor)
                       .getInterface(Ci.nsIDOMWindowUtils);

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I think Boris' snippet is what you want, assuming the intent is to actually suppress event handling in all child docshells of all chrome windows. That way you don't need gBrowser at all.

But that seems like a scary thing to do, and makes me wonder what odd side effects it could have :)

Comment 9

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

Thank you both, that's exactly what I needed!

(In reply to Boris Zbarsky (:bz) from comment #7)
> Looking at the patch, you're enumerating the window mediator, right?  That's
> handing you the nsIDOMWindow, I believe.  So why does this not work?
> 
>   let win = e.getNext();
>   let windowUtils = win.QueryInterface(Ci.nsIInterfaceRequestor)
>                        .getInterface(Ci.nsIDOMWindowUtils);

You'd think that this would have been one of the things I'd tried, but I'm dumber than that. I could swear that this was the first thing I tried though.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I think Boris' snippet is what you want, assuming the intent is to actually
> suppress event handling in all child docshells of all chrome windows. That
> way you don't need gBrowser at all.
> 
> But that seems like a scary thing to do, and makes me wonder what odd side
> effects it could have :)

Indeed this is precisely what I was looking for and I can confirm that the Browser Debugger now freezes all kinds of windows (error console, downloads, scratchpad, view source, you name it) for inspection. There are still a few things that can run, like changing the scratchpad environment by selecting the other menu item, since I can't get to menus from the window mediator. I don't suppose there is a way to freeze menus, is there?

I must say I'm pretty happy as it is right now, though. All of the use cases I originally had in mind are now properly paused.

Comment 10

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

Attachment: Patch v3

As small as it gets.

Comment 11

[Rob Campbell [:rc] (:robcee)]

well I certainly feel better now.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687938 [details] [diff] [review]
Patch v3

r=me

Comment 13

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

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

Comment 14

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/e2ae419c5e7f