540097 - IPDL: Errors processing messages should result in the child process being terminated

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

Basically this includes any condition in which a Send*() or Call*() method would return false.  In the parent, we'll |kill(SIGKILL, child)|, and in the child, we'd commit suicide.

Comment 1

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

I've come across some cases where content-process code continues to attempt to send messages long after a channel error or Close(), and bad things happen.  It's not really the fault of that code, since the content-process contract is that it commits seppuku when anything goes wrong.  So, I'm going to co-opt this bug for finishing some of this work.

I've added a new |ProcessingError(what)| notification interface for top-level actors.  It's called whenever there's a "processing error", which includes sending messages to a closed channel, handler returns false, deserialization error, and more.  This complements the existing Close()/ActorDestroy(why) path on error and shutdown.

Also, I patched ContentChild to _exit(0) from ActorDestroy() for release builds.  This is a step in the right direction by eliding all the XPCOM shutdown goop, but probably still isn't quite what we want --- instead of Close()ing the channel or requesting Destroy() or whatever, we should just |kill(SIGKILL)| the content process from chome, as described in comment 0.  We can do this in a followup if/when shutdown becomes annoying.

Comment 2

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

Attachment: Add a ProcessingError(what) notification itnerface for top-level actors

Comment 3

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

Attachment: Fix content processeses to (i) _exit(0) from ActorDestroy() in release builds; (ii) _exit(0) on send-on-closed-channel; and (iii) abort on other processing errors

Comment 4

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

Attachment: Add a ProcessingError(what) notification interface for top-level actors

Fixed typo in qhead, and fixed the test a bit while I was at it.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 460642 [details] [diff] [review]
Add a ProcessingError(what) notification interface for top-level actors

Looks good.

Comment 6

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

I've nom'd this for blocking for two reasons: first, the contract with content-process code is that it doesn't need to check return values or care about IPC connectivity.  This patch fulfills the ipc/glue part of that contract by allowing ContentChild to early-exit on connectivity errors, which may or may not be detected before the "connection error" event is posted to the main thread.  Second, this patch adds RUNTIMEABORT() for "real errors", like invalid messages.  These are really really bad, and I'd like to smoke them out if they exist ASAP.

Comment 7

[Benjamin Smedberg]

Comment on attachment 460634 [details] [diff] [review]
Fix content processeses to (i) _exit(0) from ActorDestroy() in release builds; (ii) _exit(0) on send-on-closed-channel; and (iii) abort on other processing errors

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

> void
> ContentChild::ActorDestroy(ActorDestroyReason why)
> {
>     if (AbnormalShutdown == why)
>         NS_WARNING("shutting down because of crash!");
> 
>+#ifndef DEBUG
>+    QuickExit();
>+#endif

Hrm. I think we should quick-exit if AbnormalShutdown != NormalShutdown even in debug builds.

We're not going to do this for plugin children, or will that be a different patch?

r=me with that change

Comment 8

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

(In reply to comment #7)
> Comment on attachment 460634 [details] [diff] [review]
> Fix content processeses to (i) _exit(0) from ActorDestroy() in release builds;
> (ii) _exit(0) on send-on-closed-channel; and (iii) abort on other processing
> errors
> Hrm. I think we should quick-exit if AbnormalShutdown != NormalShutdown even in
> debug builds.
> 

Yeah, that makes sense.  Will fix.

> We're not going to do this for plugin children, or will that be a different
> patch?
> 

I don't think we should do that.  We don't know what persistent state plugins keep or how they synchronize it.  I kinda don't want to open that can of worms unless we have a reason to.  In fact, I'd almost be in favor of going in the other direction and ensuring that NP_Shutdown() is called regardless of how the plugin process shuts down.  But since this apparently isn't an issue in the field (yay stable firefox), I'm inclined also not to care.

Comment 9

[Shawn Wilsher :sdwilsh]

Is this going to land?  It's a beta4 blocker and we are freezing in two days.  Additionally, it's unclear why this needs to block beta 4 instead of just shipping in a beta.  Please clarify.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

No answer --> beta5+, so sad, too bad.

Comment 11

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

Was on PTO.  This wasn't a beta4 blocker, though I think it should be a fennecb1 blocker.

Comment 12

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

To be clear, I meant "I don't think this should have blocked beta4", not "this wasn't marked blocking:beta4+".

Comment 13

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

http://hg.mozilla.org/projects/cedar/rev/2b2f565d7c8f
http://hg.mozilla.org/projects/cedar/rev/12630a576e89