Closed Bug 339987 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XPCOM, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

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.
Attached patch fix v1.0 (obsolete) — Splinter Review
We could use setenv() to set the needed env var on runtime, if it isn't explicitly set to 0...
When USERBREAK is set, Debugger() just sends SIGINT.  Why not replace Debugger() with raise(SIGINT)?
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.
Attached patch fix v1.1Splinter Review
Attachment #224098 - Attachment is obsolete: true
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.
Attachment #224103 - Flags: review?(mark)
Attachment #224103 - Flags: review?(mark) → review+
Attachment #224103 - Flags: superreview?(shaver)
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.
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.
Actually, SIGTRAP might fit the bill better, even though SIGINT is what Debugger() uses.
Attachment #224103 - Flags: superreview?(shaver) → superreview?(dougt)
Comment on attachment 224103 [details] [diff] [review]
fix v1.1

looks fine.
Attachment #224103 - Flags: superreview?(dougt) → superreview+
Attachment #224103 - Flags: approval-branch-1.8.1?(dougt)
Comment on attachment 224103 [details] [diff] [review]
fix v1.1

this is safe for the branch.
Attachment #224103 - Flags: approval-branch-1.8.1?(dougt) → approval-branch-1.8.1+
Attached patch branch fix v1.1Splinter Review
landed on trunk and 1.8.1 branch
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
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 :(
Summary: clean up Mac OS X debug break impl → clean up Mac OS X debug break impl (make XPCOM_DEBUG_BREAK=trap work)
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?
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.
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.
Blocks: 314800
You need to log in before you can comment on or make changes to this bug.