962577 - Refactor webapps actor's _connectToApp to make it reusable.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jan Keromnes [:janx]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=960933#c10 :

One option could be to factor out its _connectToApp function:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#792

In order to create the app actor by ourself. We may have to do that to support Firefox/Fennec apps, the watchApps and getAppActor will most likely be factored out to better support multiple platforms.

Comment 1

[Jan Keromnes [:janx]]

After talking with Alex we decided that bugs 963490 and 962577 needed better titles.

Renaming this bug to "Refactor webapps actor's _connectToApp".

This method takes a frame and returns its app actor. Once refactored it should be used by the devtools layers (bug #960933) to get app actors without going through the webapps actor. Removing the layers' dependency on the webapps actor will also make them work on certified apps without a pref.

Comment 2

[Jan Keromnes [:janx]]

Attachment: Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8371642 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

`git bz` stubbornly refuses to set r? flags...

Alex, could you take a look? I'm sending a message manager instead of a frame because the caller needs to do stuff with it (i.e. listen for "debug:actor" to memoize the value).

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8371642 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

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

::: toolkit/devtools/server/main.js
@@ +513,5 @@
>      return this._onConnection(transport, aPrefix, true);
>    },
>  
> +  /**
> +   * Hehe.

Oops, this comment is not very helpful.

Comment 5

[Jan Keromnes [:janx]]

Attachment: Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8371659 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

Attempt #2.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8371659 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

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

Looks good with the following comments addressed.
Thanks for providing this patch so promptly!

Ensure testing on a device with a old and a recent firefox,
to prevent introducing cross-version regressions.

Panos, Do you think DebuggerServer is a good placeholder for connectToApp?
I think this method can be shared to debug remote firefox tabs and may be also workers if we use a message manager to communicate with them.

::: toolkit/devtools/server/actors/webapps.js
@@ +828,5 @@
>        }
>  
> +      function onActorCreated(msg) {
> +        mm.removeMessageListener("debug:actor", onActorCreated);
> +        map.set(mm, msg.json.actor);

You can move that to the promise resolution instead of listening to debug:actor.

@@ +841,5 @@
> +            // The ContentAppActor within the child process doesn't necessary
> +            // have to time to uninitialize itself when the app is closed/killed.
> +            // So ensure telling the client that the related actor is detached.
> +            aConnection.send({ from: actor.actor,
> +                             type: "tabDetached" });

Ideally, this call should be in connectToApp, as that's not webapps actor specific. We will most likely want to dispatch this tabDetached event also for firefox remote tabs and/or workers.
It would introduce some state in this method as it would require saving both the related connection and actor for each message manager. That state can introduce issues when using multiple connection if we end up creating mulitple actors for the same message manager.

I'm fine figuring this out later, as your current patch replicate the current implementation/behavior of the webapps actor!

@@ +846,5 @@
> +            map.delete(mm);
> +          }
> +        }
> +      }
> +      Services.obs.addObserver(onMessageManagerDisconnect, "message-manager-disconnect", false);

It feels like message-manager-disconnect is an internal event to connectToApp, it may change depending on how it is being implemented.
What do you think about adding a onDestroyed callback to connectToApp or any other way to do similar abstraction?

Comment 8

[Jan Keromnes [:janx]]

Attachment: Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

Comment 9

[Jan Keromnes [:janx]]

Attachment: Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8373433 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

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

This patch seems to work somewhat (see comment about error). I consider the changes significant enough for another review.

> Panos, Do you think DebuggerServer is a good placeholder for connectToApp?
> I think this method can be shared to debug remote firefox tabs and may be also workers if we use a
> message manager to communicate with them.

Carrying over the feedback request.

::: toolkit/devtools/server/main.js
@@ +580,5 @@
> +    }
> +    Services.obs.addObserver(onMessageManagerDisconnect, "message-manager-disconnect", false);
> +
> +    let prefixStart = aConnection.prefix + "child";
> +    mm.sendAsyncMessage("debug:connect", { prefix: prefixStart });

This line sometimes breaks with the following error, preventing an app from being tracked:

E/GeckoConsole(  975): [JavaScript Error: "[Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js :: DebuggerServer.connectToApp :: line 584"  data: no]" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js" line: 584}]

I'm not sure how to wait for the message manager to be properly initialized.

Comment 11

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

Comment on attachment 8373433 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

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

OK, I've been looking at this for an hour and I'm still confused. I'm going to post my initial thoughts anyway while I keep thinking about it.

::: b2g/chrome/content/devtools.js
@@ +46,5 @@
>        RemoteDebugger.start();
>      }
>  
> +    let transport = DebuggerServer.connectPipe();
> +    this._conn = transport._serverConnection;

Wait, what? Why? This is something that should never be used in our code, with the possible exception of tests (which also has to be convincingly argued)!

::: toolkit/devtools/server/main.js
@@ +516,5 @@
>  
> +  /**
> +   * Create a new connection to an app, returning an app actor promise.
> +   */
> +  connectToApp: function (aConnection, aMessageManager, onDisconnect) {

Please document the arguments in the method comment and explain thoroughly the purpose of this method and how it differs from the other connectFoo methods.

@@ +529,5 @@
> +
> +    function onActorCreated(msg) {
> +      mm.removeMessageListener("debug:actor", onActorCreated);
> +
> +      dump("***** Got debug:actor\n");

Remove debugging code before landing please.

@@ +543,5 @@
> +      childTransport.ready();
> +
> +      aConnection.setForwarding(prefix, childTransport);
> +
> +      dump("establishing forwarding for app with prefix " + prefix);

If you absolutely must leave logging statements in the code use dumpn(), which is only enabled when devtools.debugger.log is turned on.

Comment 12

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

Comment on attachment 8373433 [details] [diff] [review]
Refactor webapps actor's _connectToApp and make it accessible outside the actor. r=ochameau

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

So, while I agree that DebuggerServer is a good place for a connectToApp method, it seems to me that the way it works is backwards. Indeed, as Alex says forthcoming e10s work will require a similar solution for desktop Firefox and possibly even Android. I was hoping that we could reuse or slightly tweak the connectToParent method and related machinery, but I haven't looked closely at it, so I may be way off. If so, then perhaps connectToApp will be closer to what we need, but like I said, I'm not sure.

My problem is that the only way I see connectToApp being used is by breaking the client-server boundary (having the client use transport._serverConnection), which does not provide for a clean API. As it is, connectToApp resides on the server, but is supposed to be called by the client, once the right message manager has been retrieved in order to be used as a parameter. So the client has to connect to the server first, find out the app it needs to interact with and then somehow make the server establish an internal connection in the backend with the app.

This is more complicated and confusing than the current implementation, which goes through the webapps actor, but has clearly defined semantics, and follows closely the established server architecture. I'm not sure I fully understand the reasoning behind the approach in this patch, but it looks like the problem is that the webapps actor takes some responsibilities for listing apps, which otherwise reside with the root actor. That is indeed something that sticks out, but it doesn't look to me like this change is an improvement over what we currently have.

I wouldn't be surprised if I'm missing something in your thought process though, so please enlighten me if I have gotten it wrong. I definitely want to go over our future plans here with you guys in person next week!

Comment 13

[Alexandre Poirot [:ochameau]]

Yes, let's discuss that in details during the workweek.

Let me drop down the main reason why I initiated such move:

* Frames:

  So far the webapps actor only targets main app frames,
  but there is many other similar contexts, running OOP (not always),
  related to apps, but not always:
   - App sheets:
     gaia has a UX project called "haida", where apps are going to use multiple frames
   - Attention screen are spawn in app process but in another frame
   - Activities, same thing than attention screen
   - New window opened via window.open(), same thing again
  We would like to start exposing more than just the main app frame.
  The webapps actors is going to expose tab actors for each of these contexts/frames.
  But the important point is that, content frames, the frames opened for bookmark apps,
  or browser tabs, are also loaded in a frame and we are going to reimplement all
  connectToApp/child.js/childtab.js code if we don't share this code.

* bug 917227 - network actor

  We are about to add network-related code in middle of the webapps actor,
that has nothing to do with apps! And we will need the same code for content frames.

* bug 960933 - devtools layers

  When implementing the devtools layers, we need tab actors for any apps,
even if the special "forbid-certified" pref is on... That the first time I started thinking about using connectToApp directly from chrome code. To not depend on the webapps actor and bypass this pref. But that particular usage of connectToApp ends up being weird and mess up with client/server. That's where we have to use connectPipe's transport internal.
  I think that usage, and that part of the patch is misleading. And I wish we could still bypass the webapps actor and fetch tab actors for any apps without any such hack. The devtools layers are connecting locally thought connectPipe, so that they have access to the same actors/features than a regular client. But at the end we would like to have something else without any restriction...

* tests

  By extracting this method out of the webapps actors, it opens ways to unit test this particular code without having to emulate all apps ecosystem. We currently don't have any test runner on b2g that allow to write a chrome test with a running gaia! So it ends up challenging to make the whole webapps actor to work as it currently depends on gaia.

* e10s

  There is almost no difference between a firefox OS bookmark app/browser tab and a firefox desktop remote tab. There is also very few differences between those and a OOP app!
I think we won't just share connectToParent, but connectToApp, child.js and childtab.js.

Comment 14

[Alexandre Poirot [:ochameau]]

Jan, can you ensure having attachment 8385552 [details] [diff] [review] renames in your patch?

Comment 15

[Jan Keromnes [:janx]]

I already have them: https://github.com/jankeromnes/gecko-dev/blob/connecttochild/toolkit/devtools/server/main.js#L518

Comment 16

[Jan Keromnes [:janx]]

Attachment: Refactor webapps actor's _connectToApp to make it reusable. r=ochameau

Comment 17

[Jan Keromnes [:janx]]

Attachment: _connectToApp → connectToChild (function diff)

easier to review function diff

Comment 18

[Jan Keromnes [:janx]]

Comment on attachment 8386758 [details] [diff] [review]
Refactor webapps actor's _connectToApp to make it reusable. r=ochameau

This patch only extracts _connectToApp from the webapps actor, but doesn't yet reuse it elsewhere.

Reusing it, e.g. in b2g/chrome/content/devtools.js, will be done in follow-ups.

Comment 19

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

Review ping? I'd like to land bug 937172, but it depends on this.

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8386758 [details] [diff] [review]
Refactor webapps actor's _connectToApp to make it reusable. r=ochameau

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

Bill, Sorry for the delay, I was off the last couple of days...

Looks good, just some nits.

Thanks Jan!

::: toolkit/devtools/server/actors/webapps.js
@@ +825,5 @@
> +        let map = this._appActorsMap;
> +        return DebuggerServer.connectToChild(this.conn, mm, mm => {map.delete(mm)}).then(actor => {
> +          map.set(mm, actor);
> +          return {actor: actor};
> +        });

This is hard to read. connectToChild arguments and promise are mixed up. What about highlighting a bit more the disconnect callback with new lines:
   return DebuggerServer.connectToChild(this.conn,
                                        mm,
                                        mm => map.delete(mm))
                        .then(actor => {
   		           map.set(mm, actor);
		           return {actor: actor};
		         });
Or by introducing inline onConnect/onDisconnect function,
or something...

::: toolkit/devtools/server/main.js
@@ +524,5 @@
>      let transport = new ChildDebuggerTransport(aMessageManager, aPrefix);
>      return this._onConnection(transport, aPrefix, true);
>    },
>  
> +  connectToChild: function(aConnection, aMessageManager, onDisconnect) {

nit: if you want to follow the code style of this file about argument, all arguments would be prefixed with `a`

@@ +531,5 @@
> +
> +    let mm = aMessageManager;
> +    mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
> +
> +    let childTransport, prefix;

nit: you could add `actor` to this variable list

Comment 21

[Jan Keromnes [:janx]]

Attachment: Refactor webapps actor's _connectToApp to make it reusable. r=ochameau

Comment 22

[Jan Keromnes [:janx]]

Comment on attachment 8388473 [details] [diff] [review]
Refactor webapps actor's _connectToApp to make it reusable. r=ochameau

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

ochameau's r+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a1b10b89d77