777508 - Notify parent if child processes are terminated through message manager

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

[Olli Pettay [:smaug][[e-mail]]]

I think we could send some generic
MessageManager:Disconnect message, and the data could tell whether disconnection was clean or not.

Comment 3

[Gregor Wagner [:gwagner]]

(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

[Olli Pettay [:smaug][[e-mail]]]

Yeah, observer+notification is perhaps even better.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

We already do "ipc:content-shutdown" in certain cases.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, the notification needs to provide enough information to identify the mm for the child process.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

 ... so firing it from that mm seems like a good approach.

Comment 8

[Andrew Overholt [:overholt]]

Chris, is this something you could take?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Not really, no.

Maybe Phil can take this?

Comment 10

[Philipp von Weitershausen [:philikon]]

So what's the goal here? Send a message to parent process message senders when their child process ends (whatever the reason may be)?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That seems like the easiest approach.

Comment 12

[Gregor Wagner [:gwagner]]

Attachment: first try

Comment 13

[Philipp von Weitershausen [:philikon]]

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.

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: 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?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Addressed review comments.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 18

[Gregor Wagner [:gwagner]]

Can we land this?

Comment 19

[Philipp von Weitershausen [:philikon]]

(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?

Comment 20

[Gregor Wagner [:gwagner]]

(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!

Comment 21

[Philipp von Weitershausen [:philikon]]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=5c4ca3fb9423

Comment 22

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/965397b043c0

Comment 23

[Ed Morley [:emorley]]

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

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dc877eb2b8

Comment 25

[Ed Morley [:emorley]]

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