411615 - ah_crap_handler should call _exit explicitly

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Matthew Gregan [:kinetik]]

The ah_crap_handler signal handler is a handler for fatal signals and should call _exit explicitly rather than relying on some chained signal handler calling _exit for it.

Currently we exit after handling a fatal signal because nsProfileLock::FatalSignalHandler calls exit after calling chained handlers.  This works, but I don't think we should rely on it.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v1

trivial patch to call _exit(1) in ah_crap_handler rather than returning.  abnormal_exit_handler already does this, so no need to update it.

Comment 2

[jag (Peter Annema)]

Comment on attachment 296281 [details] [diff] [review]
patch v1

FatalSignalHandler() returns the sig num passed in. Maybe you could do the same instead of returning 1?

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: alternate patch, calls _exit(signum) instead

It doesn't seem that useful to me because there's no way for someone examining the exit code to know if it is a signal number (i.e. from ah_crap_handler calling _exit(SIGABRT)) or some other coding exiting with a value that just happens to be the same value as SIGABRT on that platform.  For unhandled fatal signals on many Unix-like platforms the kernel sets the high bit of the exit value to indicate that the exit is the result of a signal.  We could do this, but I'm not sure how portable it would be. 

On the other hand, I'm not strongly against doing that.  Alternate patch attached.

Comment 4

[jag (Peter Annema)]

Comment on attachment 296285 [details] [diff] [review]
alternate patch, calls _exit(signum) instead

Hmm, I like the idea of setting the high bit. For this bug though I figure we shouldn't change the returned values.

Asking dbaron for r=

Comment 5

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

Comment on attachment 296285 [details] [diff] [review]
alternate patch, calls _exit(signum) instead

I don't quite follow why it is we want to do this -- but I suppose r=dbaron.  Though I'd actually prefer understanding what the point is.

Seems like you probably should set the high bit; I don't see why that's separate from this bug.


Are you saying that there's some other handler for these signals in the Mozilla codebase, and the only reason we exit is because of that one, and if that weren't there and we didn't exit we'd just keep running?

Comment 6

[Matthew Gregan [:kinetik]]

(In reply to comment #5)
> Are you saying that there's some other handler for these signals in the Mozilla
> codebase, and the only reason we exit is because of that one, and if that
> weren't there and we didn't exit we'd just keep running?

That's right.

Comment 7

[jag (Peter Annema)]

CC'ing David. See the previous comment.

I guess we should just set the high bit while we're at it.

Comment 8

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

Where is the other one?

Comment 9

[jag (Peter Annema)]

This is the one mentioned in comment 0: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/profile/dirserviceprovider/src/nsProfileLock.cpp&rev=1.16&mark=162,221#162

Comment 10

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

Hrm.  I though this code predated the profile locking code.

Also, will this make us not remove the profile lock file in DEBUG builds?

Comment 11

[Matthew Gregan [:kinetik]]

The profile lock will still be removed.  What happens now is that nsProfileLock::FatalSignalHandler handles the signal, does whatever it needs to, then calls the next handler in the chain (which was saved at the time nsProfileLock::FatalSignalHandler was installed).

So we currently have (in execution order):
1. nsProfileLock::FatalSignalHandler
2. ah_crap_handler

ah_crap_handler returns, then nsProfileLock::FatalSignalHandler calls exit(signum).  The patch makes ah_crap_handler do that instead, rather than accidentally relying on some other handler doing so.

Comment 12

[Matthew Gregan [:kinetik]]

Pushed 44d89e54ee9a.

Comment 13

[jag (Peter Annema)]

Matthew, is there a bug on setting the high bit?

Comment 14

[Matthew Gregan [:kinetik]]

Nope.  The landed patch retains the existing behaviour (as it does the same thing that nsProfileLock::FatalSignalHandler
 would've done) when exiting due to a signal handler.  If you file one, please CC me.