Closed Bug 931861 Opened 11 years ago Closed 10 years ago

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

Categories

(Toolkit :: Crash Reporting, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

Attached file 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
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.
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.
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.
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.
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.
(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.
Attached patch PatchSplinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8373692 - Flags: review?(ted)
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();
Attachment #8373692 - Flags: review?(ted) → review+
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.
(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.
That makes sense, since I added it to support the xpcshell use case. Thanks!
https://hg.mozilla.org/mozilla-central/rev/8ae890f65ae9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(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
(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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: