339987 - clean up Mac OS X debug break impl (make XPCOM_DEBUG_BREAK=trap work)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Aas]

On Mac OS X, the debugger won't be invoked on assertions when XPCOM_DEBUG_BREAK is set to "trap". This is because we call the "Debugger()" function, which only works if the USERBREAK env variable is set. We have no way around this on PPC, all we can do is add a comment in that code to point out that you have to set both env variables and output a warning. However, on Intel Mac OS X we can just use the same GNUC/i386 code other platforms do. It looks like that should already be happening on i386 Mac OS X, but the i386 macro being tested for right now is not defined on Mac OS X.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Håkan Waara]

We could use setenv() to set the needed env var on runtime, if it isn't explicitly set to 0...

Comment 3

[Mark Mentovai]

When USERBREAK is set, Debugger() just sends SIGINT.  Why not replace Debugger() with raise(SIGINT)?

Comment 4

[Josh Aas]

I don't want to set the environment variable for people because I don't like the idea of modifying their environment without them doing it explicitly.

There is another patch coming up with a fix for PPC and I'll try the raise() thing.

Comment 5

[Josh Aas]

Attachment: fix v1.1

Comment 6

[Josh Aas]

Comment on attachment 224103 [details] [diff] [review]
fix v1.1

I put Mac OS X's macro above the GNUC/x86 macro so it get priority, and if fixed GNUC/x86 to include __i386__ so it would get triggered on Mac OS X and other platforms that don't define __i386. I left __i386 in case some platforms require it.

Comment 7

[Jesse Ruderman]

Is it a recent regression that XPCOM_DEBUG_BREAK=trap doesn't work?  I ran into this issue today, but I thought I'd used XPCOM_DEBUG_BREAK=trap before without a problem.

Comment 8

[Josh Aas]

It isn't really a regression. If you're on PPC, the assertion would call Debugger() in order to trap, which is a Mac OS X function. However, Debugger() didn't work unless you had USERBREAK=1 defined as an env variable. So really, on Mac OS X you had to set 2 env variables, USERBREAK=1 and XPCOM_DEBUG_BREAK=trap. That is sort of silly. This patch fixes that.

Comment 9

[Mark Mentovai]

Actually, SIGTRAP might fit the bill better, even though SIGINT is what Debugger() uses.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 224103 [details] [diff] [review]
fix v1.1

looks fine.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 224103 [details] [diff] [review]
fix v1.1

this is safe for the branch.

Comment 12

[Josh Aas]

Attachment: branch fix v1.1

Comment 13

[Josh Aas]

landed on trunk and 1.8.1 branch

Comment 14

[Jesse Ruderman]

Now if I hit an assertion outside of the debugger, Firefox kills itself and I don't even see an OS crash dialog with a stack trace :(

Comment 15

[Josh Aas]

Jesse - this behavior shouldn't be an issue unless you have an environment variable set that has assertions send SIGTRAP. I use XPCOM_DEBUG_BREAK=trap to turn that on, do you have that set somehow?

Comment 16

[Jesse Ruderman]

Yes, I had done "export XPCOM_DEBUG_BREAK=trap" earlier in the terminal session in order to catch assertions in gdb.  Now I have to set it back to "warn" when I run Firefox outside of gdb, which isn't ideal.

Comment 17

[Mark Mentovai]

In the past, with XPCOM_DEBUG_BREAK=trap, we'd keep running.  If USERBREAK was set, we'd die by signal (SIGINT) and not bring up the OS crash reporter.

Now, with XPCOM_DEBUG_BREAK=trap set, we by signal (SIGTRAP) and don't bring up the OS crash reporter.  The current version of the OS crash reporter doesn't handle uncaught POSIX signals raised like this.  It catches Mach exceptions and a few other types of errors.

The new SIGTRAP implementation will generate a core file (if cores are enabled).  The old Debugger() implementation would not generate a core file even with USERBREAK set because SIGINT's default behavior is to simply exit.

As was intended, the new behavior is exactly as the old behavior was, except the need to set a second environment variable (USERBREAK) is eliminated.

It's not unreasonable to expect the OS crash reporter to appear for assertions with XPCOM_DEBUG_BREAK=trap, and it might be nice, but there's really no precedent for it to work this way.  We might be able to do it with exception_raise, and hope the result is something catchable during conventional debugging.