Closed Bug 777508 Opened 12 years ago Closed 12 years ago

Notify parent if child processes are terminated through message manager

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: gwagner, Assigned: philikon)

References

Details

(Whiteboard: [WebAPI:P0][LOE:S])

Attachments

(1 file, 2 obsolete files)

      No description provided.
Note, it's not just crashes we need to notify, but any time a subprocess shuts down.

It feels simpler to do bug 777200 in C++ to me, but I guess we want this mechanism regardless.
I think we could send some generic
MessageManager:Disconnect message, and the data could tell whether disconnection was clean or not.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Note, it's not just crashes we need to notify, but any time a subprocess
> shuts down.
> 
> It feels simpler to do bug 777200 in C++ to me, but I guess we want this
> mechanism regardless.

We could just use an observer and send a message for a regular shutdown.
Yeah, observer+notification is perhaps even better.
We already do "ipc:content-shutdown" in certain cases.
blocking-basecamp: --- → +
Note, the notification needs to provide enough information to identify the mm for the child process.
 ... so firing it from that mm seems like a good approach.
Chris, is this something you could take?
Assignee: nobody → jones.chris.g
Not really, no.

Maybe Phil can take this?
Assignee: jones.chris.g → philipp
Whiteboard: [WebAPI:P0]
So what's the goal here? Send a message to parent process message senders when their child process ends (whatever the reason may be)?
That seems like the easiest approach.
Whiteboard: [WebAPI:P0] → [WebAPI:P0][LOE:S]
Attached patch first try (obsolete) — Splinter Review
Summary: Message manager: send implicit information to parent if child crashes. → Notify parent if child processes are terminated through message manager
Comment on attachment 664713 [details] [diff] [review]
first try

Thanks, Gregor! The dom/apps portions of this patch should probably go into a separate bug. Do we have a bug on file for that yet? If not I can file one.
Attachment #664713 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
This has us notify process and frame MMs of child process shutdowns.

Tests depend on bug 776832 because I didn't know of a programmatic way to kill a child process from Mochitests other than the permission assertion. Bug 776832 is bogged down a bit by weird Mochitest-1 failures. If people need this bug to land before bug 776832, we can land the tests later, or maybe there's a different way to kill a frame process programmatically from JS?
Attachment #665151 - Flags: review?(jones.chris.g)
Comment on attachment 665151 [details] [diff] [review]
v1

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp

> void
> ContentParent::ActorDestroy(ActorDestroyReason why)
> {
>+    nsRefPtr<nsFrameMessageManager> ppm = mMessageManager;
>+    if (ppm) {
>+      ppm->ReceiveMessage(static_cast<nsIContentFrameMessageManager*>(ppm.get()),
>+                          NS_LITERAL_STRING("childprocess-shutdown"), false,

Nit: "child-process-shutdown".

Non-nit: Factor this out into a constant somewhere.  We don't this to
diverge accidentally.

Beyond that, \o/.  "Too easy", as they say in the Southern Hemisphere.

r=me with that.
Attachment #665151 - Flags: review?(jones.chris.g) → review+
Depends on: 794353
Attached patch v2Splinter Review
Addressed review comments.
Attachment #665151 - Attachment is obsolete: true
I wonder whether we should send the same notification when the in-process-child-mm dies too.  Maybe we should send it for all mms.

Fodder for a followup.
Can we land this?
(In reply to Gregor Wagner [:gwagner] from comment #18)
> Can we land this?

Short answer: maybe. Long answer: please see comment 14.

I'm working on getting bug 776832 in shape to land today. If that fails, I will land this but without the tests for now. Does that work for you?
(In reply to Philipp von Weitershausen [:philikon] from comment #19)
> (In reply to Gregor Wagner [:gwagner] from comment #18)
> > Can we land this?
> 
> Short answer: maybe. Long answer: please see comment 14.
> 
> I'm working on getting bug 776832 in shape to land today. If that fails, I
> will land this but without the tests for now. Does that work for you?

Sounds good. I will apply it locally then. Thanks!
No longer blocks: 777200
Blocks: 794353
No longer depends on: 794353
The three changesets in the push backed out for causing (/conflicting with the backout of):
https://tbpl.mozilla.org/php/getParsedLog.php?id=15626544&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d251fdb2e
https://hg.mozilla.org/mozilla-central/rev/c2dc877eb2b8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 796930
Depends on: 820353
No longer depends on: 945948
You need to log in before you can comment on or make changes to this bug.