Closed Bug 817580 Opened 12 years ago Closed 11 years ago

Expose tab actors for apps in child processes.

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:-)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g -

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files, 13 obsolete files)

Test addon that implement a client and UI for this webapps actor
185.25 KB, application/x-xpinstall
Details
master - Expose tab actors for apps in child processes.
24.29 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
Preffed off new webapps actor features until we secure remote debugger connection
5.17 KB, patch
ochameau
: review+
Details | Diff | Splinter Review 
For now, remove web console only expose shell.xul document context.
You can already try to get access to apps context by doing following steps:
Get a reference to system app window object:
  system = getContextWindow().wrappedJSObject;
And then get a particular app frame object:
  frame = system.WindowManager.getAppFrame("...app origin URL...");
And finally have access to its window object:
  frame.contentWindow

But it would be really convenient to give a direct access to each app global object. Here is a patch to do so.

The only issue is that you have to disable OOP as web console actor classes expect a `window` object. When an app is OOP (using iframe mozapp=origin mozbrowser=true remote=true>), you can't have access to its window object from the chrome process. And as web console code lives in chrome process...

Bug 797627 is tracking necessary work in order to be able to consolify OOP JS context.
Attachment #687750 - Attachment is obsolete: true
Comment on attachment 687766 [details] [diff] [review]
Bug 817580 : List running apps in remote console JS contexts

Here is the patch I was talking about this morning.
So, now, when connecting a remote console to a B2G instance, it lists all currently running apps with their name being displayed, plus the context of shell.xul.
Without this patch, it was only displaying an empty entry which was the shell.xul one. It will ignore all apps running out of process. So if you want to see all of them, you have to toggle OOP setting in phone developer settings panel, which is hidden in Settings>Device information>More information>Developer settings.

You can also see this patch here:
https://github.com/ochameau/releases-mozilla-central/compare/webconsole

Please forward this review to the right person if you don't feel confortable reviewing it.
Attachment #687766 - Flags: review?(past)
Blocks: 816961
Comment on attachment 687766 [details] [diff] [review]
Bug 817580 : List running apps in remote console JS contexts

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

This has been on my todo list for quite a while, so thank you for working on it!

I have an unusual large number of comments and suggestions per patch lines, but that's because I like it a lot :-)

r=me with these changes, but you may also want to ask for r? from Vivien as well to make it official.

::: b2g/chrome/content/dbg-browser-actors.js
@@ +62,5 @@
> +    actorPool.addActor(actor);
> +    tabActorList.push(actor);
> +  }).bind(this);
> +
> +  // Add shell.xul actor

You wouldn't have needed to add this actor separately if I had implemented a global 'chrome' actor for B2G, but that's not your problem.

@@ +64,5 @@
> +  }).bind(this);
> +
> +  // Add shell.xul actor
> +  let actor = this._tabActors.get(this.browser);
> +  if (!actor)

You need to make this same check for all the other actors you are creating, otherwise subsequent listTabs requests won't reuse the actors.

@@ +65,5 @@
> +
> +  // Add shell.xul actor
> +  let actor = this._tabActors.get(this.browser);
> +  if (!actor)
> +    registerApp(this.browser, "shell.xul");

Hard-coding shell.xul shouldn't be necessary, because bug 817537 will let us use the URL if there is no title present. Plus, there will be no need to remember to change the name when someone inevitably renames shell.xul to kernel.xul or something, in the future ;-)

@@ +68,5 @@
> +  if (!actor)
> +    registerApp(this.browser, "shell.xul");
> +
> +  // Add system app actor
> +  let systemApp = this.browser.document.getElementById('homescreen').contentWindow;

Use contentWindow.wrappedJSObject for consistency (and safety).

@@ +78,5 @@
> +  for (let origin in WindowManager.getRunningApps()) {
> +    let app = apps[origin];
> +    // We don't have access to global object for OOP apps
> +    if (app.frame.contentWindow)
> +      registerApp(XPCNativeWrapper(app.frame.contentWindow), app.name);

Same here, wrappedJSObject instead of explicit constructor call (not that it should matter though).

@@ +116,5 @@
>   *        The connection to the client.
>   * @param browser browser
>   *        The browser instance that contains this tab.
>   */
> +function DeviceTabActor(connection, browser, title) {

I'd rather you didn't pass the title as the third parameter, because DeviceTabActor inherits from BrowserTabActor, which expects a tabbrowser object for its third argument, and it becomes confusing quickly.

I propose you add a title getter method to DeviceTabActor, since that's what we are going to do for its parent as well:

https://github.com/joewalker/devtools-window/pull/320/files#L9R341

@@ +131,5 @@
>               'tab should have an actorID.');
>  
>    let response = {
>      'actor': this.actorID,
> +    'title': this.browser.title || this.title,

...and then you'd just use this.title here.
Attachment #687766 - Flags: review?(past) → review+
Attached patch Patch v2 (obsolete) — — Splinter Review 
I found the time to apply my comments to Alexandre's patch and also take into account last week's plan changes in regards to the debugger server in B2G. I removed access to the chrome global, so that now only the available apps are presented to the user.

In OOP-enabled builds we still need bug 797627, of course. Tested in m-i (b2g-desktop) and on my Nexus S (with and without the #ifndefs applied).
Attachment #687766 - Attachment is obsolete: true
Attachment #701464 - Flags: review?(fabrice)
Besides the immediate value of this change for b2g-desktop, I wanted to see if this change would alleviate the security concerns that were raised last week, so I have CCed various folks. Up until then, we would add the chrome window as a debuggee, but after this patch we add the contentWindow of the apps iframes, which should be safe.

Since we still don't have bug 797627, in the device (OOP-enabled) builds, it seems to only include the browser app, when it is running. This may not be a terribly good user experience, but it would at least move us back to where we were, and simplify the upgrade path for v1.1. Does this change plug the identified hole, or are there more to it that I haven't considered?
(In reply to Panos Astithas [:past] from comment #6)
> Since we still don't have bug 797627, in the device (OOP-enabled) builds, it
> seems to only include the browser app, when it is running. This may not be a
> terribly good user experience, but it would at least move us back to where
> we were, and simplify the upgrade path for v1.1. Does this change plug the
> identified hole, or are there more to it that I haven't considered?

The browser runs (partially?) in the parent process, which is why I guess it is exposed with your patch. But, the security requirement is that we don't expose the parent process to debugging at all.

For the browser, I think the debugger should be exposing the individual tabs of the browser as debug targets, not the browser app itself, just as I imagine we must do in Desktop Firefox.
(In reply to Brian Smith (:bsmith) from comment #7)
> (In reply to Panos Astithas [:past] from comment #6)
> > Since we still don't have bug 797627, in the device (OOP-enabled) builds, it
> > seems to only include the browser app, when it is running. This may not be a
> > terribly good user experience, but it would at least move us back to where
> > we were, and simplify the upgrade path for v1.1. Does this change plug the
> > identified hole, or are there more to it that I haven't considered?
> 
> The browser runs (partially?) in the parent process, which is why I guess it
> is exposed with your patch. But, the security requirement is that we don't
> expose the parent process to debugging at all.

The browser app runs in the parent process, and tabs are in child processes. To run the browser app OOP, we need to implement nested content processes.

> For the browser, I think the debugger should be exposing the individual tabs
> of the browser as debug targets, not the browser app itself, just as I
> imagine we must do in Desktop Firefox.

I agree for now. Once we have nested content processes, we should also allow debugging the browser app itself.
Comment on attachment 701464 [details] [diff] [review]
Patch v2

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

I'd rather have some events to query the list of running apps from gaia.

::: b2g/chrome/content/dbg-browser-actors.js
@@ +73,5 @@
> +  let systemApp = this.browser.document.getElementById('homescreen')
> +                      .contentWindow.wrappedJSObject;
> +  // Add actors for all running apps.
> +  let WindowManager = systemApp.WindowManager;
> +  let apps = WindowManager.getRunningApps();

This means we have a hard dependency on gaia's side, or whatever "system app" is running. We usually communicate with the system app using events so it's less tightly coupled (see sendChromeEvent() in shell.js at https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#500)
Attachment #701464 - Flags: review?(fabrice) → review-
(In reply to Brian Smith (:bsmith) from comment #7)
> (In reply to Panos Astithas [:past] from comment #6)
> > Since we still don't have bug 797627, in the device (OOP-enabled) builds, it
> > seems to only include the browser app, when it is running. This may not be a
> > terribly good user experience, but it would at least move us back to where
> > we were, and simplify the upgrade path for v1.1. Does this change plug the
> > identified hole, or are there more to it that I haven't considered?
> 
> The browser runs (partially?) in the parent process, which is why I guess it
> is exposed with your patch. But, the security requirement is that we don't
> expose the parent process to debugging at all.

So even exposing the content window (not the chrome window) of the main process is not acceptable? And I assume that is because sensitive apps (like the dialer?) will be running in the main process and there is some way to get to them from the content window?

If that is the case then I agree it doesn't make sense to remove the #ifndefs, but would it be acceptable to use a runtime check instead, like:

if (Services.appInfo.widgetToolkit != "gonk") ...

That would make the developing experience better for those of us that use --enable-chrome-format=symlink. 

> For the browser, I think the debugger should be exposing the individual tabs
> of the browser as debug targets, not the browser app itself, just as I
> imagine we must do in Desktop Firefox.

Right, and we can do this when the prerequisite work mentioned by Fabrice is in place.
Note that the unsafe thing here isn't exposing debugging of chrome code vs. content code. The thing we are worried about is being able to execute code in the parent process at all.

I don't understand how exposing *any* contexts for debugging can be safe at this point. Contexts that are running in the parent process aren't safe because they are running in the parent process.

Contexts that are running in a child process can't be exposed because the debugger doesn't have multiprocess support.

Am I missing something?
Understood, that's what I assumed in comment 10 as well. In that case, this patch (besides the change requested by Fabrice) will only improve debugging for b2g-desktop, by providing the running web apps as targets.

But my other question in that comment still stands: do we have to avoid building that code altogether, or can we just use a runtime check to disable it? I don't believe there is a way to change what Services.appInfo.widgetToolkit returns from JavaScript, is there?
(In reply to Panos Astithas [:past] from comment #10)
> So even exposing the content window (not the chrome window) of the main
> process is not acceptable? And I assume that is because sensitive apps (like
> the dialer?) will be running in the main process and there is some way to
> get to them from the content window?

Plus, the parent process runs as root so letting that process run arbitrary script is basically equivalent to exposing a root shell.

> if (Services.appInfo.widgetToolkit != "gonk") ...

Instead of checking for a particular platform, I would instead check for whether e10s is enabled.

(In reply to Panos Astithas [:past] from comment #12)
> But my other question in that comment still stands: do we have to avoid
> building that code altogether, or can we just use a runtime check to disable
> it? I don't believe there is a way to change what
> Services.appInfo.widgetToolkit returns from JavaScript, is there?

We have lots of runtime security checks based on prefs, so that would be fine.
(In reply to Jonas Sicking (:sicking) from comment #11)
> Contexts that are running in the parent process aren't safe because they are
> running in the parent process.

Actually, sorry, I think this is the wrong way to put it.

Contexts that are in the parent process aren't *interesting* since that's never application processes.

I think it is fine to allow the debugger to attach to the parent process in general, as long as we clearly verify with the owner of the phone that it's ok and that the owner has understood what it implies. It's just not a lot of value right now since it won't help 3rd party developers in any way. And it does introduce risk since it's a highly valuable attack target, and one that is connected to external ports.
(In reply to Jonas Sicking (:sicking) from comment #14)

> Contexts that are in the parent process aren't *interesting* since that's
> never application processes.

Except for the browser app until we have nested content processes.
Indeed, but that's also not a 3rd party app.
(In reply to Brian Smith (:bsmith) from comment #13)
> > if (Services.appInfo.widgetToolkit != "gonk") ...
> 
> Instead of checking for a particular platform, I would instead check for
> whether e10s is enabled.

But wouldn't that include (now or in the near future?) b2g-desktop, which we want to be fully debuggable for productivity reasons?

Also, how can one check for e10s? Is there a pref for that?

(In reply to Jonas Sicking (:sicking) from comment #14)
> I think it is fine to allow the debugger to attach to the parent process in
> general, as long as we clearly verify with the owner of the phone that it's
> ok and that the owner has understood what it implies. It's just not a lot of
> value right now since it won't help 3rd party developers in any way. And it
> does introduce risk since it's a highly valuable attack target, and one that
> is connected to external ports.

Agreed, there is value in debugging the parent process for Mozilla and partner developers, but not so much for 3rd party app developers. Let's postpone this work for bug 818385, which isn't in our immediate plans.
Attached patch Patch v3 (obsolete) — — Splinter Review 
I slightly tweaked an updated patch by Alex that avoids sync calls to Gaia code, but it doesn't work yet.
Attachment #701464 - Attachment is obsolete: true
I'm refining thig bug description given my better knowledges of devtools codebase:

We should expose tab actors like console, styleeditor and gcli for apps running in child (on device) or parent (on b2g desktop) processes.
So far, the existing patches was only trying to expose these actors for apps on b2g desktop. Also, we were hijacking the existing connect page and the BrowserRootActor in order to offer a way to connect to apps without having to modify the frontend/client side.
I think that we should expose apps via the webapps actor and do not mix tabs and apps. It will require some work on the connect page -or- a new Apps UI somewhere, but it will ease the integration with the simulator features where we only care about apps.

Patch is coming!
Summary: List running apps in remote console JS contexts → Expose tab actors for apps in child processes.
Comment on attachment 763864 [details] [diff] [review]
Expose tab actors for apps in child processes.

You can also see this patch on github, with its necessary dependencies:
https://github.com/ochameau/mozilla-central/compare/ed3a3484c8ffdc54385af3e972bb540752161228...b2g-actors

Mainly the devtools patches that allow to use message manager to pipe debugger protocol messages between the parent process and child processes, so that actors created in the child process can be queried from the client:
  * Bug 878958, attachment 760057 [details] [diff] [review]: Support connections with prefixed actor names, and prefix-based forwarding.
  * Bug 878958, attachment 760055 [details] [diff] [review]: Implement ChildDebuggerTransport, a debug protocol transport for communicating with content child processes via process message managers.

(I've also included bug 732553 in this branch as this intermittent failure happens quite often when playing with devtools on the device)

I ended up not using nsIContentProcessService for multiple reasons:
 * I haven't found any way to retrieve a reference to the app global window object from the XPCOM component started via `startService`. A bunch of XPCOM aren't working in the child process, Window related components are part of the non-functional ones.
 * Message manager(s) components seems to already provide necessary requirement to connect an actor in the child process. Also, at the end we are using them to pipe debugger protocol messages. So that it felt natural to give it a try. We can list processes by getting one specific message manager instance per app/process and there is some limited support for loading scripts in the child (loadFrameScript).
 * Message manager has the benefit of working seemlessly for both OOP modes. The API is going to expose a fully functional mesage manager instance for all apps no matter if they are OOP or in the parent process. Such behavior is going to allow us to have the exact same codepath for both b2g desktop (that runs apps in parent process) and b2g on phones (that runs all apps [but system, keyboard and browser] in child processes)

For the same reason, I haven't expose any child actor. There can be some usecases for it, but for apps usecase, the client shouldn't have to care if the app is OOP or in parent process. We should abstract that and return tab actors for the running apps.

Now, let me describe what I did:

 * Add a `listRunningApps` in the webapps actor.
 * This request uses the parent process message manager to send a `debug:connect` message to all running apps.
 * This message is listened by BrowserElementChild.js. This frame script is always loaded. It is a workaround. Ideally, we would have listen for this message in a new frame script dedicated to devtools. But the existing message manager components doesn't offer a way to load a new frame script on existing childs. (So far, it seems to only allow loading one in childs being opened after the call to loadFrameScript)
To minimize the impact of this workaround, I load another JS file, child.js, only when we receive this message.
 * child.js is initializing a new DebuggerServer instance. Remember that frame script are evaluated for both app in child processes, but also for app in parent process. So here it also run in the parent. In this case, the debugger server is already initialized. That's the reason why I removed the #ifdef GONK in shell.js.
 * child.js then connect child and parent debugger instances by calling DebuggerServer.connectToParent and pipe message via the message managers
 * child.js instanciate a tab actor for the app
 * and send it to the parent message manager
 * the listRunningApps request receive this message and also connect to the child via the message managers by using a ChildDebuggerTransport instance.
 * Finally, we return the list of app tab actors back to the client.

There is various tweaks we can do that I commented in the source code, but there is some point that are worth discussing immediatly:
 * some features related to the network in the web console won't work in the child process. http-on-* observer notification aren't dispatched in child process.
 * we should come to a conclusion for bug 802246. The WebConsoleActor constructor feels too magic. I had to modify this code again and I'm not sure I understand all the details behind the original issue. But can't we remove this conditional code and let the actor expose the contentWindow and isGlobalActor ?
 * my goal was to have something that works ASAP, but It could be worth factoring out listRunningApps code and use the iterator pattern you used for tabList.


Panos, I'm especially interested in the reason why we almost disabled remote debugging on device. We still let the user debug the master process, right?
Do you think we can re-enable it now? What is the rational behind the security issues? Here I enable debugging for all apps, so also apps in parent process like system, keyboard and browser. We can easily blacklist them and only enable it on desktop.

Mihai, Any thoughts about bug 802246 and WebConsoleActor constructor?

Jim, Obviously, I need plenty of your feedback :)
Attachment #763864 - Flags: feedback?(past)
Attachment #763864 - Flags: feedback?(mihai.sucan)
Attachment #763864 - Flags: feedback?(jimb)
I'm not sure if debugging the master process will ever be enabled on devices or not. In the discussions I had with b2g folks when the decision to disable remote debugging on devices was made, I remember someone mentioning that the ability for a process to modify the functionality of the dialer was a no-no from a certification perspective. The chrome debugger could certainly do that, and so could the global console actor.

The right people are CC'd here, so I'll let them elaborate on that. BTW, bug 832000 contains some more discussion on related risks.
(In reply to Panos Astithas [:past] from comment #22)
> I remember someone mentioning that the
> ability for a process to modify the functionality of the dialer was a no-no
> from a certification perspective. The chrome debugger could certainly do
> that, and so could the global console actor.

Alex tells me the dialer runs as a content app, so this restricts content debugging as well. Alex has volunteered to talk to the B2G people about this; we'll need to sort out the best we can do under the restrictions.
Wonderful --- this is great work, Alex! This isn't the way I had imagined we'd do it, but in some ways this fits the existing architecture better, so I think we should try to finish this up and land it.

In this patch, we don't use a root actor in the child process to list that child's tabs and apps. Instead, the parent process's root actor enumerates children, and then creates a tab actor for each child.


Some points that came up in our Vidyo discussion this morning:

- Because app management is much more involved than tab management (for example, Alex's simulator work is largely concerned with app-specific features that don't make sense for tabs), treating them separately in the protocol is probably okay. Alex is going to write documentation for the new 'listRunningApps' protocol request.


I understand that this is a proof-of-concept patch, with many parts only roughed in, but let me point out some issues I noticed anyway:

- I think the implementation of the 'listRunningApps' packet will hang if a tab is closed or exits while it is gathering the list. It counts the children of the nsIMessageBroadcaster, broadcasts a 'debug:connect' message, and then waits for the expected number of responses. If a child exits as the parent obtains the child count, it will never have a chance to reply to the 'debug:connect' packet, and the promise listRunningApps returned will never resolve.

- Each time a 'listRunningApps' packet is received, it broadcasts a fresh 'debug:connect' message to all children, and creates a fresh DebuggerServerConnections, ContentTabActor, and so on. Once an actor has been created for a given parent connection, subsequent 'listRunningApps' requests should return the same actors, until the apps are closed.

- The code doesn't seem to distinguish between apps and tabs; it returns actors for every TabChild, regardless of whether its docShell.appId is set.

- The patch uses the process message manager tree instead of the DOM message manager tree. If I remember correctly, we agreed that the DOM message manager tree would be better, because they implement nsIFrameScriptLoader, which would be a fine way to get the debugging server started in content apps/tabs. I don't remember why the patch doesn't do this. I'm probably missing something, but when I experimented it seemed to work fine:

The global message manager --- the root of the DOM message manager tree --- is available as '@mozilla.org/globalmessagemanager;1'; that implements nsIFrameScriptLoader. Within each child, since TabChildGlobal inherits from nsIContentFrameMessageManager, 'sendAsyncMessage' is available as a global function. In desktop Firefox, at least, I'm able to load a script via loadFrameScript in the tabs, and have them send back their URLs.

I edited toolkitv/devtools/server/transport.js to contain the text:

dump("Yo, from transport.js: " + uneval(content.document.URL) + "\n");
sendAsyncMessage("jimb:fanatic", { reason: "For The Love Of God" });

And then evaluated the following code in the parent:

var gmm = Components.classes['@mozilla.org/globalmessagemanager;1']
          .getService(Components.interfaces.nsIMessageBroadcaster);
gmm.QueryInterface(Components.interfaces.nsIFrameScriptLoader);
gmm.addMessageListener('jimb:fanatic', (m) => { dump("message: " + uneval(m.json) + "\n"); });
gmm.loadFrameScript("resource://gre/modules/devtools/server/transport.js", false);

I got the following output at the terminal:

Yo, from transport.js: "about:blank"
Yo, from transport.js: "about:home"
message: ({reason:"For The Love Of God"})
message: ({reason:"For The Love Of God"})

I may well be forgetting something we talked about, but if this works on B2G as well, then the patch should use this technique, right?

- ContentTabActor should perhaps be split across parent and child, and only bother the child when the client actually attaches to it.


Some tasks for me:

- This patch uses attachments 760057 and 760055 from bug 878958; I should get those landed.

- The patch creates a DebuggerServerConnection, but has no use for the RootActor that that creates; the top-level actor on that connection will be the ContentTabActor. There should be a way to create DebuggerServerConnections that doesn't insist on creating a RootActor.
(In reply to Jim Blandy :jimb from comment #24)
> - I think the implementation of the 'listRunningApps' packet will hang if a
> tab is closed or exits while it is gathering the list.

I'll listen for `child-process-shutdown` event, that is send on message manager disconnect. That appears to be also called when the process crashed:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#866

> - The patch uses the process message manager tree instead of the DOM message
> manager tree. If I remember correctly, we agreed that the DOM message
> manager tree would be better, because they implement nsIFrameScriptLoader,
> which would be a fine way to get the debugging server started in content
> apps/tabs. I don't remember why the patch doesn't do this. I'm probably
> missing something, but when I experimented it seemed to work fine:
> 
> The global message manager --- the root of the DOM message manager tree ---
> is available as '@mozilla.org/globalmessagemanager;1'; that implements
> nsIFrameScriptLoader. Within each child, since TabChildGlobal inherits from
> nsIContentFrameMessageManager, 'sendAsyncMessage' is available as a global
> function. In desktop Firefox, at least, I'm able to load a script via
> loadFrameScript in the tabs, and have them send back their URLs.

As said on irc, globalmessagemanager doesn't work on mozbrowser/mozapp frames.
loadFrameScript only create a script for the system app.
I'll try to list all iframes in the system app and retrieve the DOM message manager from them.

> 
> - ContentTabActor should perhaps be split across parent and child, and only
> bother the child when the client actually attaches to it.
> 

Yes, I'd really like to avoid doing anything in child until explicitely needed.
I tried to do that, but it appears to be complicated because of tab actors like webconsole, script, ... that are returning in actor.grip(), during listTabs/listRunningApps! That happen to be done before the call to actor's attach request :( And as for the AppActor, these tab actors should also be instanciated in child. As for AppActor, we could work around that by using some actor proxy and using the fact that we can give actor constructor functions.
But it looks like we will end up with a bunch of complex asynchronous code between the parent and child.

I'm suggesting two options here (more welcomed!):
* Modify clients to expect tab actors only in BrowserTabActor's attach request result. So, longer expect them to be defined directly in listTabs result,

* Add a new method in the WebappsActor, like getAppActor(manifestURL),
  that will return the actor's grip with tab actors being set,
  and the app actor itself will be connected to the child.
  We can also wait for the attach request before connecting to the child,
  but it looks like getAppActor and actor.attach calls will be most likely
  always be called one after another, so we can just simplify it to avoid having 
  to create a proxy in the parent.

We would end up with something similar to this:

{ to: "webapps", type: "listRunningApps" }
 =>
{ from: "webapps", apps: ["http://mozilla.org/webapp.manifest", "https://wikipedia.org/firefox.manifest"] }

{ to: "webapps", type: "getAppActor", manifestURL: "http://mozilla.org/webapp.manifest" }
 =>
{ from: "webapps", actor: { actor: "conn1.tab1", webconsole: "conn1.console1", script: "conn1.script1" }

I have a very similar patch to what I've uploaded here in bugzilla, but for the simulator and targeting only in-parent-process apps, and I end up willing such "getAppActor" request.
I've had to also implement app-open and app-close events in addition to listRunningApps and I want to lazily create the actor in both cases. So that it seems natural to have an explicit function to do so.
Depends on: 887809
Depends on: 887781
I've a new set of patches, I think I've now addressed all remarks from comment 24.
I need to land dependencies (see bug 887809, 887781) and then squash and eventually split my patch in meaningfull pieces. 
You can get a preview (and working branch) over here:
https://github.com/ochameau/mozilla-central/compare/c9c1c00eaeea47ab7cf3bf1f742072091acc3e68...b2g-actors


(In reply to Alexandre Poirot (:ochameau) from comment #25)
> (In reply to Jim Blandy :jimb from comment #24)
> > - I think the implementation of the 'listRunningApps' packet will hang if a
> > tab is closed or exits while it is gathering the list.
> 
> I'll listen for `child-process-shutdown` event, that is send on message
> manager disconnect. That appears to be also called when the process crashed:
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#866

child-process-shutdown ends up only be fired for apps being OOP,
I discussed with smaug and introduced a new nsIObserverService notification "message-manager-disconnect" in bug 887781. This event allows us to detect when the app is closed or crashed in order to stop using the message manager and stop the DebuggerChildTransport and unplug the prefix forwarding.


> > - ContentTabActor should perhaps be split across parent and child, and only
> > bother the child when the client actually attaches to it.
> 
> * Add a new method in the WebappsActor, like getAppActor(manifestURL),
>   that will return the actor's grip with tab actors being set,
>   and the app actor itself will be connected to the child.

I ended up doing that, and it integrated really well in the simulator.
Attachment #763864 - Attachment is obsolete: true
Attachment #763864 - Flags: feedback?(past)
Attachment #763864 - Flags: feedback?(mihai.sucan)
Attachment #763864 - Flags: feedback?(jimb)
Comment on attachment 768577 [details] [diff] [review]
Expose tab actors for apps in child processes.

Ok, tested on device and with b2g-master connected via the simulator and it appears to be working quite well!
We can even reuse actors without creating a new one each time we reopen a new toolbox against the same app, nor opening a brand new connection.

I think that's ready for review!
Attachment #768577 - Flags: review?(jimb)
Comment on attachment 768577 [details] [diff] [review]
Expose tab actors for apps in child processes.

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

Mihai, I'm also requesting your feedback as I had to tweak two additional things in webconsole to make it work in child processes.

::: toolkit/devtools/server/actors/webconsole.js
@@ -66,3 @@
>      // behaves as if the user picked the global console actor.
> -    //this._window = aParentActor.browser;
> -    this._window = Services.wm.getMostRecentWindow("navigator:browser");

I found that really weird to let the console code to choose the global object.
I tend to think that the parent actor should control the global, so that the WebConsoleActor would always fetch it from aParentActor.
But I just want to make it work for child processes, so I only modified the first b2g-specific condition.

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +1770,5 @@
>  
> +    if (Services.appinfo.processType != Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) {
> +      Services.obs.addObserver(this._httpResponseExaminer,
> +                               "http-on-examine-response", false);
> +    }

I had to disable these observers as registering various http-on notifications are rejected in child processes (more info in bug 806753).
I heard that all network requests were done in parent process, so if that's (still) true, we will most likely have to do some parent<->child communication to make work some network-related features.
Attachment #768577 - Flags: feedback?(mihai.sucan)
I rebased all these patches against v1-train in order to request an uplift.
You can easily fetch a working branch if you are using git by cloning this branch:
https://github.com/ochameau/mozilla-central/tree/actors-v1-train

And see the patches here:
https://github.com/ochameau/mozilla-central/compare/fc47f58d8eb1d600b3553cda7cf7cbcfea636a3d...actors-v1-train

Also, here is an addon, that hack the connect page to display phone apps and easily connect a toolbox to running ones. Note that style editor and network panel are not working as they are not uplifted to v1-train.
Also, if you want to apply this patch queue without having to use git, you can fetch a patch file from this URL:
https://github.com/ochameau/mozilla-central/compare/fc47f58d8eb1d600b3553cda7cf7cbcfea636a3d...actors-v1-train.patch
(In reply to Alexandre Poirot (:ochameau) from comment #29)
> Comment on attachment 768577 [details] [diff] [review]
> Expose tab actors for apps in child processes.
> 
> Review of attachment 768577 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mihai, I'm also requesting your feedback as I had to tweak two additional
> things in webconsole to make it work in child processes.
> 
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ -66,3 @@
> >      // behaves as if the user picked the global console actor.
> > -    //this._window = aParentActor.browser;
> > -    this._window = Services.wm.getMostRecentWindow("navigator:browser");
> 
> I found that really weird to let the console code to choose the global
> object.
> I tend to think that the parent actor should control the global, so that the
> WebConsoleActor would always fetch it from aParentActor.
> But I just want to make it work for child processes, so I only modified the
> first b2g-specific condition.

The console code chooses the global object only when used as a global console actor - eg. the parent is not the browser tab actor. Can you please file a bug about improving the console actor constructor? We need a better strategy of picking the global object and basing it on the parent actor makes sense.


> ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
> @@ +1770,5 @@
> >  
> > +    if (Services.appinfo.processType != Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) {
> > +      Services.obs.addObserver(this._httpResponseExaminer,
> > +                               "http-on-examine-response", false);
> > +    }
> 
> I had to disable these observers as registering various http-on
> notifications are rejected in child processes (more info in bug 806753).
> I heard that all network requests were done in parent process, so if that's
> (still) true, we will most likely have to do some parent<->child
> communication to make work some network-related features.

Will network logging continue to work when the client attaches to the parent process? Can we have the client receive network info from the parent process and the rest of logs from the child process?
(In reply to Mihai Sucan [:msucan] from comment #32)
> The console code chooses the global object only when used as a global
> console actor - eg. the parent is not the browser tab actor. Can you please
> file a bug about improving the console actor constructor? We need a better
> strategy of picking the global object and basing it on the parent actor
> makes sense.

bug 889361


> > ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
> > @@ +1770,5 @@
> > >  
> > > +    if (Services.appinfo.processType != Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) {
> > > +      Services.obs.addObserver(this._httpResponseExaminer,
> > > +                               "http-on-examine-response", false);
> > > +    }
> > 
> > I had to disable these observers as registering various http-on
> > notifications are rejected in child processes (more info in bug 806753).
> > I heard that all network requests were done in parent process, so if that's
> > (still) true, we will most likely have to do some parent<->child
> > communication to make work some network-related features.
> 
> Will network logging continue to work when the client attaches to the parent
> process?

I assume it works. I can see network related messages in the web console when I connect to the browser app that runs in parent process.

> Can we have the client receive network info from the parent process
> and the rest of logs from the child process?

The webconsole actor has to live in child in order to get access to the document global object, but yes, we can also spawn some code in parent. We will have to find a smart way to do that without introducing too much complexity...
Depends on: 889838
I pushed new changeset to the v1-train patch queue:
https://github.com/ochameau/mozilla-central/compare/fc47f58d8eb1d600b3553cda7cf7cbcfea636a3d...actors-v1-train.patch

It fixes various issues:
- non-functional object stringification/inspection in webconsole,
- non-functional object variable inspection in debugger.

I fixed that by:
- uplifting Bug 783499 that needs Bug 796073 to work for the first issue,
- tuning firefox code for the second, bug 889838

This patch queue has been tested by some gaia devs who helped me find these issues and some others that are still happening and may stay broken on this branch:
- we can't modify property values in webconsole/debugger object inspectors (may be bug 837723)
- webconsole/debugger object inspectors don't list much properties for DOM objects (bug 830818 and eventual dependencies)
- new script being added dynamically aren't tracked by the debugger
- style editor doesn't work (it may be easy as we are using it in the simulator and it uses b2g18)

Here is the current dependency:
Bug 878958: Support connections with prefixed actors
Bug 878958: Implement ChildDebuggerTransport
Bug 887809: Webconsole actor doesn't unregister correctly listeners
All these 3 are javascript patches targeting only devtools codebase.
To be landed.

Bug 887781: Send message-manager-disconnect notification vent
Quite trivial C++ patch to be able to detect when an app is closed.
But is executed outside of devtools scope, anytime an app is closed.
Landed in master last week.

Bug 844227: Add more functions to the webapps actor
New code in devtools, unit tested, but comes with some simple refactoring of Webapps.jsm.
Landed in master in May.

Bug 732553: NS_BASE_STREAM_CLOSED in dbg-transport.js
Trivial js bugfix in devtools codebase.
Landed in master two weeks ago.

Bug 796073: [jsdbg2] addDebuggee should not accept non-globals 
Dependency for next bug. C++ patch targetting Debugger object implementation.
I think Debugger is only used by devtools?
Landed in master in october.

Bug 783499: Web Console should use the debugger API
Substantial patch against JS devtools codebase and especially webconsole actor's code.
Landed in master in April.

We can avoid uplifting bug 783499 and 796073 if trade the object inspector feature in the webconsole.
Attachment #769875 - Attachment is obsolete: true
I forgot to mention the necessary security fix to be provided in bug 832000 that depends on bug 834002.

All these patches are going to allow getting tab actors for all apps,
so that we will be able to connect remote toolboxes for app on device, 
but also on b2g-desktop for the simulator.
It also add more features to the webapps actor to be able to: launch/close/uninstall apps. And also watch for apps being launched/close.
Given the addition of the sole existing devtool feature in v1-train: app install.
It will give us the necessary devtools features on the device to start building an app manager tool in firefox in order to ease creating/developing/debugging apps directly on the device.
Attachment #771333 - Attachment description: Expose tab actors for apps in child processes. → b2g-18 - Expose tab actors for apps in child processes.
Attachment #771333 - Flags: review?(jimb)
Attachment #768577 - Attachment is obsolete: true
Attachment #768577 - Flags: review?(jimb)
Attachment #768577 - Flags: feedback?(mihai.sucan)
Attachment #771407 - Attachment is obsolete: true
Comment on attachment 771411 [details] [diff] [review]
master - Expose tab actors for apps in child processes.

Jim, here is two patches, one for each branch.
I've attached b2g18 patches to all required dependencies.

Also, I went ahead and ask review for required patches from bug 878958 that we depend on.
So that the only patch left to have stuff working is this attachment (and the master equivalent patch)
Attachment #771411 - Attachment description: Expose tab actors for apps in child processes. → b2g-18 - Expose tab actors for apps in child processes.
Attachment #771411 - Flags: review?(jimb)
Attachment #724695 - Attachment is obsolete: true
Attachment #771411 - Attachment description: b2g-18 - Expose tab actors for apps in child processes. → master - Expose tab actors for apps in child processes.
Comment on attachment 771411 [details] [diff] [review]
master - Expose tab actors for apps in child processes.

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

Some preliminary comments:

::: toolkit/devtools/server/actors/webapps.js
@@ +30,5 @@
>    Cu.import("resource://gre/modules/FileUtils.jsm");
>    Cu.import('resource://gre/modules/Services.jsm');
>    Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> +
> +  this._appActorsMap = new WeakMap();

It would be nice to have a brief comment here explaining what this map's keys and values are. (It's clear from reading the code, but I think it is still helpful to people who aren't reading the whole thing.)

WeakMaps are kind of expensive for the GC to manage; can we use an ordinary map, and have the onMessageManagerDisconnect function remove entries from it?

@@ +391,5 @@
> +
> +    // Register apps hosted in the system app. i.e. the homescreen, all regular
> +    // apps and the keyboard.
> +    // XXX: We can easily fetch bookmark apps and other system app internal frames
> +    // like captive portal here. They are not using mozapp attribute.

Perhaps this should be a follow-up bug --- but I don't think 'XXX' comments in the code are really a good way to track future work.

@@ +398,5 @@
> +      yield frames[i];
> +    }
> +  },
> +
> +  listRunningApps: function wa_actorListRunningApps(aRequest) {

I don't think we need names on internal functions like this any more: they should appear in stack traces, etc. as WebappsActor.prototype.listRunningApps --- the compiler figures out those names.

@@ +401,5 @@
> +
> +  listRunningApps: function wa_actorListRunningApps(aRequest) {
> +    debug("listRunningApps\n");
> +
> +    let defer = Promise.defer();

'defer' seems to be unused.

@@ +404,5 @@
> +
> +    let defer = Promise.defer();
> +    let apps = [];
> +
> +    for each (let frame in this._appFrames()) {

Is it possible to use for-of here, and elsewhere in the patch? 'for each' didn't make ES6, so it'll be going away eventually.

@@ +412,5 @@
> +
> +    return { apps: apps };
> +  },
> +
> +  _connectToApp: function (aFrame, aCallback) {

aCallback is unused.

@@ +420,5 @@
> +    mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
> +
> +    let childTransport, prefix;
> +
> +    let onActorCreated = (function (msg) {

Is it possible to use an arrow function here, instead of (function ...).bind(this)? Or does that just make back-porting to 18 harder?

@@ +446,5 @@
> +      defer.resolve(actor);
> +    }).bind(this);
> +    mm.addMessageListener("debug:actor", onActorCreated);
> +
> +    let onMessageManagerDisconnect = (function (subject, type, data) {

"type" is usually called "topic".

@@ +457,5 @@
> +          this.conn.cancelForwarding(prefix);
> +        } else {
> +          // Otherwise, the app has been closed before the actor
> +          // had a chance to be created, so we are not able to create
> +          // the actor.

Great comments. I was wondering about exactly this.

@@ +465,5 @@
> +    }).bind(this);
> +    Services.obs.addObserver(onMessageManagerDisconnect,
> +                             "message-manager-disconnect", false);
> +
> +    let prefix_start = this.conn.prefix + "child";

The coding style here would call this variable 'prefixStart'. (I prefer underscores myself, but this variation is just noise.)

@@ +474,5 @@
> +
> +  getAppActor: function (aRequest) {
> +    debug("getAppActor\n");
> +
> +    let { manifestURL } = aRequest;

Why not just do this destructuring directly in the function head?

  getAppActor: function({ manifestURL }) ...

@@ +479,5 @@
> +
> +    let appFrame = null;
> +    for each (let frame in this._appFrames()) {
> +      if (frame.getAttribute("mozapp") == manifestURL) {
> +        appFrame = frame;

We can 'break;' here, right?

Is it guaranteed that there won't be two apps running with the same manifestURL?

@@ +490,5 @@
> +                        "is '" + manifestURL + "'" };
> +    }
> +
> +    // XXX: I'd have liked to use messageManager as key, but for some reasons
> +    // they can't be weakmap keys :(

This is a helpful explanation, but the "XXX: " should go. (If you're planning to fix it, then there should be a follow-up bug for that work - but no "XXX" comment.)

::: toolkit/devtools/server/child.js
@@ +20,5 @@
> +
> +  // We have to wait for webbrowser.js to be loaded before fetching BrowserTabActor
> +  let { BrowserTabActor } = DebuggerServer;
> +
> +  //XXX: Move ContentTabActor to its own file?

This should be another follow-up bug.

@@ +39,5 @@
> +  }
> +
> +  ContentTabActor.prototype.constructor = ContentTabActor;
> +
> +  ContentTabActor.prototype = Object.create(BrowserTabActor.prototype);

Shouldn't this come before the assignment to ContentTabActor.prototype.constructor, above?

@@ +68,5 @@
> +
> +  // Override grip just to rename this._tabActorPool to this._tabActorPool2
> +  // in order to prevent it to be cleaned on detach.
> +  // We have to keep tab actors alive as we keep the ContentTabActor
> +  // alive after detach and reuse it for multiple debug sessions.

Doesn't this mean that simultaneous connections will behave in confusing ways, with each client affecting the other clients' state?
Comment on attachment 771411 [details] [diff] [review]
master - Expose tab actors for apps in child processes.

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

If I'm understanding correctly, the problems in child.js are serious, so I'm clearing review on this. (Feel free to re-request if I've misunderstood.)

::: b2g/chrome/content/shell.js
@@ +1040,1 @@
>        }

Let's move this try/catch to b2g/chrome/content/settings.js, where we call RemoteDebugger.server, since that's the context that makes normal exception propagation problematic.

::: toolkit/devtools/server/actors/webapps.js
@@ +420,5 @@
> +    mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
> +
> +    let childTransport, prefix;
> +
> +    let onActorCreated = (function (msg) {

Also: you should use makeInfallible here, to make sure errors aren't lost.

@@ +426,5 @@
> +
> +      dump("***** Got debug:actor\n");
> +      let { actor, appId } = msg.json;
> +      prefix = msg.json.prefix;
> +      let messageManager = msg.target;

This is unused. (Won't msg.target always be the same as mm? If so, it's best to just use 'mm' everywhere, to avoid confusing readers.)

@@ +446,5 @@
> +      defer.resolve(actor);
> +    }).bind(this);
> +    mm.addMessageListener("debug:actor", onActorCreated);
> +
> +    let onMessageManagerDisconnect = (function (subject, type, data) {

Use 'makeInfallible' here, too.

@@ +532,5 @@
> +        if (this._appActorsMap.has(frame)) {
> +          return;
> +        }
> +
> +        // XXX: workaround to be able to get the frame during appterminated evt

This should be expanded into a full comment about why _framesByOrigin is needed, and the "XXX" removed.

@@ +534,5 @@
> +        }
> +
> +        // XXX: workaround to be able to get the frame during appterminated evt
> +        origin = event.detail.origin;
> +        this._framesByOrigin[origin] = frame;

Will this leak frames? Perhaps the "appterminated" event should remove entries.

::: toolkit/devtools/server/child.js
@@ +1,4 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import('resource://gre/modules/devtools/dbg-server.jsm');
> +
> +addMessageListener('debug:connect', function (msg) {

You should use makeInfallible here.

@@ +32,5 @@
> +   *        The conection to the client.
> +   * @param browser browser
> +   *        The browser instance that contains this tab.
> +   */
> +  function ContentTabActor(connection, browser)

Couldn't the definition of ContentTabActor be moved to the top level of the file, and out of the message listener?

These days the functions won't get compiled until they're called anyway.

@@ +68,5 @@
> +
> +  // Override grip just to rename this._tabActorPool to this._tabActorPool2
> +  // in order to prevent it to be cleaned on detach.
> +  // We have to keep tab actors alive as we keep the ContentTabActor
> +  // alive after detach and reuse it for multiple debug sessions.

I'm confused --- doesn't this patch still create a fresh ContentTabActor each time it receives a 'debug:connect' message? So all this actor pool shuffling means is that we add pools to the connection's table, but never remove them.
Attachment #771411 - Flags: review?(jimb)
(In reply to Jim Blandy :jimb from comment #40)
> Comment on attachment 771411 [details] [diff] [review]
> @@ +404,5 @@
> > +
> > +    let defer = Promise.defer();
> > +    let apps = [];
> > +
> > +    for each (let frame in this._appFrames()) {
> 
> Is it possible to use for-of here, and elsewhere in the patch? 'for each'
> didn't make ES6, so it'll be going away eventually.

As arrow function, for-of doesn't work out of the box on gecko18.
If you don't mind, I'd like to postpone usage of non-18 JS feature until we know if we are going to uplift of not in order to ease maintaining both patches.

> @@ +479,5 @@
> > +
> > +    let appFrame = null;
> > +    for each (let frame in this._appFrames()) {
> > +      if (frame.getAttribute("mozapp") == manifestURL) {
> > +        appFrame = frame;
> 
> We can 'break;' here, right?
> 
> Is it guaranteed that there won't be two apps running with the same
> manifestURL?

Yes, I forgot to break!

> ::: toolkit/devtools/server/child.js
> @@ +20,5 @@
> > +
> > +  // We have to wait for webbrowser.js to be loaded before fetching BrowserTabActor
> > +  let { BrowserTabActor } = DebuggerServer;
> > +
> > +  //XXX: Move ContentTabActor to its own file?
> 
> This should be another follow-up bug.
> @@ +32,5 @@
> > +   *        The conection to the client.
> > +   * @param browser browser
> > +   *        The browser instance that contains this tab.
> > +   */
> > +  function ContentTabActor(connection, browser)
> 
> Couldn't the definition of ContentTabActor be moved to the top level of the
> file, and out of the message listener?
> 
> These days the functions won't get compiled until they're called anyway.

We can't evaluate ContentTabActor at child.js's toplevel as we need to wait for addActors call for webbrowser.js to be done before fetching BrowserTabActor.
I ended up moving ContentTabActor to actors/contenttab.js in this patch.
Do you think that's a good idea?
I'm wondering if we should move more (almost everything) code from child.js to main.js, may be connectToParent (or somewhere else)?
So that child.js would end up like this:
  addMessageListener('debug:connect', function (msg) {
    let prefix = msg.data.prefix + docShell.appId;
    let actor = DebuggerServer.connectToParent(prefix, mm);
    sendAsyncMessage("debug:actor", {actor: actor.grip(),
                                     appId: docShell.appId,
                                     prefix: prefix});
  });

> @@ +68,5 @@
> > +
> > +  // Override grip just to rename this._tabActorPool to this._tabActorPool2
> > +  // in order to prevent it to be cleaned on detach.
> > +  // We have to keep tab actors alive as we keep the ContentTabActor
> > +  // alive after detach and reuse it for multiple debug sessions.
> 
> Doesn't this mean that simultaneous connections will behave in confusing
> ways, with each client affecting the other clients' state?
> @@ +68,5 @@
> > +
> > +  // Override grip just to rename this._tabActorPool to this._tabActorPool2
> > +  // in order to prevent it to be cleaned on detach.
> > +  // We have to keep tab actors alive as we keep the ContentTabActor
> > +  // alive after detach and reuse it for multiple debug sessions.
> 
> I'm confused --- doesn't this patch still create a fresh ContentTabActor
> each time it receives a 'debug:connect' message? So all this actor pool
> shuffling means is that we add pools to the connection's table, but never
> remove them.

Correct me if I'm wrong, but `WebappsActor` is going to be instanciated once per connection?
If that's the case, each client is going to have it's own WebappsActor's _appActorMap Map. This map allows to reuse already instanciated actor, but only for the given WebappsActor instance. So that we will create new tab actors for each new client that comes up.

Otherwise, from child.js perspective, yes, we are creating a new ContentTabActor instance anytime we receive a new debug:connect. (But webapps.js ensures calling it only when required: once per app per connection)

Also, actors are big beast and there is a lot of specific details for each tool (console vs debugger), so I can easily be missing some cleanup and leave some low level actors when reusing the same ContentTabActor instance... But I verified, it works well with webconsole and debugger, we can reuse the same ContentTabActor multiple times with these fixes.


> ::: toolkit/devtools/server/actors/webapps.js
> @@ +420,5 @@
> > +    mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
> > +
> > +    let childTransport, prefix;
> > +
> > +    let onActorCreated = (function (msg) {
> 
> Also: you should use makeInfallible here, to make sure errors aren't lost.

I'm concerned with this practice, that is very viral and doesn't look free performance-wise. Shouldn't we patch APIs that are silently ignoring exceptions?

In anycase, I modified the patch to use it.

> @@ +534,5 @@
> > +        }
> > +
> > +        // XXX: workaround to be able to get the frame during appterminated evt
> > +        origin = event.detail.origin;
> > +        this._framesByOrigin[origin] = frame;
> 
> Will this leak frames? Perhaps the "appterminated" event should remove
> entries.

Oops!!!
Yes, appterminated will be called whenever the app is closed/crashed/killed.


Otherwise, I addressed all other comments not being mentioned here.
(In reply to Alexandre Poirot (:ochameau) from comment #43)
> As arrow function, for-of doesn't work out of the box on gecko18.
> If you don't mind, I'd like to postpone usage of non-18 JS feature until we
> know if we are going to uplift of not in order to ease maintaining both
> patches.

Okay - that makes sense.


> > ::: toolkit/devtools/server/child.js
> > @@ +20,5 @@
> > > +
> > > +  // We have to wait for webbrowser.js to be loaded before fetching BrowserTabActor
> > > +  let { BrowserTabActor } = DebuggerServer;
> > > +
> > > +  //XXX: Move ContentTabActor to its own file?
> > 
> > This should be another follow-up bug.
> > @@ +32,5 @@
> > > +   *        The conection to the client.
> > > +   * @param browser browser
> > > +   *        The browser instance that contains this tab.
> > > +   */
> > > +  function ContentTabActor(connection, browser)
> > 
> > Couldn't the definition of ContentTabActor be moved to the top level of the
> > file, and out of the message listener?
> > 
> > These days the functions won't get compiled until they're called anyway.
> 
> We can't evaluate ContentTabActor at child.js's toplevel as we need to wait
> for addActors call for webbrowser.js to be done before fetching
> BrowserTabActor.

Oh, I see. You even tried to explain that in the comments; sorry.


> I ended up moving ContentTabActor to actors/contenttab.js in this patch.
> Do you think that's a good idea?

Sure, that seems like a good approach. Perhaps childtab.js would be a better name; I would think desktop Firefox tabs were 'content tabs'.

It seems to me that we always call loadFrameScript('child.js') just before we're about to send it a 'debug:connect' message, so it doesn't seem to matter if we put off calling DebuggerServer.init and .addBrowserActors until we actually receive the 'debug:connect' message. It's guaranteed to happen anyway. We might as well just call DebuggerServer.init and DebuggerServer.addBrowserActors at the top level in child.js and keep things simple.

I think it would be fine to add the "addActors('childtab.js')" to addBrowserActors, so we could just call addBrowserActors; or create a new DebuggerServer.addChildActors that loads exactly what you want if addBrowserActors isn't appropriate.


> I'm wondering if we should move more (almost everything) code from child.js
> to main.js, may be connectToParent (or somewhere else)?
> So that child.js would end up like this:
>   addMessageListener('debug:connect', function (msg) {
>     let prefix = msg.data.prefix + docShell.appId;
>     let actor = DebuggerServer.connectToParent(prefix, mm);
>     sendAsyncMessage("debug:actor", {actor: actor.grip(),
>                                      appId: docShell.appId,
>                                      prefix: prefix});
>   });

I don't think DebuggerServer.connectToParent should create the actor. I don't think the 'connect' functions should even create the root actor, so having them create ContentTabActors as well seems like going in the wrong direction.

So child.js would end up like this:

Cu.import("resource://gre/modules/Services.jsm");
Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");

if (!DebuggerServer.initialized) {
  DebuggerServer.init();
  DebuggerServer.addBrowserActors(); // loads childtab.js
}

addMessageListener('debug:connect', DevToolsUtils.makeInfallible(function (msg) {
  let mm = msg.target;

  let prefix = msg.data.prefix + docShell.appId;

  let conn = DebuggerServer.connectToParent(prefix, mm);

  let actor = new DebuggerServer.ContentTabActor(conn, content);
  let actorPool = new ActorPool(conn);
  actorPool.addActor(actor);
  conn.addActorPool(actorPool);

  sendAsyncMessage("debug:actor", {actor: actor.grip(),
                                   appId: docShell.appId,
                                   prefix: prefix});
}));


> > @@ +68,5 @@
> > > +
> > > +  // Override grip just to rename this._tabActorPool to this._tabActorPool2
> > > +  // in order to prevent it to be cleaned on detach.
> > > +  // We have to keep tab actors alive as we keep the ContentTabActor
> > > +  // alive after detach and reuse it for multiple debug sessions.
> > 
> > Doesn't this mean that simultaneous connections will behave in confusing
> > ways, with each client affecting the other clients' state?
> > @@ +68,5 @@
> > > +
> > > +  // Override grip just to rename this._tabActorPool to this._tabActorPool2
> > > +  // in order to prevent it to be cleaned on detach.
> > > +  // We have to keep tab actors alive as we keep the ContentTabActor
> > > +  // alive after detach and reuse it for multiple debug sessions.
> > 
> > I'm confused --- doesn't this patch still create a fresh ContentTabActor
> > each time it receives a 'debug:connect' message? So all this actor pool
> > shuffling means is that we add pools to the connection's table, but never
> > remove them.
> 
> Correct me if I'm wrong, but `WebappsActor` is going to be instanciated once
> per connection?
> If that's the case, each client is going to have it's own WebappsActor's
> _appActorMap Map. This map allows to reuse already instanciated actor, but
> only for the given WebappsActor instance. So that we will create new tab
> actors for each new client that comes up.

Okay, I see. That seems fine.


> Also, actors are big beast and there is a lot of specific details for each
> tool (console vs debugger), so I can easily be missing some cleanup and
> leave some low level actors when reusing the same ContentTabActor
> instance... But I verified, it works well with webconsole and debugger, we
> can reuse the same ContentTabActor multiple times with these fixes.

Okay. Let's go with this, then.


> > ::: toolkit/devtools/server/actors/webapps.js
> > @@ +420,5 @@
> > > +    mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
> > > +
> > > +    let childTransport, prefix;
> > > +
> > > +    let onActorCreated = (function (msg) {
> > 
> > Also: you should use makeInfallible here, to make sure errors aren't lost.
> 
> I'm concerned with this practice, that is very viral and doesn't look free
> performance-wise. Shouldn't we patch APIs that are silently ignoring
> exceptions?

I would welcome such a patch to the message managers. Until that happens, though, this saves a lot of developer time. Why would the performance be bad?


> In anycase, I modified the patch to use it.

Thanks. We've found it very useful elsewhere in the debugger.
Comment on attachment 772690 [details] [diff] [review]
master - Expose tab actors for apps in child processes, addressed review comments.

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

It's a pity that webbrowser.js gives you the root actor whether you want it or not. You should add a second optional parameter to _onConnection that, if true, disables creating the root actor.

::: toolkit/devtools/server/actors/webapps.js
@@ +33,5 @@
> +
> +  // Keep reference of already created app actors.
> +  // key: app iframe, value: ContentTabActor's grip() value
> +  // Note that it would have been ideal to use messageManager as key,
> +  // but for some reasons they can't be weakmap keys :(

It's not a WeakMap any more, so the comment should be fixed.

Would using the MM as a key be a worthwhile cleanup? If so, should we do it in this bug, or in a follow-up?

::: toolkit/devtools/server/child.js
@@ +1,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
> +Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> +
> +addMessageListener('debug:connect', DevToolsUtils.makeInfallible(function (msg) {

Each time we get a 'getAppActor' request, we're going to loadFrameScript, and add this message listener, right? So the listener function needs to remove the listener, lest the second debug:connect sent by the parent get two debug:actor replies, the third three, the fourth four, etc.

@@ +6,5 @@
> +  let mm = msg.target;
> +
> +  let prefix = msg.data.prefix + docShell.appId;
> +
> +  if (!DebuggerServer.initialized) {

This whole 'if' should be at the top level in this script, I think.

@@ +18,5 @@
> +
> +  // In case of apps being loaded in parent process, DebugedServer is already
> +  // initialized, but contenttab.js hasn't been loaded yet.
> +  if (!("ContentTabActor" in DebuggerServer)) {
> +    DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/contenttab.js");

Said in a previous comment, but: we should just extend addBrowserActors to load contenttab.js, or give it a new sibling named 'addContentChildActors' or something.

This should also be folded up into the 'if' above, and moved to the top level along with it.
Attachment #772690 - Flags: review?(jimb) → review+
Comment on attachment 771333 [details] [diff] [review]
b2g-18 - Expose tab actors for apps in child processes.

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

Looks good; r=me, with any needed and appropriate fixes for the trunk patch backported.
Attachment #771333 - Flags: review?(jimb) → review+
New interdiff:
https://github.com/ochameau/mozilla-central/commit/55e9a1a7dc780de6c716be2a09c2bf1e27dd8f7d

Here is some comments about addressing these comments:
* Map accepts message manager as keys, so I switched to that!
* I added a DebuggerServer.addChildActors, a bit more complex than the addBrowserActors
as we have to check whether actors are already loaded or not. 
That's because we call this method in parent and child.
In parent, browser actors are already loaded and we only need to load childtab.js
whereas in child, we need to load all actors.
Otherwise, I haven't reused addBrowserActors, as we do not want to load webapps.js,
not the chromeDebugger in child as they are meant to be loaded only once in chrome process.
* I tuned DebuggerServer._onConnection to avoid creating the root actor.
That led me to add an additional check in getActor as the root actor can be undefined.
Attachment #772690 - Attachment is obsolete: true
Attachment #773966 - Flags: review+
This bug and its dependencies would make it possible to debug web applications (console, js debugger, profiler) on 1.1 devices. I think this is a very important feature and therefore I'd like to nominate it leo.

Surely this has some risk, the dependency list is quite high, and there are adhoc patches for the b2g18 branch. But the reward is quite high too. Plus, most touched files are not part of the "normal" build, rather they are used when we use the devtools, which reduces the risk.
blocking-b2g: --- → leo?
not a blocker at this late stage in the game.
blocking-b2g: leo? → -
Blocks: gaia-devtools
Blocks: 893813
Blocks: 894352
Additional patch on top of the main one to pref it off.
So until we get unix domain socket, developers will have to set a custom pref
to get remote app debugger to work: devtools.debugger.enable-content-actors

I've not only disabled content actor, but also recent webapp actor features
that may introduce security leaks if requested from an app...
But I kept the install method as it is expected to work.
(this method is safe as it rely on putting a file in /tmp folder, and no app can do that)
Attachment #779854 - Flags: review?(jimb)
Attachment #779854 - Flags: review?(jimb) → review?(paul)
Attachment #779854 - Flags: review?(paul) → review+
Can we land that?
Yes, both two last attachments. But we should just do sanity checks to ensure that these patches still apply correctly to last master as the big patch is 13days old.
Attachment #771333 - Attachment is obsolete: true
Rebased patch.
Attachment #773966 - Attachment is obsolete: true
Attachment #780407 - Flags: review+
That's now ready to land, the two patches I just rebased. It depends on bug 878958 attachment 774787 [details] [diff] [review] that has been r+ but not landed.

(While you are landing patches, it would be worth also landing bug 893848 attachment 779180 [details] [diff] [review] that broke the webapps actor completely)
Keywords: checkin-needed
(In reply to Alexandre Poirot (:ochameau) from comment #56)
> (While you are landing patches, it would be worth also landing bug 893848
> attachment 779180 [details] [diff] [review] that broke the webapps actor completely)

Attachment 780408 [details] [diff] already has this fix AFAICT.

https://hg.mozilla.org/projects/birch/rev/738b7898b6c7
https://hg.mozilla.org/projects/birch/rev/36323782eaa3
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: