548810 - crashes [@ CrashReporter::OnChildProcessDumpRequested ] during main process shutdown

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Benjamin Smedberg]

There are a few crashes in the browser process on shutdown at CrashReporter::OnChildProcessDumpRequested. 

http://crash-stats.mozilla.com/report/index/d734961d-c574-4ac1-a483-86cd72100225

http://hg.mozilla.org/mozilla-central/annotate/059e9961a122/toolkit/crashreporter/nsExceptionHandler.cpp#l1233

I think we're just after the component manager shutdown here and so createinstance is always going to fail. We should either unhook the child-process crash reporting before entering full shutdown or sanity-check this code more carefully: I'm not sure why we chose to use XPCOM/nsIOutputStream rather than OpenNSPRFileDesc/straight NSPR calls, but that's also a possible workaround.

Also, shouldn't we have killed off all the plugin processes by the time we get here? Main thread is at

http://hg.mozilla.org/mozilla-central/annotate/059e9961a122/xpcom/build/nsXPComInit.cpp#l892

Comment 1

[(not currently active) Ted Mielczarek]

I think I just used XPCOM because it seemed easier at the time. Doesn't really matter.

Comment 2

[Benedict Hsieh [:benedict]]

Attachment: NSPR writes instead of XPCOM

Just uses PR_Write instead of XPCOM streams. I am not sure how to test this patch yet, suggestions would be very helpful.

Other questions: are there reasons to prefer unhooking crash reporting before entering full shutdown over this approach? Is the ordering of killing off processes (killing plugin processes before we get to main thread shutdown) important other than this bug?

Comment 3

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

(In reply to comment #2)
> Other questions: are there reasons to prefer unhooking crash reporting before
> entering full shutdown over this approach? Is the ordering of killing off
> processes (killing plugin processes before we get to main thread shutdown)
> important other than this bug?

There are three issues here

 (1) Whether to collect minidumps of (plugin) subprocesses that crash around shutdown time
 (2) Whether XPCOM or the IO thread should live longer

I'm not sure about (1).  TBH, I don't know what would happen if we grabbed a minidump of a plugin subprocess after its nsNPAPIPlugin/PluginModuleParent were destroyed.  If we can do something useful with those, then collecting them seems like a worthy goal.  ted/bsmedberg thoughts?

If we don't care about (1), then we should probably destroy the crash server (but not in-process crashreporter; their lifetimes are currently the same) as early as possible during XPCOM shutdown, and avoid issue (2).  In our current setup, having the IO thread outlive XPCOM is very convenient, and I'd prefer not to change that unless we really must.

Comment 4

[Benedict Hsieh [:benedict]]

Seems like just doing PR_Writes instead of XPCOM streams is the right solution for both of those things then, right? (Assuming we do want minidumps and the IO thread should live longer.)

I went ahead and requested review, since this seems like a pretty simple fix. I can revisit writing tests for this if we have this issue again, or if we come up with some less painful ways of doing the test.

Comment 5

[Benedict Hsieh [:benedict]]

Attachment: NSPR writes instead of XPCOM

Sorry, I thought I had uploaded this version already.

Comment 6

[Benedict Hsieh [:benedict]]

Attachment: NSPR writes instead of XPCOM

hg is kicking my butt today

Comment 7

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

> (In reply to comment #3)
> There are three issues here
> 

Whoops sorry, one issue ending up reducing to (2).

(In reply to comment #4)
> Seems like just doing PR_Writes instead of XPCOM streams is the right solution
> for both of those things then, right? (Assuming we do want minidumps and the IO
> thread should live longer.)
> 

If we don't care about collecting minidumps from subprocesses around shutdown, we could just unhook OOP crashreporter early in XPCOM shutdown and wouldn't need this patch.  But IMHO it's a good patch to take regardless of what we do wrt to crashreporter/XPCOM shutdown ordering.

Comment 8

[Benjamin Smedberg]

This patch doesn't compile, the change from nsIFile -> nsILocalFile broke the Clone() call.

Comment 9

[Benedict Hsieh [:benedict]]

Attachment: fixes build problem

ergh, didn't have the right flag set in my mozconfig while compiling that last one.

Comment 10

[[e-mail]]

Comment on attachment 431966 [details] [diff] [review]
fixes build problem

http://hg.mozilla.org/mozilla-central/rev/a2de54c0c675

Comment 11

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/5912e2df32c3

Comment 12

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

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 13

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430