524522 - Tinderbox debug build unit tests don't capture a stack trace for JS_ASSERT assertion failure

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

A JS_ASSERT failure does asm("int $3"), which raises SIGTRAP, which breakpad doesn't consider a report-worthy crash.

The reason we do this is bug 308395. It's hard to get a program to continue running in GDB once abort() has been called.

I propose calling raise(SIGABRT) instead of "int $3" or abort(). This will cause breakpad to engage, but unlike abort() it's resumable:

  Assertion failure: "oops" == "ugh", at ../jstracer.cpp:9250

  Program received signal SIGABRT, Aborted.
  0xb7fe1430 in __kernel_vsyscall ()
  (gdb) signal 0
  Continuing with no signal.
  js>

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Removing OS2 cruft as I don't think OS2 folks are debugging JS assertions much these days.

Still needs testing on Mac, hang on.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

Works fine on Mac.

Let's do this on all platforms except Windows.

This would be the right thing to do on Windows too (<signal.h> and raise are supported there), but I'm not sure it would work. Bug 345118 comment 0 says that a comment in the CRT source code says it wouldn't work; the MSDN documentation for raise() seems to say it would. Windows users, feel free to try it.

Comment 3

[(not currently active) Ted Mielczarek]

I assume on Windows you'd want DebugBreak for the debugger, but I don't know what that does to Breakpad:
http://msdn.microsoft.com/en-us/library/ms679297%28VS.85%29.aspx

Comment 4

[(not currently active) Ted Mielczarek]

Oh, apparently Breakpad can handle them, but you have to explicitly ask for it, precisely so it doesn't interfere with your debugger:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#408

Comment 5

[Jason Orendorff [:jorendorff]]

I pushed this change directly to m-c.

http://hg.mozilla.org/mozilla-central/rev/cf193e9f7eaa

This is debug-only but I think we probably want it in 1.9.2 anyway after a little baking.

Comment 6

[Jason Orendorff [:jorendorff]]

Ted, how should I test this?

I tried building a browser with --enable-crashreporter and a manually added bogus JS_ASSERT, and ran it locally with MOZ_CRASHREPORTER=1 in my environment, but the crash reporter didn't engage.

Comment 7

[(not currently active) Ted Mielczarek]

That should be sufficient. If you'd like a more formal test, I have unit tests in attachment 400154 [details] [diff] [review] that actually test our exception handler. You could replace the guts of nsTestCrasher::Crash(), and then run make && make check in toolkit/crashreporter/test.

Comment 8

[Jason Orendorff [:jorendorff]]

Per comment 6 and some discussion with Ted on IRC, I think this still isn't fixed on Mac, where breakpad does not respond to signals (!).

The solution, I think, is to write to NULL, on Mac only, before calling raise(SIGABRT).

Comment 9

[Jason Orendorff [:jorendorff]]

Pushed the change suggested in comment 8 paragraph 2 to m-c:

http://hg.mozilla.org/mozilla-central/rev/eba15b3faf31

Comment 10

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in
transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked
fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300
days. Some of these got lost simply because the assignee/patch provider never
requested a checkin, or just because they were forgotten about.

Comment 11

[Nicholas Nethercote [inactive]]

This landed, jorendorff just forgot to update the status.