If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Notify parent if child processes are terminated through message manager

RESOLVED FIXED in mozilla18

Status

()

Core
IPC
RESOLVED FIXED
5 years ago
4 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.

Comment 2

5 years ago
I think we could send some generic
MessageManager:Disconnect message, and the data could tell whether disconnection was clean or not.
(Reporter)

Comment 3

5 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.

Comment 4

5 years ago
Yeah, observer+notification is perhaps even better.

Comment 5

5 years ago
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

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

Updated

5 years ago
Blocks: 791911
(Reporter)

Comment 12

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

Updated

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

Updated

5 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

5 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

5 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

5 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!
Try build: https://tbpl.mozilla.org/?tree=Try&rev=5c4ca3fb9423
(Assignee)

Updated

5 years ago
No longer blocks: 777200
(Assignee)

Updated

5 years ago
Blocks: 794353
No longer depends on: 794353
https://hg.mozilla.org/integration/mozilla-inbound/rev/965397b043c0
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/integration/mozilla-inbound/rev/c2dc877eb2b8
https://hg.mozilla.org/mozilla-central/rev/c2dc877eb2b8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Depends on: 796930
Blocks: 795164

Updated

5 years ago
Depends on: 820353

Updated

4 years ago
Depends on: 945948

Updated

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