The browser sets the breakpad signal handlers early in startup. This is good because the breakpad handlers don't chain so, if anyone else wants to add a handler, they need to install their handlers after breakpad (and chain, of course). It would be useful if xpcshell did the same thing as the browser since the crashreporter tests are xpcshell tests. This is especially important to verify that new signal handlers integrate properly with the crash reporter (e.g. bug 840280). It would also by great if we could totally remove the dynamic support for lazily setting breakpad signal handlers since almost any use of this feature would be a bug.
This should be mostly just a matter of porting the code from XRE_Main: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3003 to xpcshell's main. I'm not sure why I didn't implement this this way in the first place. I believe the only users of the run-time enabling are test harnesses that could simply use the environment variable anyway.
Oh, I think I know why I didn't implement it that way. xpcshell builds with external linkage, so it can't call directly into the CrashReporter:: stuff. I think the way to fix this would be to rework the nsICrashReporter interface, to remove the .enabled property (or make it read-only) and add a non-scriptable Enable method that we could call during startup in xpcshell.
We're close to landing another JS use of signal handlers which we'd like to ensure doesn't break the crash reporter (bug 864220). It'd be really good to fix this soon so we could add unit tests. The regression we're trying to protect against is failing to call breakpad and thus losing crash reports. On a side note: have any designs been considered that do crash-handling via some process-external mechanism that cannot be corrupted by the process itself? At the very least, it seems like Windows can do this with Dr. Watson and OSX can do this since mach tasks can listen to other tasks' exception ports.
I've got this mostly done, just need to fix up the test harness. Patch up soon.
Created attachment 791356 [details] [diff] [review] init Breakpad from C++ in xpcshell, remove ability to init from script This changes the nsICrashReporter interface to make the enabled property readonly, and adds a separate noscript SetEnabled method. I changed xpcshell to respect the MOZ_CRASHREPORTER environment variable and enable crash reporting early in startup if it's set, and then changed the xpcshell test harness to set that instead of enabling it from script. I had to tweak the crashreporter tests to match as well (there were a set of tests for enabling/disabling that were mostly commented out, so I just removed them). r?gps for the test harness bits, r?bholley for the xpcshell bits.
Comment on attachment 791356 [details] [diff] [review] init Breakpad from C++ in xpcshell, remove ability to init from script Review of attachment 791356 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley ::: xpcom/system/nsICrashReporter.idl @@ +26,5 @@ > + * Enable or disable crash reporting at runtime. Not available to script > + * because the JS engine relies on proper exception handler chaining. > + */ > + [noscript] > + void SetEnabled(in bool enabled); Please lowercase the S.
Comment on attachment 791356 [details] [diff] [review] init Breakpad from C++ in xpcshell, remove ability to init from script Review of attachment 791356 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: toolkit/crashreporter/test/unit/test_crashreporter.js @@ -14,5 @@ > - * Check behavior when disabled. > - * XXX: don't run these tests right now, there's a race condition in > - * Breakpad with the handler thread. See: > - * http://code.google.com/p/google-breakpad/issues/detail?id=334 > - ********** This is the least obvious non-ending comment block I've ever seen.
On the plus side, I did eventually fix the race condition mentioned there, I just never re-enabled those tests. https://hg.mozilla.org/integration/mozilla-inbound/rev/48ee2152dcbd
mkaply: it respects the environment variable MOZ_CRASHREPORTER_DISABLE=1 on startup. There's no scriptable way to disable crash reporting now, and I don't think we'd add one back.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11) > mkaply: it respects the environment variable MOZ_CRASHREPORTER_DISABLE=1 on > startup. There's no scriptable way to disable crash reporting now, and I > don't think we'd add one back. That works great. I guess it will have to do. There used to be a way to do it via the Windows registry. Is that still valid?
I believe the registry value is just for the default value of the "Submit report" checkbox.