initialize breakpad signal handlers early in startup in xpcshell

RESOLVED FIXED in mozilla26

Status

()

Toolkit
Breakpad Integration
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: luke, Assigned: ted)

Tracking

({dev-doc-needed})

unspecified
mozilla26
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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.
(Reporter)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
I've got this mostly done, just need to fix up the test harness. Patch up soon.
Assignee: nobody → ted
(Assignee)

Comment 5

4 years ago
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.
Attachment #791356 - Flags: review?(gps)
Attachment #791356 - Flags: review?(bobbyholley+bmo)
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.
Attachment #791356 - Flags: review?(bobbyholley+bmo) → review+

Comment 7

4 years ago
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.
Attachment #791356 - Flags: review?(gps) → review+
(Assignee)

Comment 8

4 years ago
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

4 years ago
https://hg.mozilla.org/mozilla-central/rev/48ee2152dcbd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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?
Keywords: dev-doc-needed
(Assignee)

Comment 11

a year ago
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?
(Assignee)

Comment 13

a year ago
I believe the registry value is just for the default value of the "Submit report" checkbox.
You need to log in before you can comment on or make changes to this bug.