Closed Bug 548810 Opened 14 years ago Closed 14 years ago

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

Categories

(Toolkit :: Crash Reporting, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: benjamin, Assigned: benedict)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 file, 3 obsolete files)

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
I think I just used XPCOM because it seemed easier at the time. Doesn't really matter.
Assignee: nobody → bhsieh
Attached patch NSPR writes instead of XPCOM (obsolete) — Splinter Review
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?
(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.
Attachment #430161 - Flags: review?(ted.mielczarek)
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.
Attached patch NSPR writes instead of XPCOM (obsolete) — Splinter Review
Sorry, I thought I had uploaded this version already.
Attachment #430161 - Attachment is obsolete: true
Attachment #431473 - Flags: review?
Attachment #430161 - Flags: review?(ted.mielczarek)
Attachment #430161 - Flags: review?(ted.mielczarek)
Attached patch NSPR writes instead of XPCOM (obsolete) — Splinter Review
hg is kicking my butt today
Attachment #431473 - Attachment is obsolete: true
Attachment #431473 - Flags: review?
Attachment #431476 - Flags: review?(ted.mielczarek)
> (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.
Attachment #430161 - Flags: review?(ted.mielczarek)
Attachment #431476 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
This patch doesn't compile, the change from nsIFile -> nsILocalFile broke the Clone() call.
Keywords: checkin-needed
ergh, didn't have the right flag set in my mozconfig while compiling that last one.
Attachment #431476 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: