811636 - Parent process does not handle the situation correctly if its child process is failed at launching

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shelly Lin [:shelly]]

Parent process is fail to get any notification if its child process does not launch or execute itself properly.

Comment 1

[Dave Hylands [:dhylands]]

Launching a child process involves a fork/exec

There are at least 4 very distinct time periods that we need to concern ourselves with:

1 - Failures which occur in the parent before the fork
2 - Failures which occur in the child after the fork but before the exec
3 - Failures which occur in the child after the exec but before the IPC channel is setup
4 - Failures which occur in the child after the IPC channel is setup

Comment 2

[Shelly Lin [:shelly]]

Attachment: draft 1: Handle when a child process exit abnormally

This is happened when a child process is forked successfully, but failed after execv, and call _exit(127).

The current system misses the situation when a child process exit itself abnormally.

This draft of patch is trying to watch the SIGCHLD in an earlier state of creating a child process. Modify the fetched result from DidProcessCrash() (in process_util_posix.cc) in order to defer a child process is exit normally or not.

And after receives an abnormal exit of a child process, sends a "oop-frameloader-crashed' to window_manager.js. Or, maybe we should do something else?

Comment 3

[Shelly Lin [:shelly]]

Please reference comment 7 from bug 823474 for details of reproduce steps.
https://bugzilla.mozilla.org/show_bug.cgi?id=823474#c7

Comment 4

[Justin Lebar (not reading bugmail)]

Comment on attachment 684599 [details] [diff] [review]
draft 1: Handle when a child process exit abnormally

I think cjones is probably a better person to look at this; I'm not familiar with any of this code.

Chris, if you're swamped, I can figure this stuff out.

Comment 5

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

Comment on attachment 684599 [details] [diff] [review]
draft 1: Handle when a child process exit abnormally

Hi Shelly, this is a very good UNIX-y solution to this problem.  I
like it a lot :).

The problem is, this process mangement code is cross-platform, and we
need to keep all of them in mind when designing features like this.

We have an existing mechanism to notify us of "channel errors" (which
during startup, correspond to process crashes/exits).  The first place
this code is used is when we launch the subprocess.  The subprocess is
launched from ipc/glue/GeckoChildProcessHost.cpp.  If an error occurs,
it will be notified here [1].  The code doesn't properly handle the
error notification at the moment; fixing that is bug 773925.

The second piece we need is to let ContentParent know when a channel
error has occurred during launch.  We can set a particular state on
the GeckoChildProcessHost, for example.

The advantage of this alternate approach is that it will work across
all the platforms we support.

Let me know if this makes sense.

[1] http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#827

Comment 6

[Shelly Lin [:shelly]]

Attachment: draft 2: Close the client fd earlier

Comment 7

[Shelly Lin [:shelly]]

Comment on attachment 703172 [details] [diff] [review]
draft 2: Close the client fd earlier

Hi Chris,

Thanks for your feedback! I've worked on another solution to handle this kind of error, with less changes and closer to the outlook of our exist mechanism.

When a child process is created, its parent process waits for a hello message and close the client_pipe_. However, in this kind of situation, a system interrupt with signal SIGCHLD is sent, interrupts epoll_dispatch, and thus the hello message is never added to active event list, making the libevent fail to receive any notification. I figured maybe we can close the client_pipe_ eariler in time, in order to find out the connection has been reset already.

This change uses the exist mechanism to notify us channel error, but the rest is like you mentioned, where OnChannelError from GeckoChildProcessHost is not yet being called at this moment.

Comment 8

[Shelly Lin [:shelly]]

Btw, bug 823474 is no longer needed with this version of fix.

Comment 9

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

I'm very sorry for the long feedback lag here :(.  I'll be clearing out my queue very soon.

We need this work for 100% rock-solid stability.  When this kind of lmk death occurs, we permanently lose a substantial amount of device memory.  That makes the possibility of this crash happening again higher, etc, creating a disastrous feedback loop.

Comment 10

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

Comment on attachment 703172 [details] [diff] [review]
draft 2: Close the client fd earlier

Hi Shelly, is it correct that we're not getting the error notification at [1] currently?  If so, I /think/ I understand what this patch is doing, but there's still a race condition where the process can die without the notification.

We should try to discuss this on IRC this week.  (I want to make sure I understand the details here ;).)

[1] http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#833

Comment 11

[Dietrich Ayala (:dietrich)]

How often does this occur? How can this be triggered? Is it showing up in any of our or our partners' stability tests?

Comment 13

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

Needed for competitive stability.

Comment 14

[Shelly Lin [:shelly]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> Comment on attachment 703172 [details] [diff] [review]
> draft 2: Close the client fd earlier
> 
> Hi Shelly, is it correct that we're not getting the error notification at
> [1] currently?  If so, I /think/ I understand what this patch is doing, but
> there's still a race condition where the process can die without the
> notification.
> 
> We should try to discuss this on IRC this week.  (I want to make sure I
> understand the details here ;).)
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.
> cpp#833

Hi Chris,

Thanks for your feeback, and yes, we're not getting the error notification at [1], I'm not 100 percent sure if it is not called other where else, but at least it's not being called when the app is killed (shutdown abnormally).

Please feel free to ping me on IRC, my nickname is ShellyLin. :)

Comment 15

[Shelly Lin [:shelly]]

Attachment: Handle the error of execv() when launching child processes

Hi Justin,

Without this fix, we will end up having lots of zombie processes in the system. Since project Nuwa (bug 771765) might refactor most of the ipc implementation, especially the part of creating child processes, I'm not sure this is needed anymore, however, this fix is small and not risky, I still think it is worth trying.

I know you have re-directed this issue to cjones, or could you re-direct to someone else? Thanks.

Comment 16

[Justin Lebar (not reading bugmail)]

We should definitely still fix this even though we're trying to do Nuwa.

Did you and cjones ever figure out the race condition he mentioned in comment 10?

Comment 17

[Dave Hylands [:dhylands]]

We need to cover the child dying before it gets the IPC channel opened as well as after it gets the IPC channel opened.

Normally, the parent would launch a thread which does wait calls (a reaper thread). This reaps the zombies and allows the parent to be notified regardless when the child dies.

Comment 18

[Dave Hylands [:dhylands]]

Even under Nuwa, we need the same thing, but it needs to be the template process that does the reaping (you can only reap children, not grand children).

Comment 19

[Shelly Lin [:shelly]]

(In reply to Justin Lebar [:jlebar] from comment #16)
> We should definitely still fix this even though we're trying to do Nuwa.
> 
> Did you and cjones ever figure out the race condition he mentioned in
> comment 10?

I didn't get a chance to talk with cjones, but I've discussed this issue with Thinker. Race condition won't happen here, since we are closing the client's fd and the call to |base::LaunchApp()| is synchronized, from the part of fork(), execv() till return.

Comment 20

[Shelly Lin [:shelly]]

(In reply to Dave Hylands [:dhylands] from comment #17)
> We need to cover the child dying before it gets the IPC channel opened as
> well as after it gets the IPC channel opened.
> 
> Normally, the parent would launch a thread which does wait calls (a reaper
> thread). This reaps the zombies and allows the parent to be notified
> regardless when the child dies.

Yes, normally it does. In this abnormal case, child process dies before our ipc mechanism receives the "hello message", that is, the parent won't be notified because this child process does not event "exist".

Comment 21

[Shelly Lin [:shelly]]

The hacky steps to produce a zombie process:

1. Launch b2g, and kill the Preallocated-app process.
2. Go to system/b2g, rename |plugin-container| to something else, e.g. |plugin-container.2|. 
3. Launch any web-app from Homescreen.
4. Observe a Z process with name "Gecko_IOThread".

Comment 22

[Dave Hylands [:dhylands]]

(In reply to Shelly Lin [:shelly] from comment #20)
> (In reply to Dave Hylands [:dhylands] from comment #17)
> > We need to cover the child dying before it gets the IPC channel opened as
> > well as after it gets the IPC channel opened.
> > 
> > Normally, the parent would launch a thread which does wait calls (a reaper
> > thread). This reaps the zombies and allows the parent to be notified
> > regardless when the child dies.
> 
> Yes, normally it does. In this abnormal case, child process dies before our
> ipc mechanism receives the "hello message", that is, the parent won't be
> notified because this child process does not event "exist".

Well the child does, in fact, exist, it just didn't get around to using the ipc mechanism.

My point is that we shouldn't be using the IPC mechanism to track the child processes.

If the fork succeeds, then we should start tracking the child and be issuing a wait for the child. Otherwise, any child that fails for any reason between the fork and the open of the ipc channel becomes a zombie (because the parent needs to reap it so that the zombie goes away).

Comment 23

[Justin Lebar (not reading bugmail)]

It sounds like dhylands understands this better than I do, and it also looks like the patch here doesn't fix the problem dhylands identified in comment 22.

Shelly, are you willing to wait a few weeks for Dave to get back from vacation and figure this out with him?

Comment 24

[Justin Lebar (not reading bugmail)]

Comment on attachment 761912 [details] [diff] [review]
Handle the error of execv() when launching child processes

Kicking this over to Dave.  If you need this taken care of more quickly, let me know and we can figure that out.

Comment 25

[Andrew McCreight [:mccr8]]

This problem sounds similar to this issue that was fixed upstream:
  https://codereview.chromium.org/7870008

"The problem was that when the child process died before connecting to the parent's IPC channel, the parent wouldn't notice the crash because the child end of the IPC pipe was kept open for too long."

Comment 26

[Shelly Lin [:shelly]]

(In reply to Dave Hylands [:dhylands] (PTO back July 2) from comment #22)
> (In reply to Shelly Lin [:shelly] from comment #20)
> > (In reply to Dave Hylands [:dhylands] from comment #17)
> > > We need to cover the child dying before it gets the IPC channel opened as
> > > well as after it gets the IPC channel opened.
> > > 
> > > Normally, the parent would launch a thread which does wait calls (a reaper
> > > thread). This reaps the zombies and allows the parent to be notified
> > > regardless when the child dies.
> > 
> > Yes, normally it does. In this abnormal case, child process dies before our
> > ipc mechanism receives the "hello message", that is, the parent won't be
> > notified because this child process does not event "exist".
> 
> Well the child does, in fact, exist, it just didn't get around to using the
> ipc mechanism.
> 
> My point is that we shouldn't be using the IPC mechanism to track the child
> processes.
> 
> If the fork succeeds, then we should start tracking the child and be issuing
> a wait for the child. Otherwise, any child that fails for any reason between
> the fork and the open of the ipc channel becomes a zombie (because the
> parent needs to reap it so that the zombie goes away).

I see. "Issuing a wait" happens to be my very first solution, but Chris (in comment #5) thinks it's better to use our IPC mechanism to report errors. Attaching that patch back for further discussion.

Comment 27

[Shelly Lin [:shelly]]

Attachment: Issuing a wait for child process

Issuing a wait on newly created child process, in order to monitor its life state.

Comment 28

[Shelly Lin [:shelly]]

Justin, sounds good to me, thanks.

Andrew, it does look exactly the same like this issue!
(https://codereview.chromium.org/7870008)

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 761912 [details] [diff] [review]
Handle the error of execv() when launching child processes

Review of attachment 761912 [details] [diff] [review]:
-----------------------------------------------------------------

OK - I spent some time looking at the child/parent IPC socket and I things are a bit clearer.

What actually happens is something along these lines:

1 - parent creates a socketpair
2 - parent fork/execs the child, which inherits the open file descriptors

So I now believe that this patch is doing the right thing (and this should cover all of the failure points).

Comment 30

[Shelly Lin [:shelly]]

Attachment: Close the FD of client side after parent fork/execs the child

Thanks for your time very much!

I've added and updated some comments, could you give it a reivew?

Comment 31

[Dave Hylands [:dhylands]]

Comment on attachment 771146 [details] [diff] [review]
Close the FD of client side after parent fork/execs the child

Review of attachment 771146 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ +781,5 @@
>  
> +void Channel::ChannelImpl::CloseClientFileDescriptor() const {
> +  if (client_pipe_ != -1) {
> +    Singleton<PipeMap>()->Remove(pipe_name_);
> +    HANDLE_EINTR(close(client_pipe_));

This function should be setting client_pipe_ to -1 after closing the file descriptor.

This then implies that this can't be a const method (so you'll need to remove the const or declare client_pipe_ mutable.

@@ +951,5 @@
>    return channel_impl_->GetServerFileDescriptor();
>  }
>  
> +void Channel::CloseClientFileDescriptor() const {
> +  return channel_impl_->CloseClientFileDescriptor();

nit: I'd remove the return since this is a void function.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +656,5 @@
>  #endif
>                    false, &process, arch);
>  
> +  // Close the FD of client side as early as possible, letting our ipc mechanism
> +  // to detect unexpected error at any stage.

nit: I'd reword this slightly.

// We're in the parent and the child was launched. Close the child FD
// in the parent as soon as possible, which will allow the parent
// to detect when the child closes its FD (either due to normal exit or
// due to crash).

Comment 32

[Shelly Lin [:shelly]]

Attachment: Close the FD of client side after parent fork/execs the child (w/ nit fix)

Patch updated with nit fixes, and thanks for your review :)

Comment 33

[Shelly Lin [:shelly]]

Attachment: Close the FD of client side after parent fork/execs the child (w/ nit fix)

Fixed compiling error.

Comment 34

[Dave Hylands [:dhylands]]

Comment on attachment 771949 [details] [diff] [review]
Close the FD of client side after parent fork/execs the child (w/ nit fix)

Review of attachment 771949 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +658,5 @@
>  
> +  // We're in the parent and the child was launched. Close the child FD in the
> +  // parent as soon as possible, which will allow the parent to detect when the
> +  // child closes its FD (either due to normal exit or due to crash).
> +  const_cast<IPC::Channel&>(channel()).CloseClientFileDescriptor();

nit: You should be able to do it without the cast by doing:

GetChannel()->CloseClientFileDescriptor();

GetChannel is declared here:
https://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.h#83

Comment 35

[Shelly Lin [:shelly]]

Attachment: [check-in] Fixed patch, w/ nit fixed, and carry r+

Patch to checkin, with nit fixed, and carry r+ from :dhylands.

Comment 36

[Shelly Lin [:shelly]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=4bd1b967abfd

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/68fa5bec3312

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68fa5bec3312

Comment 39

[Andrew McCreight [:mccr8]]

Marking leo? per bug 893012#c54.

Comment 40

[Justin Lebar (not reading bugmail)]

Blocks a blocker, and already has testing done by Leo.

Comment 41

[Andre Graziani (:graziani)]

As I said in bug 893012, comment 54, the accumulated messages in output queue is solved by this patch.
But I observed just now, that a preallocated process is leaked after this patch.
I am checking how it happened, and if it is really related to this patch.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a9f1d82103c6

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a9f1d82103c6

Comment 44

[Andre Graziani (:graziani)]

Ok, the extra preallocated process in comment 41 is a known issue (bug 867739) and not related to this patch.

Comment 45

[Shelly Lin [:shelly]]

(In reply to andre.graziani from comment #44)
> Ok, the extra preallocated process in comment 41 is a known issue (bug
> 867739) and not related to this patch.

Thanks for your confirm. :)