823474 - Add error handling after attempting to generate crash reporter

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Shelly Lin [:shelly]]

When system detects an abnormal shutdown, it attempts to generate a crash reporter here:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#643

Notice that we are assuming the |crashReporter| is always valid, however, if the child process is fail at launching and is dead already, the return value of |GenerateCrashReport| will be false, keep calling methods of this null instance result in a segmentation fault.

Propose: Add an error handling to |crashReporter| here, or maybe somewhere else too.

Comment 1

[Shelly Lin [:shelly]]

Attachment: draft 1: Add error handling if fail to generate crash reporter

Comment 2

[Shelly Lin [:shelly]]

Attachment: draft 1: Add error handling if fail to generate crash reporter

Compile error fixed.

Comment 3

[Shelly Lin [:shelly]]

Attachment: The call stack of signal SIGSEGV

This is the call stack when |ActorDestroy| is being called from ContentParent, and program received a signal SIGSEGV, result in a crash due to segmentation fault.

Comment 4

[Shelly Lin [:shelly]]

Attachment: Call stack of signal SIGSEGV

Comment 5

[Michael Vines [:m1] [:evilmachines]]

noming, as /may/ be a fix for bug 700594 and if it's not then this is yet another stability issue that we want fixed for v1.

Comment 6

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

Hi Shelly, how are you reproducing this bug?

Comment 7

[Shelly Lin [:shelly]]

Hi Chris, I found this bug while working on bug 811636, here's a short summary about bug 811636: Child process might die before IPC channel is connected properly, e.g. child process fails at execv() after doing a fork(), and currently we miss to handle such an extreme error case. 

The reproduce steps would be the following:
1. Apply the patch from bug 811363.
2. Launch b2g, and kill the Preallocated-app process.
3. Go to system/b2g, rename |plugin-container| to something else, e.g. |plugin-container.2|. 
4. Launch any web-app from Homescreen.

Result:
Program received SIGSEGV.

When a SIGCHLD signal is caught and if it's an abnormal exit from a child process, I call |ContentParent->OnChannelError()| to handle this kind of situation. |ActorDestroy()| is then being called from ContentParent, at this point, it tries to generate a crash reporter but fails at |crashReporter->GenerateCrashReport(this, NULL);|.

Comment 8

[Shelly Lin [:shelly]]

(In reply to Shelly Lin [:shelly] from comment #7) 
> The reproduce steps would be the following:
> 1. Apply the patch from bug 811363.

Typo, should be bug 811636.

Comment 9

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

Oh my, bug 811636 is very scary.  Is this something you've been seeing in the field?

Comment 10

[Shelly Lin [:shelly]]

I was reported "If you use up all the system memory, sometimes result in a blank Homescreen (Homescreen app fail to relaunch)". However, I didn't experience the exact fail in person because I never use the b2g aggressively, plus, we lack of such an error log about this kind of situation, at least not on logcat, make it harder to narrow down the cause.

Comment 11

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

Child processes are launched with the same OOM class as their launcher, and will only use less memory, so the odds of this causing bug 700594 are pretty slim.

Comment 12

[Andrew Overholt [:overholt]]

How are things going here, Shelly?  Do you need a hand?

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

since this no longer blocks 808607, is this really blocking-basecamp?

Comment 14

[Shelly Lin [:shelly]]

I've already attached a fix to this bug, if you think it's okay for v1, then I'll add some comments for the change, thanks.

Comment 15

[Andrew Overholt [:overholt]]

cjones can likely provide comments on the fix.

Comment 16

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

The patch here is aiming to fix the issue that caused bug 825976, but I don't think this patch is correct.

In the steps of comment 7, we wouldn't have finished initializing IPC so I'm not sure that we would reach ActorDestroy().  I would expect us to need something like bug 773925 to fix that problem.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

This won't block basecamp as we can ship without shutdown crashes in v1.

Comment 18

[BMO Automation]

Firefox OS is not being worked on