937172 - Use the ChildDebuggerTransport for the Debug Protocol on Desktop when e10s is enabled

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

We currently have local and remote. We should add a Message Manager option to the transport for working across processes with e10s.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The webapps actor[1] uses message manager to transport Dev Tools messages on B2G, but this might not be what you are looking for.

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#809

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to J. Ryan Stinnett [:jryans] from comment #1)
> The webapps actor[1] uses message manager to transport Dev Tools messages on
> B2G, but this might not be what you are looking for.
> 
> [1]:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webapps.js#809

I think the goal of this bug is to repurpose that code so that it works on desktop as well.

Comment 3

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

yeah, aiui, we just need to make the existing desktop devtools detect if we're in an e10s environment and if so, use the ChildDebuggerTransport.

I'll reword the subject of this accordingly.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: debug-progress-listener

This patch changes the kind of progress listener used by DebuggerProgressListener. It now will run in the child process, so it's not allowed to use the tabbrowser's progress listener. It's fine if it just uses the debuggee window's progress listener. This patch also fixes the problem in bug 978476.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: prefix-change

This patch changes the connection prefix used to talk to the child process so that it's allocated in the parent rather than the child. As far as I understand, the only requirement on this prefix is that it be unique within the connection. If we use allocID, I think that requirement will be satisfied. I needed to make this change because there's no concept of an appID in the desktop browser--it's always 0.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: connect-to-child

This patch moves the code that establishes the connection with the child into DebuggerServer. I want to be able to use it in the webbrowser.js actors as well as webapps.js.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-content-actor

This patch renames ContentAppActor to ContentActor. I want to use it for e10s browser tabs, so the App name no longer applies.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remote-browser-tab

This patch splits BrowserTabActor into two classes. There's a base class, TabActor, that contains most of the code. BrowserTabActor inherits from it and just contains code specific to in-process (non-e10s) <browser> elements.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rearrange-tab-actor

This patch cleans up the split between ContentActor and BrowserTabActor. They both have TabActor as a base class. The subclasses just implement their own versions of the docShell and window getters, since these are the only parts that are different between in-process and out-of-process actors.

The main problem that this patch fixes is that ContentActor was always using the content script's |content| value for the window object. I guess that probably works for apps, but browser apps change windows whenever you navigate, and so the value of |content| changes. The old code just used the value of |content| when the actor was created, so it didn't work if you navigated. This patch fixes navigation.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remote-browser-tab

This patch creates a shim object, RemoteBrowserTabActor, in the parent process that "looks like" a BrowserTabActor. Really it just exists to remember the actor ID of the ContentActor in the child process. This patch also changes listTabs to return RemoteBrowserTabActor for out-of-process tabs.

With this patch, basic devtools functionality (web console, inspector, debugger) appear to work in e10s.

Comment 11

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

unassigning myself from this.

I'll probably assign these reviews to some others who are more familiar with our server code. Alex Poirot was looking at unifying some of the child process debugging. Tagging him to reply here...

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385075 [details] [diff] [review]
connect-to-child

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

That patch is bug 962577, would you mind rebasing on top of it?
I was already planning to factor out that method for e10s firefox...
We discussed with panos offline and got a offline f+ on that patch,
so we should be able to land it shortly.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: prefix-change v2

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: connect-to-child v2

This patch just renames connectToApp to connectToChild.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remote-browser-tab v2

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-tab-actor

Comment 17

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

Bill, can you tag Panos Astithas for review as well? He needs to review server changes. Thanks!

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: prefix-change v3

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry for all the churn. I did the rebase and also realized that one of the patches I uploaded yesterday was a different one than I intended. I think it should all be fixed now. The order of the patches is:

82508f3c2088 (base changeset)
refactor-webapps-actors-connectToApp (from bug 962577)
debug-progress-listener
prefix-change
connect-to-child
rename-content-actor
rename-tab-actor
rearrange-tab-actor
remote-browser-tab

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385071 [details] [diff] [review]
debug-progress-listener

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

Note that the DebuggerProgressListener modification will conflict with bug 977043 I'm working on, but you will most likely land first.

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385076 [details] [diff] [review]
rename-content-actor

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

::: toolkit/devtools/server/main.js
@@ +364,5 @@
>      if (!("BrowserTabActor" in this)) {
>        this.addActors("resource://gre/modules/devtools/server/actors/webbrowser.js");
>        this.addTabActors();
>      }
> +    if (!("ContentActor" in DebuggerServer)) {

I think you will have to rebase this, we should have landed a patch that renamed DebuggerServer to this.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385080 [details] [diff] [review]
rearrange-tab-actor

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

That patch also conflict with bug 977043, but introduce some hopefully adressable complexity.

In 977043, I'm implementing iframe selection at toolbox level, so that the same TabActor
instance can target all child iframe on demand.
So far, it was really easy as all BrowserTabActor sub classes depends on `_browser`,
which, is everywhere considered either as a <xul:browser> or a content window object.
Here BrowserTabActor and ContentActor have a very different implementation and
each requires multiple internal variables to be set.
BrowserTabActor would require to modify _chromeEventHandler as well as _browser,
whereas ContentActor would require _chromeEventHandler and _chromeGlobal.
And in both cases, when switching to an iframe I have no chrome content script global,
nor a xul:browser. I can pass the <iframe>, I think it would work, but it sounds weak.

Instead of requiring to implement a docshell attribute on subclasses,
what about replacing the chromeEventHandler TabActor argument with docshell?
And then pull everything out of the docshell, even the `window` and chromeEventHandler objects.
webProgress.DOMWindow, docShell.chromeEventHandler (hopefully it will work in child process...)

Then for the frame selection patch, I can only modify the tab actor's docshell, which appear to be the root object to designate a precise document!

Otherwise there is some exception because of exit methods, but it looks good.
I can always figure out something on top of your patch for frames selection,
but that would be great to refactor this code only once!

::: toolkit/devtools/server/actors/childtab.js
@@ +45,5 @@
> +  enumerable: true,
> +  configurable: false
> +});
> +
> +Object.defineProperty(ContentActor.prototype, "exit", {

Same thing here, exit is a method.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +988,5 @@
> +  enumerable: true,
> +  configurable: false
> +});
> +
> +Object.defineProperty(BrowserTabActor.prototype, "exit", {

exit is a method so we get an exception.
There is no need to use Object.defineProperty for functions, we can do:
BrowserTabActor.prototype.exit = function () {
  TabActor.prototype.exit.call(this);
  ...
}

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385552 [details] [diff] [review]
connect-to-child v2

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

Let Jan include this modification in bug 962577. Actually, I think he already did.

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385554 [details] [diff] [review]
remote-browser-tab v2

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

I don't really know this code, so I'll fully defer to Panos on this one ;)

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 8385557 [details] [diff] [review]
rename-tab-actor

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

I imagine we would also rename BrowserTabActor occurrences from this file also:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/root.js#14
(As well as BrowserRootActor which doesn't exists anymore, I'd imagine we refer to RootActor now)

Comment 26

[Alexandre Poirot [:ochameau]]

Thanks Bill, that patch looks great and simple!
That's fun, because that's exactly what I had in mind for adding e10s support. I talked to Rob and Panos same weeks ago and opened bug 962577 to start sharing code between b2g and firefox e10s...

FYI, bug 917227, is going to provide support for the network panel on b2g.
But unfortunately, it will only work for apps, as it filters by appId.
If you have some thoughts as you have more e10s background, feel free to join 917227 conversation!

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think it would be hard to switch to using docShell for everything.

Comment 28

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

I've played with the patches looking for regressions on desktop and Fennec (assuming Alex tested on B2G), and the only problem I encountered was with the toolbox highlighter in a content page, which produced this error:

console.error:
  Message: TypeError: target is undefined
  Stack:
    HighlighterActor<._startPickerListeners@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:204:5
HighlighterActor<.pick<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:170:5
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:906:1
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1095:9
LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:258:11
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80:7

This happens when clicking on the highlighter icon and is always reproducible for me. I'm not entirely sure this is a bug with one of your patches though, as I had to rebase a few of them (and might have botched it) and it might also be an unrelated bug that existed in the 82508f3c2088 base changeset that I've used. Just make sure to retest with the updated patch(es) or ping me to do it.

Comment 29

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

Comment on attachment 8385071 [details] [diff] [review]
debug-progress-listener

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

LGTM.

Comment 30

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

Comment on attachment 8385076 [details] [diff] [review]
rename-content-actor

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

LGTM.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fixes2

This patch fixes the exception you saw. I guess the highlighter requires the actual <browser> element in order to use the BoxModelHighlighter. That means that highlighting in e10s won't look as nice because it will use the SimpleOutlineHighlighter, but I guess we can fix that later.

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fixes2 v2

This needed a little more work to work with e10s.

Comment 33

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

Comment on attachment 8385565 [details] [diff] [review]
prefix-change v3

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

I think this is OK.

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just an update about comment 22: I was able to use this window getter inside TabActor:

   get window() {
    return this.docShell
      .QueryInterface(Ci.nsIInterfaceRequestor)
      .getInterface(Ci.nsIDOMWindow);
  },

That way the subclasses don't have to provide it. Unfortunately, we can't use docShell.chromeEventHandler, since it's always null in e10s. So I had to leave the _chromeEventHandler field around. Not sure how this affects your patch, Alex.

Comment 35

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

Comment on attachment 8385557 [details] [diff] [review]
rename-tab-actor

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

LGTM.

Comment 36

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

Comment on attachment 8385080 [details] [diff] [review]
rearrange-tab-actor

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

::: toolkit/devtools/server/actors/childtab.js
@@ +6,5 @@
>  
>  /**
>   * Tab actor for documents living in a child process.
>   *
> + * Depends on TabActor, defined in webbrowser.js actor.

A nit while you are here: "defined in webbrowser.js." (it's a file not an actor).

@@ +48,5 @@
> +
> +Object.defineProperty(ContentActor.prototype, "exit", {
> +  get: function() {
> +    TabActor.prototype.exit.call(this);
> +    this._chromeGlobal = null;

There are 3 paths to tearing down a tab actor:
1. the root actor calls tabActor.exit() when it receives a TabClosed event or similar
2. actor.disconnect() is called for every actor by the framework when the client close the connection
3. actor.onDetach() is called from a client that explicitly wants to detach from a specific tab

This is the "mess" that bug 710213 refers to.

In order to avoid leaking the _chromeGlobal, you will need to clear it in all those cases, which is precisely the reason the private _detach method exists. Now inheritance makes this hairier, but I think we should accept reality and just override _detach (regardless if you prefer to drop the underscore or not).

::: toolkit/devtools/server/actors/webbrowser.js
@@ -680,5 @@
>        return false;
>      }
>  
> -    if (this._progressListener) {
> -      this._progressListener.destroy();

I remember having added this seemingly useless check a couple of years ago (bug 751949) due to errors in tests. We have probably fixed those races in the Grand Debugger Test Rewrite, but let's keep it in mind in case it surfaces again.

Comment 37

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

Comment on attachment 8385554 [details] [diff] [review]
remote-browser-tab v2

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

Love it, so much cleaner than the approach we use for apps!

::: toolkit/devtools/server/actors/webbrowser.js
@@ +1036,5 @@
> +    return this._form;
> +  },
> +
> +  exit: function() {
> +    this._browser = null;

Again, we have to clear the _browser reference in all of [exit, disconnect, onDetach], so I suggest to override _detach for that purpose.

Comment 38

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

Comment on attachment 8386959 [details] [diff] [review]
fixes2 v2

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

Makes sense.

Comment 39

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

I discovered another bug with this patch:
1.Open the browser with at least 2 tabs.
2. Open the web console in the last tab.
3. Close another tab:

Handler function BrowserTabList.prototype.handleEvent threw an exception: TypeError: aActor.exit is not a function
Stack: BrowserTabList.prototype._handleActorClose@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:307:3
BrowserTabList.prototype.handleEvent<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:384:7
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:80:7
Line: 307, column: 2

Comment 40

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

Actually, that seems to be the error that Alex was referring to in comment 22.

Comment 41

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/11209dad5892
remote:   https://hg.mozilla.org/integration/fx-team/rev/7d9a9a8d653b
remote:   https://hg.mozilla.org/integration/fx-team/rev/6d99499b74b6
remote:   https://hg.mozilla.org/integration/fx-team/rev/36f64670a41f
remote:   https://hg.mozilla.org/integration/fx-team/rev/bdc2b431ba22
remote:   https://hg.mozilla.org/integration/fx-team/rev/8f90256a56e8

I wasn't able to make the change to null stuff out in detach. It looks like it's possible to detach and then attach again, and in that case we don't want to null anything out. I saw this in browser/devtools/debugger/test/browser_dbg_listtabs-01.js and a few other tests.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11209dad5892
https://hg.mozilla.org/mozilla-central/rev/7d9a9a8d653b
https://hg.mozilla.org/mozilla-central/rev/6d99499b74b6
https://hg.mozilla.org/mozilla-central/rev/36f64670a41f
https://hg.mozilla.org/mozilla-central/rev/bdc2b431ba22
https://hg.mozilla.org/mozilla-central/rev/8f90256a56e8

Comment 43

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

(In reply to Bill McCloskey (:billm) from comment #41)
> I wasn't able to make the change to null stuff out in detach. It looks like
> it's possible to detach and then attach again, and in that case we don't
> want to null anything out. I saw this in
> browser/devtools/debugger/test/browser_dbg_listtabs-01.js and a few other
> tests.

Oh right, I forgot about that, sorry! disconnect() should clear the reference though, that was my main concern, as it is implicitly called on connection teardown. I'll file a followup for that.

Comment 44

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

just tested this out and was able to launch the inspector and debugger and dig around in a webpage. Nicely done! Thanks for the patches.