Status

defect
P1
normal
RESOLVED WONTFIX
6 years ago
11 months ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(11 attachments, 4 obsolete attachments)

4.62 KB, patch
dcamp
: review+
jimb
: checkin+
Details | Diff | Splinter Review
1.74 KB, patch
dcamp
: review+
jimb
: checkin+
Details | Diff | Splinter Review
956 bytes, patch
dcamp
: review+
jimb
: checkin+
Details | Diff | Splinter Review
1.16 KB, patch
dcamp
: review+
jimb
: checkin+
Details | Diff | Splinter Review
1.91 KB, patch
dcamp
: review+
jimb
: checkin+
Details | Diff | Splinter Review
3.65 KB, patch
dcamp
: review+
dcamp
: feedback+
jimb
: checkin+
Details | Diff | Splinter Review
829 bytes, patch
Details | Diff | Splinter Review
5.30 KB, patch
Details | Diff | Splinter Review
12.10 KB, patch
Details | Diff | Splinter Review
7.64 KB, patch
Details | Diff | Splinter Review
15.39 KB, patch
dcamp
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

6 years ago
Blocks: 879639
(Assignee)

Updated

6 years ago
No longer blocks: 797627
(Assignee)

Updated

6 years ago
No longer depends on: 878901
(Assignee)

Comment 1

6 years ago
Assignee: nobody → jimb
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #760060 - Flags: review-
(Assignee)

Comment 11

6 years ago
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
Depends on: 880930
(Assignee)

Comment 12

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

Updated

6 years ago
Attachment #760049 - Flags: review?(dcamp)
(Assignee)

Updated

6 years ago
Attachment #760050 - Flags: review?(dcamp)
(Assignee)

Updated

6 years ago
Attachment #760052 - Flags: review?(dcamp)
(Assignee)

Updated

6 years ago
Attachment #760053 - Flags: review?(dcamp)
(Assignee)

Updated

6 years ago
Attachment #760054 - Flags: review?(dcamp)
(Assignee)

Comment 13

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

Updated

6 years ago
Attachment #760060 - Attachment is obsolete: true

Updated

6 years ago
Attachment #760049 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #760050 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #760052 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #760053 - Flags: review?(dcamp) → review+

Updated

6 years ago
Attachment #760054 - Flags: review?(dcamp) → review+
(Assignee)

Updated

6 years ago
Attachment #760049 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #760050 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #760052 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #760053 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #760054 - Flags: checkin+
Priority: -- → P1
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.
Attachment #771310 - Attachment description: attachment 760057: Support connections with prefixed actor names, and prefix-based forwarding. → b2g18 - Support connections with prefixed actor names, and prefix-based forwarding.
Attachment #771310 - Attachment filename: Bug-878958---attachment-760057-Support-connections.patch → Bug-878958-Support-connections.patch
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.
Attachment #771313 - Attachment description: attachment 760055: Implement ChildDebuggerTransport, a debug protocol transport for communicating with child processes via message managers. → b2g18 - Implement ChildDebuggerTransport, a debug protocol transport for communicating with child processes via message managers.
Attachment #771313 - Attachment filename: Bug-878958---attachment-760055-Implement-ChildDebu.patch → Bug-878958---Implement-ChildDebu.patch
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...
Attachment #760055 - Flags: review?(dcamp)
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)
Attachment #760057 - Flags: review?(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
Attachment #760055 - Flags: review?(dcamp) → feedback+
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
Attachment #760057 - Flags: review?(dcamp) → feedback+
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.
Attachment #760055 - Flags: review+
(Assignee)

Comment 25

6 years ago
Now, with tests!
Attachment #760057 - Attachment is obsolete: true
Attachment #774785 - Flags: review?(dcamp)
(Assignee)

Comment 26

6 years ago
Now, with tests! (And without debugging 'dump' calls!)
Attachment #774785 - Attachment is obsolete: true
Attachment #774785 - Flags: review?(dcamp)
Attachment #774787 - Flags: review?(dcamp)
(Assignee)

Comment 27

6 years ago
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
Attachment #760055 - Flags: checkin+
(Assignee)

Comment 28

6 years ago
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.
Attachment #771313 - Flags: review+

Updated

6 years ago
Attachment #774787 - Flags: review?(dcamp) → review+
Attachment #774787 - Flags: checkin+
Attachment #771313 - Attachment is obsolete: true
What do we still need to be able to resolve this bug?
I am pretty sure this is done, right Alex?
Flags: needinfo?(poirot.alex)
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.
Flags: needinfo?(poirot.alex)
Summary: JS debugger: implement child process actors → Implement child process actors
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
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.

Updated

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