931861 - xpcshell installs breakpad signal handlers after installing AsmJS/Ion signal handlers

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Stacktraces

Bug 929374 will enable Ion and TI for chrome scripts but the patch there is causing xpcshell test failures on Linux [0].

The problem is that we're installing the AsmJS/Ion signal handler before we install the breakpad handler. Bug 842728 attempted to fix this but apparently the problem is still there.

NS_InitXPCOM2 installs the AsmJS signal handlers, then crashReporter->SetEnabled(true) installs the breakpad signal handlers:

        nsCOMPtr<nsIServiceManager> servMan;
        rv = NS_InitXPCOM2(getter_AddRefs(servMan), appDir, &dirprovider);
        ... snip ...

#ifdef MOZ_CRASHREPORTER
        const char *val = getenv("MOZ_CRASHREPORTER");
        crashReporter = do_GetService("@mozilla.org/toolkit/crash-reporter;1");
        if (val && *val) {
            crashReporter->SetEnabled(true);
        }
#endif

I'm attaching two stack traces, one for EnsureAsmJSSignalHandlersInstalled and one for google_breakpad::ExceptionHandler::InstallHandlersLocked.

[0] https://tbpl.mozilla.org/?tree=Try&rev=6de5964df384

Comment 1

[(not currently active) Ted Mielczarek]

Ugh. If those signal handlers get installed at XPCOM Init then fixing this is more of a PITA. We'll have to add a new XRE API to init crash reporting, and call it from xpcshell prior to init'ing XPCOM.

Comment 2

[Luke Wagner [:luke]]

Yes, signal handlers are now installed when the JSRuntime is first created which I think happens about the time that we start loading JS components during initialization.

Is the crash reporter service C++ and does it get initialized before the JS components?  If so, then it seems like we could install handlers then.

Comment 3

[(not currently active) Ted Mielczarek]

The actual crash reporter stuff is not XPCOM. The nsICrashReporter interface is hung off of the nsXULAppInfo singleton:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#578

During browser startup the crash reporter gets initialized very early:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2991

We can't call those APIs directly because they're not exported from libxul. What I suggested in comment 1 was basically to export an API that can call  CrashReporter::SetExceptionHandler, then we can call that from xpcshell.

Comment 4

[Benjamin Smedberg]

What do the AsmJS signal handlers actually do? Note that if we have ordering requirements, we should probably remove the nsICrashReporter.enabled API since any uninstall/reinstall of the signal handlers would break the system.

Comment 5

[(not currently active) Ted Mielczarek]

bug 842728 made .enabled read-only, and added a noscript SetEnabled method, which was enough to fix this the first time. Apparently AsmJS signal handlers are now init'ed during XPCOM init though, which means we can't do it that way anymore.

Comment 6

[Luke Wagner [:luke]]

(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #4)
The handlers are used by both asm.js and non-asm.js JS to avoid dynamic checks in a few hot paths where we can safely guarantee a SIGSEGV.  The handlers do fixup and then allow execution to resume.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

We'd like to enable Ion/TI/asm.js etc everywhere. This patch fixes the xpcshell issue; it turned out to be fairly straight-forward. As you suggested, we can call CrashReporter::SetExceptionHandler directly from xpcshell.

This happens before initializing XPCOM (the whole point of this patch), so we set the GRE dir from the current working directory if it's not passed explicitly (we can't use XPCOM to get it).

With this patch xpcshell tests are green on all platforms with Ion/TI chrome prefs flipped.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8373692 [details] [diff] [review]
Patch

Review of attachment 8373692 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this! It definitely got a lot more straightforward since bug 920695 was fixed.

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +1473,1 @@
>          crashReporter = do_GetService("@mozilla.org/toolkit/crash-reporter;1");

You could just kill this variable entirely and replace the crashReporter->SetEnabled(false); down below with CrashReporter::UnsetExceptionHandler();

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae890f65ae9

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> You could just kill this variable entirely and replace the
> crashReporter->SetEnabled(false); down below with
> CrashReporter::UnsetExceptionHandler();

Done.

Comment 10

[[e-mail]]

(In reply to Ted Mielczarek from comment #8)
> You could just kill this variable entirely and replace the
> crashReporter->SetEnabled(false); down below with
> CrashReporter::UnsetExceptionHandler();

This looks to be the last remaining caller of SetEnabled (just pushed a Try build as a quick check, as I --disable-crashreporter locally) so assuming that's the case I'll file a followup bug to remove it.

Comment 11

[(not currently active) Ted Mielczarek]

That makes sense, since I added it to support the xpcshell use case. Thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ae890f65ae9

Comment 13

[[e-mail]]

(In reply to comment #10)
> (In reply to Ted Mielczarek from comment #8)
> > You could just kill this variable entirely and replace the
> > crashReporter->SetEnabled(false); down below with
> > CrashReporter::UnsetExceptionHandler();
> 
> This looks to be the last remaining caller of SetEnabled (just pushed a Try
> build as a quick check, as I --disable-crashreporter locally) so assuming
> that's the case I'll file a followup bug to remove it.

Looks like gtests are still using it:
http://dxr.mozilla.org/mozilla-central/source/testing/gtest/mozilla/GTestRunner.cpp#105

Comment 14

[[e-mail]]

(In reply to comment #13)
> (In reply to comment #10)
> > (In reply to Ted Mielczarek from comment #8)
> > > You could just kill this variable entirely and replace the
> > > crashReporter->SetEnabled(false); down below with
> > > CrashReporter::UnsetExceptionHandler();
> > 
> > This looks to be the last remaining caller of SetEnabled (just pushed a Try
> > build as a quick check, as I --disable-crashreporter locally) so assuming
> > that's the case I'll file a followup bug to remove it.
> 
> Looks like gtests are still using it:
> http://dxr.mozilla.org/mozilla-central/source/testing/gtest/mozilla/
> GTestRunner.cpp#105

Also media and places tests, but I don't know how to get them to link.
http://dxr.mozilla.org/mozilla-central/source/media/mtransport/test/mtransport_test_utils.h#72
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/cpp/places_test_harness_tail.h#117