148453 - Mozilla no longer dumps core on SEGV signal!

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Aleksey Nogin]

I've noticed that Mozilla have recently stopped dumping the core when it
crashes; instead it just exits with exit status 11. I believe that this was
caused by the bug 76431 checkin - the SEGV signal gets intercepted by the
profile manager which then deletes the lock file and exits and core is never dumped!

This is pretty bad. It makes it significantly harder to investigate unexpected
(e.g. those that I didn't cause intentially when investigating a previous crash)
crashes.

Comment 1

[Aleksey Nogin]

This is a recent regression. It's also a dataloss - important stack trace
information gets lost instead of being saved in a core file.

Comment 2

[Bradley Baetz (:bbaetz)]

This will also presumably affect the stack-trace-on-crash hack which dbaron will
check in one of these days. The code chains signal handlers, so as long as we
run after talkback is initialied, talkback should still work. If the profile
code runs before, then the lock files probably aren't cleaned up - anyone with a
talkback build (or the comm source) want to test that?

Can we reset the sigaction to SIG_DFL, and then raise() from a signal handler?
Or should the final call be to abort(), not _exit ?

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

re-assign and cc some people from bug 76431

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

I wonder whether this affects talkback as well?

Comment 5

[Andrew Schultz]

this doesn't seem to affect talkback, but it prevents platforms without talkback
from getting stacktraces on unreproducible crashes.

Comment 6

[Aleksey Nogin]

Can bug 14989 check-ins help here?

Comment 7

[Aleksey Nogin]

Any progress on this? A recent Mozilla build (2002122113) crashed twice on me
over the last two days without me doing anything distinctive, and I have no idea
what's wrong. :-( It would be *really* nice to get the core dumps back!

Comment 8

[Aleksey Nogin]

Attachment: When old action was SIG_DFL, make sure that default handler is actually executed.

This patch ammends the current nsProfileAccess signal handler to check whether
the old handler was SIG_DFL and if so, then restore that SIG_DFL sigaction and
re-raise the signal to make sure the default action is executed.

This ensures that the core is actually dumped on SIGSEGV/SIGABRT/etc.

Comment 9

[Aleksey Nogin]

Comment on attachment 110048 [details] [diff] [review]
When old action was SIG_DFL, make sure that default handler is actually executed.

Please review. Thanks!

Comment 10

[Aleksey Nogin]

Comment on attachment 110048 [details] [diff] [review]
When old action was SIG_DFL, make sure that default handler is actually executed.

Oops, made a typo, fixing.

Comment 11

[Aleksey Nogin]

Attachment: When original action was SIG_DFL, make sure the default handler is still executed before we exit.

Fixed a small typo in the patch.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

does this maybe fix bug 176886?

Comment 13

[Aleksey Nogin]

I doubt it (although I do not really know how the talkback works). My code only
makes sure that the default handler is executed properly when there was no
non-default handler specified before the profile was locked. If talkback is
implemented as a signal handler, then profile locking should start with
non-default handler and my code shouldn't change anything...

But if something really strange is going with signal handling, it might help, I
do not know.

Comment 14

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 15

[Aleksey Nogin]

Attachment: When original action was SIG_DFL, make sure the default handler is still executed before we exit.

Updating for bitrot.

Comment 16

[Aleksey Nogin]

Comment on attachment 113656 [details] [diff] [review]
When original action was SIG_DFL, make sure the default handler is still executed before we exit.

It's a pretty simple fix, please review.

Comment 17

[Aleksey Nogin]

Comment on attachment 113656 [details] [diff] [review]
When original action was SIG_DFL, make sure the default handler is still executed before we exit.

Blizzard thinks dbaron should look at it.

Comment 18

[Daniel Veditz [:dveditz]]

removing obsolete mozilla1.0 keywords and the misleading "dataloss" (which
applies to losing user data not an inability to get debugging core dumps).
Accordingly bumping severity back down to normal, but it would be nice to get fixed.

Comment 19

[Samir Gehani]

adt: nsbeta1-

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 113656 [details] [diff] [review]
When original action was SIG_DFL, make sure the default handler is still executed before we exit.

>                 struct sigaction act, oldact;
>                 act.sa_handler = FatalSignalHandler;
>-                act.sa_flags = 0;
>+                act.sa_flags = SA_NOMASK;
>                 sigfillset(&act.sa_mask);

I'm surprised this does anything useful, given that |man sigaction| seems to
suggest that, with the full |sa_mask|, it won't, since "In addition [to
sa_mask], the signal which triggered the handler will be blocked, unless the
SA_NODEFER or SA_NOMASK flags are used."  But I'm guessing that one of them is
undone by the new |sigaction| call and the other isn't, or something like that?

But it looks like this code was moved in revision 1.80.  Could you attach a new
patch?	Sorry to take so long to get to this...

Comment 21

[Conrad Carlen (not reading bugmail)]

Comment on attachment 113656 [details] [diff] [review]
When original action was SIG_DFL, make sure the default handler is still executed before we exit.

>@@ -2131,7 +2135,7 @@
>                 // because mozilla is run via nohup.
>                 struct sigaction act, oldact;
>                 act.sa_handler = FatalSignalHandler;
>-                act.sa_flags = 0;
>+                act.sa_flags = SA_NOMASK;

I moved the patch to where this code is now. Problem is, SA_NOMASK doesn't
exist on OS X. Then probably not BSD either? If this is really needed, you need
to figure out the #ifdefs.

Comment 22

[Martins Krikis]

I disagree with comment #18---"dataloss" is not necessarily
misleading, it can be right on the point because there 
is _no_ autosave (see bug 16359 and bug 16360). 

My wife lost several hours of work yesterday and I failed 
to find any of it in kernel memory (not sure why, probably 
destroyed it while making a copy of it; I've been successful 
before with other programs crashing). A coredump, I believe,
would have allowed me to recover her text fairly easily.

My Mozilla 1.4 for GNU/Linux does not dump core regardless of 
whether the talkback agent is or is not in use. If people want
coredumps (which they show by raising the ulimit for it from
the normally default 0), then they should get coredumps. Please
fix this. It may be a weird way to recover data but it will be
something. Autosave hasn't happened for 3+ years already. This
should be easier to fix, I think.

Comment 23

[Brendan Eich [:brendan]]

There's no need to set blocking1.6a yet, the trunk is still open for checkins.

/be

Comment 24

[timeless]

Attachment: compiles on solaris and linux

Comment 25

[timeless]

Comment on attachment 133812 [details] [diff] [review]
compiles on solaris and linux

With the patch:
$ ./run-mozilla.sh ./xpcshell
js> Components.classes['@mozilla.org/generic-factory;1'].createInstance()
Segmentation Fault - core dumped
$ uname -a
SunOS DEATHSTARII 5.9 Generic_112233-08 sun4u sparc SUNW,Ultra-80

Yes I have a patch for generic factory too, but it's known broken today, so it
was convenient :)

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133812 [details] [diff] [review]
compiles on solaris and linux

Sure, sr=dbaron, but don't invent new bracing styles.

    ...
} else if (...)
{
    ...

isn't normal.  Prevailing style here suggests adding a break before the else
and a break before the { following the if.

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 133812 [details] [diff] [review]
compiles on solaris and linux

And how about space after , in argument lists (sigaction)?

/be

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 133812 [details] [diff] [review]
compiles on solaris and linux

I'm a little concerned about signal handling recursion should we ever crash in
the signal handler, due to NODEFER being used.	Could you instead just unblock
the signal before calling raise() by calling sigprocmask() ?

Comment 29

[Brian Ryner (not reading)]

Attachment: this is what i had in mind

unblock the signal in question just before invoking the default handler

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 134344 [details] [diff] [review]
this is what i had in mind

>+        if (oldact->sa_handler == SIG_DFL) {
>+            // Make sure the default sig handler is executed

period at end of comment sentence.

>+            // We need it to get Mozilla to dump core.
>+            sigaction(signo,oldact,NULL);
>+
>+            // Now that we've restored the default handler, unmask the
>+            // signal and invoke it.
>+

Why the extra blank line here, but not previously.  I'd say axe this one, but
in any event be consistent.

>+            sigset_t unblock_sigs;
>+            sigemptyset(&unblock_sigs);
>+            sigaddset(&unblock_sigs, signo);
>+
>+            sigprocmask(SIG_UNBLOCK, &unblock_sigs, NULL);
>+
>+            raise(signo);
>+        } else if (oldact->sa_handler && oldact->sa_handler != SIG_IGN)
>+        {
>+            oldact->sa_handler(signo);
>+        }
>     }
> 
>     // Backstop exit call, just in case.

Sorry for the drive-by nit-picking, it looks good to me otherwise.  Timeless,
darin?

/be

Comment 31

[Darin Fisher]

Comment on attachment 134344 [details] [diff] [review]
this is what i had in mind

this patch makes good sense to me.

to be nit picky, the brace style is not consistent near the "else" and there
should be spaces in the parameter list of sigaction ;-)

r/sr=darin fwiw

Comment 32

[Brian Ryner (not reading)]

Comment on attachment 133812 [details] [diff] [review]
compiles on solaris and linux

Minusing, I prefer the patch I attached that doesn't use SA_NODEFER...

Comment 33

[Brian Ryner (not reading)]

Comment on attachment 134344 [details] [diff] [review]
this is what i had in mind

Brendan, what do you think?

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 134344 [details] [diff] [review]
this is what i had in mind

Fix the nit mentioned in comment 27 and r/sr=me.

/be

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134344 [details] [diff] [review]
this is what i had in mind

>+        } else if (oldact->sa_handler && oldact->sa_handler != SIG_IGN)
>+        {

Put the |else| on a new line after the }, make sure someone's tested this, and
sr=dbaron.

Comment 36

[Brian Ryner (not reading)]

checked in.

Comment 37

[Mike Kaply [:mkaply]]

Brian,

Can you check the correct version of this patch into 1.4.2 and mark fixed1.4.2
please?