bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

e10s: closing the Browser Console breaks the toolbox

RESOLVED FIXED in Firefox 34

Status

DevTools
Console
P2
normal
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: msucan, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 34
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [e10s])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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)
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
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)
tracking-e10s: --- → +
Duplicate of this bug: 1052815
(Assignee)

Comment 5

4 years ago
Created attachment 8474612 [details] [diff] [review]
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 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.
(Assignee)

Comment 7

4 years ago
(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?
(Assignee)

Comment 8

4 years ago
Created attachment 8475957 [details] [diff] [review]
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
Attachment #8474612 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8475984 [details] [diff] [review]
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
Attachment #8475957 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
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+
(Assignee)

Comment 12

4 years ago
Created attachment 8476755 [details] [diff] [review]
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
Attachment #8475984 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8479046 [details] [diff] [review]
patch with commented test

Same patch, with some comments to help follow the test.
Attachment #8476755 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8479116 [details] [diff] [review]
patch with green test

The previous test was failing in e10s mode...
This one pass in both modes.
Attachment #8479046 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 977043
(Assignee)

Updated

4 years ago
Assignee: nobody → poirot.alex
https://hg.mozilla.org/mozilla-central/rev/a23b7b404770
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.