Notify parent if child processes are terminated through message manager

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gwagner, Assigned: philikon)

Tracking

unspecified
mozilla18
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
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.
(Reporter)

Comment 3

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

Updated

6 years ago
Whiteboard: [WebAPI:P0] → [WebAPI:P0][LOE:S]

Updated

6 years ago
Blocks: 791911
(Reporter)

Comment 12

6 years ago
Created attachment 664713 [details] [diff] [review]
first try

Updated

6 years ago
Blocks: 783392
Blocks: 793361
(Assignee)

Updated

6 years ago
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
Created attachment 665151 [details] [diff] [review]
v1

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

Updated

6 years ago
Depends on: 794353
Created attachment 665206 [details] [diff] [review]
v2

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.
(Reporter)

Comment 18

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

Comment 20

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

Updated

6 years ago
No longer blocks: 777200
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

6 years ago
Depends on: 796930
Blocks: 795164

Updated

6 years ago
Depends on: 820353
Depends on: 945948
No longer depends on: 945948
You need to log in before you can comment on or make changes to this bug.