983610 - The developer HUD doesn't work for certified apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Jan Keromnes [:janx]]

Currently, most of the developer HUD widgets don't work for certified apps unless you set the "devtools.debugger.forbid-certified-apps" pref to false.

To fix this, the developer HUD needs to bypass the webapps actor by getting frame actors directly from `DebuggerServer.connectToChild(conn, mm, onDisconnect)`.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Make the developer HUD work for certified apps. r=ochameau

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8391382 [details] [diff] [review]
Make the developer HUD work for certified apps. r=ochameau

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

This bypasses the webapps actor completely, and mostly works.

Mostly, because of bug #983618's race condition for in-process apps (e.g. the keyboard), hence my asking for feedback and not review. Sadly, this can't land before bug #983618 is fixed.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8391382 [details] [diff] [review]
Make the developer HUD work for certified apps. r=ochameau

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

Looks good!

::: b2g/chrome/content/devtools.js
@@ +66,1 @@
>      this._client.connect((type, traits) => {

Do we even have to call connect? (tbh, I don't know!)

Comment 4

[Jan Keromnes [:janx]]

Attachment: Make the developer HUD work for certified apps. r=ochameau

Comment 5

[Jan Keromnes [:janx]]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> Looks good!

Thanks! Actually after rebasing it even looks like the race condition went away, I'm not sure what happened but I'll investigate.

> ::: b2g/chrome/content/devtools.js
> @@ +66,1 @@
> >      this._client.connect((type, traits) => {
> 
> Do we even have to call connect? (tbh, I don't know!)

I think you're right, connect[1] seems to only call `transport.ready()` and then set `this.traits` when connected. In our case `transport` is a LocalDebuggerTransport created by `DebuggerServer.connectPipe()` so it has an empty ready[2] method. No point in calling it. 

[1] DebuggerClient.connect: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#362-371
[2] LocalDebuggerTransport.ready: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/transport.js#288

Comment 6

[Jan Keromnes [:janx]]

(In reply to Jan Keromnes [:janx] from comment #5)
> Thanks! Actually after rebasing it even looks like the race condition went
> away, I'm not sure what happened but I'll investigate.

Apparently the keyboard app switched to being remote instead of inprocess, so the HUD now works there. Since the problem affects inprocess frames, it still happens for the browser. Are there other inprocess apps left in firefox os?

Comment 7

[Jan Keromnes [:janx]]

Attachment: Make the developer HUD work for certified apps. r=ochameau

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8394206 [details] [diff] [review]
Make the developer HUD work for certified apps. r=ochameau

This patch greatly improves the situation, because no workaround is needed for certified apps anymore. I filed bug #987154 for the browser app issue, so that it can be addressed as a follow-up.

Comment 9

[Jan Keromnes [:janx]]

Comment on attachment 8395686 [details] [diff] [review]
Make the developer HUD work for certified apps. r=ochameau

Sorry, wrong attachment.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8395686 [details] [diff] [review]
Make the developer HUD work for certified apps. r=ochameau

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

Looks good!

::: b2g/chrome/content/devtools.js
@@ +62,5 @@
>      }
>  
> +    let transport = DebuggerServer.connectPipe();
> +    this._conn = transport._serverConnection;
> +    this._client = new DebuggerClient(transport);

It would be handy to add a comment about out "kind of special" usage of DebuggerServer!
Something like:
"We instanciate a local debugger connection,
in order to later query apps tab actors like webconsole actor.
Note the special usage of _serverConnection debugger server internal.
This is meants to be given to DebuggerServer.connectToChild,
just to make it work as it requires a connection to pipe child process actors messages to
the parent process, and will allow to query apps tab actors on our DebuggerClient instance!"
But please, feel free to document it with your own words!!

Comment 11

[Jan Keromnes [:janx]]

Attachment: Make the developer HUD work for certified apps. r=ochameau

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8396342 [details] [diff] [review]
Make the developer HUD work for certified apps. r=ochameau

Thanks for the r+! I added a comment to explain the _serverConnection trick.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Carsten Book [:Tomcat]]

hi, sorry had to back this out in https://tbpl.mozilla.org/?rev=9afe2a1145bd since one of this 3 pushes seems to have caused Bug 986173

Comment 16

[Ryan VanderMeulen [:RyanVM]]

We disabled the test for now since it seems to just be sensitive to devtools changes in general.
https://hg.mozilla.org/integration/fx-team/rev/f4e0eb9671c8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4e0eb9671c8