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

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!

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.

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.

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

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

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

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

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

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

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

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