1239362 - nptest_gtk2 uses g_error to abort which doesn't trigger Breakpad

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P5)

Description

[(not currently active) Ted Mielczarek]

This showed up in bug 1239258.

Example:
https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/dom/plugins/test/testplugin/nptest_gtk2.cpp#264

The code there expects this to abort, and it does, but it doesn't wind up triggering Breakpad (because abort is called by way of glib?)

We have a custom glib logging function that does intercept and print a stack:
https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/toolkit/xre/nsSigHandlers.cpp#140

but since it's using NS_DEBUG_ASSERTION and assertions are not fatal in Mochitest we just get a stack.

I think there are two options here:
1) Make that glib logging function use NS_DEBUG_ABORT, which will crash us the right way. This might cause problems elsewhere if things outside our control are logging glib errors.
2) Just make nptest_gtk2 use MOZ_RUNTIMEABORT or MOZ_CRASH directly.

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> We have a custom glib logging function that does intercept and print a stack:
> https://dxr.mozilla.org/mozilla-central/rev/
> e790bba372f14241addda469a4bdb7ab00786ab3/toolkit/xre/nsSigHandlers.cpp#140
> 
> but since it's using NS_DEBUG_ASSERTION and assertions are not fatal in
> Mochitest we just get a stack.

That just intercepts the message printing functions.
g_error() triggers SIGTRAP after that call.

https://git.gnome.org/browse/glib/tree/glib/gmessages.c?h=2.32.1#n733
https://git.gnome.org/browse/glib/tree/glib/gbacktrace.h#n42

Could SIGTRAP trigger the crash handler?

(gdb) handle SIGTRAP
SIGTRAP is used by the debugger.
Are you sure you want to change it? (y or n) y
Signal        Stop      Print   Pass to program Description
SIGTRAP       Yes       Yes     No              Trace/breakpoint trap

> I think there are two options here:
> 1) Make that glib logging function use NS_DEBUG_ABORT, which will crash us
> the right way. This might cause problems elsewhere if things outside our
> control are logging glib errors.

g_error is always fatal, so we should catch these, but instead
catching SIGTRAP would catch the errors in the crash handler even when
XPCOM_DEBUG_BREAK is not set.  I think that would be helpful.

Comment 2

[(not currently active) Ted Mielczarek]

Handling SIGTRAP seems...odd. I'm not sure that that's actually something we want Breakpad to do.

Comment 3

[Karl Tomlinson (:karlt)]

FWIW the default action of SIGTRAP is to terminate and dump core, so ptrace-based programs setting breakpoints won't pass this through to the app.

However, aborting in my_glib_log_func() when G_LOG_FLAG_FATAL should be harmless, and I guess catches crashes on test infra.

Comment 4

[Karl Tomlinson (:karlt)]

This is fixed by Bug 1264367.

https://hg.mozilla.org/mozilla-central/diff/68fb465c2951/toolkit/crashreporter/google-breakpad/src/client/linux/handler/exception_handler.cc

https://github.com/google/breakpad/commit/a34c929f6f46c1c414147ecfa56c3562b2eca027

See https://bugzilla.mozilla.org/show_bug.cgi?id=1253154#c49