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

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

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.

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.

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.

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.

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

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.

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.

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.

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.

This patch just renames connectToApp to connectToChild.

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

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.

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

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.

Comment on attachment 8385076 [details] [diff] [review] rename-content-actor Review of attachment 8385076 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

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.

This needed a little more work to work with e10s.

Comment on attachment 8385565 [details] [diff] [review] prefix-change v3 Review of attachment 8385565 [details] [diff] [review]: ----------------------------------------------------------------- I think this is OK.

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.

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 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 on attachment 8386959 [details] [diff] [review] fixes2 v2 Review of attachment 8386959 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.

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.

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.

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

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