842728 - initialize breakpad signal handlers early in startup in xpcshell

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Luke Wagner [:luke]]

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.

Comment 4

[(not currently active) Ted Mielczarek]

I've got this mostly done, just need to fix up the test harness. Patch up soon.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: 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 6

[Bobby Holley (:bholley)]

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 7

[Gregory Szorc [:gps]]

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.

Comment 8

[(not currently active) Ted Mielczarek]

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/48ee2152dcbd

Comment 10

[Mike Kaply [:mkaply]]

So I realize that this happened a long time ago, but I've just now realized that this removed the ability to disable the crash reporter via JavaScript.

This doesn't appear to have been documented anywhere, especially not here:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICrashReporter

Enterprises need the ability to disable the crash reporter...

I am aware of the ability to use the registry, but that doesn't work on Mac.

Is there some other documented way to disable the crash reporter?

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[Mike Kaply [:mkaply]]

(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?

Comment 13

[(not currently active) Ted Mielczarek]

I believe the registry value is just for the default value of the "Submit report" checkbox.