1049103 - e10s: closing the Browser Console breaks the toolbox

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Mihai Sucan [:msucan]]

STR:

Enable e10s, remote tabs (browser.tabs.remote.autostart=true), and restart firefox.

1. Open the toolbox for a tab.
2. Open the Browser Console.
3. Close the Browser Console.
4. Try to use the webconsole or the inspector.

You'll see everything is broken. The terminal only shows:

###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID

Comment 1

[Mihai Sucan [:msucan]]

From some testing and investigation I did for a webconsole test (for bug 1042253) which does what the STR suggests, I found that the client sends packets to the server (over rdp) which are received by the server. Apparently, they are forwarded correctly through the child transport - sendAsyncMessage() with the message manager. However, there's nothing on the receiving end.

Alex, any ideas why the browser console can break the toolbox so badly? I had the impression that the server/client mixes things on disconnect. Someehow the browser console disconnect breaks the state of the other connection.

Thanks!

Comment 2

[Alexandre Poirot [:ochameau]]

This is really disturbing as browser toolbox should spawn a new loader and everything should be segregated... May be we end up sharing stuff within the content process by sharing the message manager.

Comment 3

[Alexandre Poirot [:ochameau]]

OMG yes, the RDP forwarding between the parent and child processes is designed to have only one instance.
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#644
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/child.js#47
We should somehow allow multiple parent<->child RDP forwards.
I think debug:connect message implementation is fine.
It should be about allowing multiple `conn` to be created and closing only the expected one.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: ensure closing only one child connection when closing a toolbox

Here is a patch that should fix these issues.
The problem was mainly in the debug:disconnect listener,
where we were closing the child whenever we received an event.
It ends up being bad as soon as there is more than one debugger server connected to a child.

https://tbpl.mozilla.org/?tree=Try&rev=5b849bf9569b

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8474612 [details] [diff] [review]
ensure closing only one child connection when closing a toolbox

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

::: toolkit/devtools/server/main.js
@@ +566,5 @@
>               .messageManager;
>      mm.loadFrameScript("resource://gre/modules/devtools/server/child.js", false);
>  
>      let actor, childTransport;
> +    let prefix = aConnection.allocID("child") + generateUUID();

Can we use a incremented int instead of a UUID?  I am just thinking of what the RDP logs would look like for debugging... A UUID might be might it kind of nasty to follow the traffic.  Not a big deal though.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> Comment on attachment 8474612 [details] [diff] [review]
> > +    let prefix = aConnection.allocID("child") + generateUUID();
> 
> Can we use a incremented int instead of a UUID?  I am just thinking of what
> the RDP logs would look like for debugging... A UUID might be might it kind
> of nasty to follow the traffic.  Not a big deal though.

I wish I could use something else as well but lacked of ideas on how to compute a safe incremental id.
There is a slight chance to collide ids between two DebuggerServer instances loaded in two distinct loaders.
(regular toolbox server and browser toolbox server for ex).
If I put an idea like "DebuggerServer.serverID++", it will be colliding between loaders.
May be I can compute an incremental id on child.js child? Pipe it back on debug:connect response and use it for debug:disconnect?

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: ensure closing only one child connection when closing a toolbox

Here is a new patch that tries to compute an ID from child.js side.
It is slightly more complex but simplify the protocol values...

https://tbpl.mozilla.org/?tree=Try&rev=8a2ec4117cf5

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Ensure closing only one child connection when closing a toolbox

Do not use `let childID = 1` in child.js as this JS file is evaluated each time
we call connectToChild, so that childID is always set to 1...
Instead I'm hacking an attribute on DebuggerServer.

https://tbpl.mozilla.org/?tree=Try&rev=9926af0d92ed

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8475984 [details] [diff] [review]
Ensure closing only one child connection when closing a toolbox

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

The DebuggerServer.childID isn't very elegant (if you have any suggestions...),
but I think this version is much better than the uuid!

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8475984 [details] [diff] [review]
Ensure closing only one child connection when closing a toolbox

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

If it's possible to construct, we really should have a test for this so we can avoid breaking it again.

I'd like to see that, or hear why it's not possible before r+.

::: toolkit/devtools/server/child.js
@@ +33,5 @@
>      removeMessageListener("debug:connect", onConnect);
>  
>      let mm = msg.target;
> +    let prefix = msg.data.prefix;
> +    let id = DebuggerServer.childID++;

If you prefer it, you could instead write:

    let id = ++DebuggerServer.childID || (DebuggerServer.childID = 1);

and then you can remove the init step above.

Either way, I think using an attribute on DebuggerServer is fine for this purpose.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: patch with simple test

I haven't had time to test it with --e10s,
but a test like that should cover this codepath
when run in e10s mode.

https://tbpl.mozilla.org/?tree=Try&rev=9d169400d354

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch with commented test

Same patch, with some comments to help follow the test.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: patch with green test

The previous test was failing in e10s mode...
This one pass in both modes.

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8479116 [details] [diff] [review]
patch with green test

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

So, this test will only be useful once we will run this mochitest in e10s mode,
hopefully it will be soon.
https://tbpl.mozilla.org/?tree=Try&rev=bd3056a3c362

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8479116 [details] [diff] [review]
patch with green test

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

Cool, thanks for working on a test for this! :D

Comment 17

[Alexandre Poirot [:ochameau]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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