Closed Bug 1049103 Opened 6 years ago Closed 6 years ago

e10s: closing the Browser Console breaks the toolbox

Categories

(DevTools :: Console, defect, P2)

x86_64
Linux
defect

Tracking

(e10s+)

RESOLVED FIXED
Firefox 34
Tracking Status
e10s + ---

People

(Reporter: msucan, Assigned: ochameau)

References

Details

(Whiteboard: [e10s])

Attachments

(1 file, 5 obsolete files)

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!
Flags: needinfo?(poirot.alex)
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.
Flags: needinfo?(poirot.alex)
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
Attachment #8474612 - Attachment is obsolete: true
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
Attachment #8475957 - Attachment is obsolete: true
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!
Attachment #8475984 - Flags: review?(jryans)
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.
Attachment #8475984 - Flags: review?(jryans) → feedback+
Attached patch patch with simple test (obsolete) — Splinter Review
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
Attachment #8475984 - Attachment is obsolete: true
Attached patch patch with commented test (obsolete) — Splinter Review
Same patch, with some comments to help follow the test.
Attachment #8476755 - Attachment is obsolete: true
The previous test was failing in e10s mode...
This one pass in both modes.
Attachment #8479046 - Attachment is obsolete: true
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
Attachment #8479116 - Flags: review?(jryans)
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
Attachment #8479116 - Flags: review?(jryans) → review+
Keywords: checkin-needed
Blocks: 977043
Assignee: nobody → poirot.alex
https://hg.mozilla.org/mozilla-central/rev/a23b7b404770
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.