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.
Created attachment 296281 [details] [diff] [review] 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 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?
Created attachment 296285 [details] [diff] [review] 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 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 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?
Attachment #296285 - Flags: review?(dbaron) → review+
(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.
CC'ing David. See the previous comment. I guess we should just set the high bit while we're at it.
Where is the other one?
Hrm. I though this code predated the profile locking code. Also, will this make us not remove the profile lock file in DEBUG builds?
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Matthew, is there a bug on setting the high bit?
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.
You need to log in before you can comment on or make changes to this bug.