Closed Bug 937172 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: rcampbell, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 6 obsolete files)

3.82 KB, patch
ochameau
: review+
past
: review+
Details | Diff | Splinter Review
7.87 KB, patch
ochameau
: review+
past
: review+
Details | Diff | Splinter Review
16.30 KB, patch
ochameau
: review+
past
: review+
Details | Diff | Splinter Review
2.59 KB, patch
Details | Diff | Splinter Review
3.92 KB, patch
past
: review+
Details | Diff | Splinter Review
6.11 KB, patch
ochameau
: review+
past
: review+
Details | Diff | Splinter Review
2.92 KB, patch
ochameau
: review+
past
: review+
Details | Diff | Splinter Review
2.02 KB, patch
past
: review+
Details | Diff | Splinter Review
We currently have local and remote. We should add a Message Manager option to the transport for working across processes with e10s.
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
OS: Mac OS X → All
Hardware: x86 → All
(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.
Summary: Provide a Message Manager Transport for the Debug Protocol → Use the Message Manager Transport for the Debug Protocol on Desktop
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.
Summary: Use the Message Manager Transport for the Debug Protocol on Desktop → Use the ChildDebuggerTransport for the Debug Protocol on Desktop when e10s is enabled
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Priority: -- → P3
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.
Attachment #8385071 - Flags: review?(rcampbell)
Attached patch prefix-change (obsolete) — Splinter Review
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.
Attachment #8385074 - Flags: review?(rcampbell)
Attached patch connect-to-child (obsolete) — Splinter Review
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.
Attachment #8385075 - Flags: review?(rcampbell)
This patch renames ContentAppActor to ContentActor. I want to use it for e10s browser tabs, so the App name no longer applies.
Attachment #8385076 - Flags: review?(rcampbell)
Attached patch remote-browser-tab (obsolete) — Splinter Review
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.
Attachment #8385078 - Flags: review?(rcampbell)
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.
Attachment #8385080 - Flags: review?(rcampbell)
Attached patch remote-browser-tab (obsolete) — Splinter Review
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.
Attachment #8385083 - Flags: review?(rcampbell)
Assignee: rcampbell → nobody
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 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.
Attached patch prefix-change v2 (obsolete) — Splinter Review
Assignee: nobody → wmccloskey
Attachment #8385550 - Flags: review?(poirot.alex)
Attachment #8385071 - Flags: review?(rcampbell) → review?(poirot.alex)
Attachment #8385074 - Attachment is obsolete: true
Attachment #8385074 - Flags: review?(rcampbell)
This patch just renames connectToApp to connectToChild.
Attachment #8385075 - Attachment is obsolete: true
Attachment #8385075 - Flags: review?(rcampbell)
Attachment #8385552 - Flags: review?(poirot.alex)
Attachment #8385083 - Attachment is obsolete: true
Attachment #8385083 - Flags: review?(rcampbell)
Attachment #8385554 - Flags: review?(poirot.alex)
Attachment #8385076 - Flags: review?(rcampbell) → review?(poirot.alex)
Attachment #8385078 - Attachment is obsolete: true
Attachment #8385078 - Flags: review?(rcampbell)
Attachment #8385080 - Flags: review?(rcampbell) → review?(poirot.alex)
Bill, can you tag Panos Astithas for review as well? He needs to review server changes. Thanks!
Attached patch prefix-change v3Splinter Review
Attachment #8385550 - Attachment is obsolete: true
Attachment #8385550 - Flags: review?(poirot.alex)
Attachment #8385565 - Flags: review?(poirot.alex)
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 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.
Attachment #8385071 - Flags: review?(poirot.alex) → review+
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.
Attachment #8385076 - Flags: review?(poirot.alex) → review+
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);
  ...
}
Attachment #8385080 - Flags: review?(poirot.alex) → review+
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.
Attachment #8385552 - Flags: review?(poirot.alex)
Attachment #8385552 - Flags: review?(past)
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 ;)
Attachment #8385554 - Flags: review?(poirot.alex)
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)
Attachment #8385557 - Flags: review?(poirot.alex) → review+
Attachment #8385565 - Flags: review?(poirot.alex) → review+
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!
I don't think it would be hard to switch to using docShell for everything.
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 on attachment 8385071 [details] [diff] [review]
debug-progress-listener

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

LGTM.
Attachment #8385071 - Flags: review?(past) → review+
Comment on attachment 8385076 [details] [diff] [review]
rename-content-actor

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

LGTM.
Attachment #8385076 - Flags: review?(past) → review+
Attached patch fixes2 (obsolete) — Splinter Review
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.
Attachment #8386903 - Flags: review?(past)
Attached patch fixes2 v2Splinter Review
This needed a little more work to work with e10s.
Attachment #8386903 - Attachment is obsolete: true
Attachment #8386903 - Flags: review?(past)
Attachment #8386959 - Flags: review?(past)
Comment on attachment 8385565 [details] [diff] [review]
prefix-change v3

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

I think this is OK.
Attachment #8385565 - Flags: review?(past) → review+
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 on attachment 8385557 [details] [diff] [review]
rename-tab-actor

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

LGTM.
Attachment #8385557 - Flags: review?(past) → review+
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.
Attachment #8385080 - Flags: review?(past) → review+
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.
Attachment #8385554 - Flags: review?(past) → review+
Comment on attachment 8386959 [details] [diff] [review]
fixes2 v2

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

Makes sense.
Attachment #8386959 - Flags: review?(past) → review+
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
Actually, that seems to be the error that Alex was referring to in comment 22.
Depends on: 962577
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.
(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.
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.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.