878958 - Implement child process actors

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P1)

Description

[Jim Blandy :jimb]

We have a draft protocol specification for communication with child processes here:

https://github.com/jimblandy/DebuggerDocs/blob/b2g-subprocesses/protocol

This includes packets for listing child processes, getting notification when children start and stop, and attaching to particular child processes, yielding a connection to a root actor for that particular child.

Comment 1

[Jim Blandy :jimb]

Attachment: Use arrow functions in JS debugger client code.

Comment 2

[Jim Blandy :jimb]

Attachment: Use a Map to map actor names to reply handlers, not an object.

Comment 3

[Jim Blandy :jimb]

Attachment: In DebuggerClient, substitute mainRoot for _connected.

Comment 4

[Jim Blandy :jimb]

Attachment: Separate out a function for setting reply handlers.

Comment 5

[Jim Blandy :jimb]

Attachment: Handle a greeting packet as if it were reply from the root actor.

Comment 6

[Jim Blandy :jimb]

Attachment: Implement ChildDebuggerTransport, a debug protocol transport for communicating with content child processes via process message managers.

Comment 7

[Jim Blandy :jimb]

Attachment: Avoid hard-coding the root actor name in DebuggerServerConnection.

Comment 8

[Jim Blandy :jimb]

Attachment: Support connections with prefixed actor names, and prefix-based forwarding.

Comment 9

[Jim Blandy :jimb]

Attachment: Implement @mozilla.org/DebuggerService;1, for starting servers in child processes.

Comment 10

[Jim Blandy :jimb]

Attachment: Define ChildClient; add RootClient attachChild method; fill in ChildProcessActor.

Comment 11

[Jim Blandy :jimb]

With this, we can attach to child processes, exchange packets with their root actors, and attach to a chrome debugger actor.

We can't actually do chrome debugging, because our root actor isn't quite disentangled from the browser code. But at least it dies trying to pause the chrome. Filed as bug 880930.

You'll need all the patches attached to

Comment 12

[Jim Blandy :jimb]

[ahem]

With the patches in this bug and its dependencies, the xpcshell test toolkit/devtools/server/tests/unit/test_child_attach.js is able to attach to a content child process, and attempt to debug its chrome.

To experience the thrill yourself, you'll need all the patches on these bugs applied, in this order:

Bug 874755: Implement nsIContentParent and nsIContentProcessService, for enumerating and controlling content processes.

Bug 878901: Create a separate client front object, RootClient, for communicating with root actors.

Bug 874753: JS debugger server: Implement the listChildProcesses request in RootActor

... followed by the patches in this bug.

I've pushed the current state of my Mercurial Queue here:

http://hg.mozilla.org/users/jblandy_mozilla.com/jsdbg2-mq/

That also includes some error-tightening patches.

Comment 13

[Jim Blandy :jimb]

Attachment: Define ChildClient; add RootClient attachChild method; fill in ChildProcessActor.

With this patch, toolkit/devtools/server/tests/unit/test_child_attach.js has been observed to attach to a content child process, begin chrome debugging, and list the loaded JS sources.

Comment 14

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70aac30d291d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26e0070bb65
https://hg.mozilla.org/integration/mozilla-inbound/rev/72dc9a2bbc58
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ef467a1f18
https://hg.mozilla.org/integration/mozilla-inbound/rev/05183deb734a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70aac30d291d
https://hg.mozilla.org/mozilla-central/rev/d26e0070bb65
https://hg.mozilla.org/mozilla-central/rev/72dc9a2bbc58
https://hg.mozilla.org/mozilla-central/rev/f1ef467a1f18
https://hg.mozilla.org/mozilla-central/rev/05183deb734a

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: b2g18 - Support connections with prefixed actor names, and prefix-based forwarding.

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 771310 [details] [diff] [review]
b2g18 - Support connections with prefixed actor names, and prefix-based forwarding.

Here is the minimal modification required to make tab actors work.
This patch is targetting b2g18 branch.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: b2g18 - Implement ChildDebuggerTransport, a debug protocol transport for communicating with child processes via message managers.

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 771313 [details] [diff] [review]
b2g18 - Implement ChildDebuggerTransport, a debug protocol transport for communicating with child processes via message managers.

Same patch than attachment 760055 [details] [diff] [review], but rebased against b2g18.

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 760055 [details] [diff] [review]
Implement ChildDebuggerTransport, a debug protocol transport for communicating with content child processes via process message managers.

We need two patches from this bug for tab actors on b2g.
This one.. and...

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 760057 [details] [diff] [review]
Support connections with prefixed actor names, and prefix-based forwarding.

...and this one.

(You can also see b2g18 rebased patches in this bug)

Comment 22

[Dave Camp (:dcamp)]

Comment on attachment 760055 [details] [diff] [review]
Implement ChildDebuggerTransport, a debug protocol transport for communicating with content child processes via process message managers.

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

r+ when it has tests

Comment 23

[Dave Camp (:dcamp)]

Comment on attachment 760057 [details] [diff] [review]
Support connections with prefixed actor names, and prefix-based forwarding.

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

r+ when tested

Comment 24

[Dave Camp (:dcamp)]

Comment on attachment 760055 [details] [diff] [review]
Implement ChildDebuggerTransport, a debug protocol transport for communicating with content child processes via process message managers.

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

Switching to r+ after discussion of our testing options.

Comment 25

[Jim Blandy :jimb]

Attachment: Support connections with prefixed actor names, and prefix-based forwarding.

Now, with tests!

Comment 26

[Jim Blandy :jimb]

Attachment: Support connections with prefixed actor names, and prefix-based forwarding.

Now, with tests! (And without debugging 'dump' calls!)

Comment 27

[Jim Blandy :jimb]

Comment on attachment 760055 [details] [diff] [review]
Implement ChildDebuggerTransport, a debug protocol transport for communicating with content child processes via process message managers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/26e08cf5aabe

Comment 28

[Jim Blandy :jimb]

Comment on attachment 771313 [details] [diff] [review]
b2g18 - Implement ChildDebuggerTransport, a debug protocol transport for communicating with child processes via message managers.

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

Pre-emptive r=me; the corresponding trunk patch is already landed on inbound.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/26e08cf5aabe

Comment 30

[Alexandre Poirot [:ochameau]]

checkin needed for attachment 774787 [details] [diff] [review]

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/fedda0ce50c8

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Eddy Bruel [:ejpbruel]]

What do we still need to be able to resolve this bug?

Comment 34

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

I am pretty sure this is done, right Alex?

Comment 35

[Alexandre Poirot [:ochameau]]

Thing is that I kind of hijacked Jim's plan for debugging apps in middle of its implementation.
The original goal was to implement a process-based abstraction to connect and debug apps.
Whereas I ended up implementing a more app-based / iframe-based / message manager-based solution.

Patch like attachment 760059 [details] [diff] [review] may still be relevant for advanced debugging of e10s processes where we may share processes between tabs/apps and have a more complex relation between contextes (app/tabs) and processes.

I didn't wanted to close all existing bugs just because we were able to debug apps.

Comment 36

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

Given that we already have a way to reach the child process through the messageManager and we're also thinking of converting to the Chrome protocol as well, I don't think we would try to change our approach for reaching the child process in our own protocol anymore.

If the situation changes, we can always revisit this work later.

Comment 37

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

I suppose some pieces did land here... unclear what the correct resolution state is, since we didn't make it all the way to the part described in the summary.