345118 - JS_Assert DebugBreak|abort windows foo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

On Windows, JS_Assert calls DebugBreak <http://windowssdk.msdn.microsoft.com/en-us/library/ms679297.aspx>, then calls abort

DebugBreak causes an unhandled exception on Windows however abort() <http://msdn2.microsoft.com/en-us/library/k089yyh0.aspx> will also invoke the Abort, Retry, Ignore dialog in addition to the invocation caused by DebugBreak.

abort() causes a SIGABRT to be sent in single threaded builds, but according to the source source for abort.c in the VC8 crt

*   Multi-thread version does not raise SIGABRT -- this isn't supported
*   under multi-thread.

This results in calls to JS_Assert not reporting a non-zero exit code in Firefox and the js shell since they are multithreaded which can result in the JS_Asserts being missed when processing logs of the automated js tests.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: patch

This patch just adds an exit(3) after the DebugBreak call to make sure that the program is terminated with the same exit code as a single threaded app would have if terminated by abort(), and prevents the duplicate Abort, Retry, Ignore dialogs in debug builds. The only difference is if JS_Assert is called in a release build, then it would terminate without the Unusual termination Ok dialog.

Comment 2

[Benjamin Smedberg]

Comment on attachment 229724 [details] [diff] [review]
patch

I am not a js/ peer.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 229724 [details] [diff] [review]
patch

This *should* be fine, since we do break into the debugger *first*, but abort() and exit() *do* do different things (I'm thinking of destructors running on the way up the stack in particular).

Comment 4

[Bob Clary [:bc] (inactive)]

Maybe _exit would be closer to abort
<http://msdn2.microsoft.com/en-us/library/6wdz5232.aspx>

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 229724 [details] [diff] [review]
patch

Windows, sigh.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 229724 [details] [diff] [review]
patch

This is DEBUG only and it obviously should go into the 1.8 branch for testing sanity.

/be

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229724 [details] [diff] [review]
patch

a=drivers. Please land on MOZILLA_1_8_BRANCH.

Comment 8

[Bob Clary [:bc] (inactive)]

fixed on trunk

Checking in jsutil.c;
/cvsroot/mozilla/js/src/jsutil.c,v  <--  jsutil.c
new revision: 3.27; previous revision: 3.26

Comment 9

[Bob Clary [:bc] (inactive)]

fixed on 1.8 branch
Checking in jsutil.c;
/cvsroot/mozilla/js/src/jsutil.c,v  <--  jsutil.c
new revision: 3.21.4.4; previous revision: 3.21.4.3

Comment 10

[Bob Clary [:bc] (inactive)]

Comment on attachment 229724 [details] [diff] [review]
patch

No risk, debug only, testing request for 1.8.0.6 after 1.8.0.5 is out the door.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 229724 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: patch for 1.8.0

the approved patch does not apply to the 1.8.0 branch. Can I just check this in without re-requesting approval for 1.8.0?

Comment 13

[Daniel Veditz [:dveditz]]

> the approved patch does not apply to the 1.8.0 branch. Can I just check this in
> without re-requesting approval for 1.8.0?

In this case yes, a=me, thanks for asking.

There's a kind of blurry line between context merge changes that are substantive and need some sort of second look or approval, and those that don't. Changes in an unrelated (but nearby) #elif are pretty clearly on the non-substantive side :-)

If you think it needs re-review then it probably needs separate approval. If it's trivial enough that you know the module owner is going to be OK not re-reviewing it then you can generally carry over the approval.

Comment 14

[Bob Clary [:bc] (inactive)]

Checking in jsutil.c;
/cvsroot/mozilla/js/src/jsutil.c,v  <--  jsutil.c
new revision: 3.21.10.1; previous revision: 3.21

damn, forgot the bug number in the checkin comment. :-(

Comment 15

[timeless]

eww. i'm sorry i didn't see this bug. it's unfortunate and i'd request that it be reverted.

bug 419192 comment 7 suffers from this change, namely we now have random confusing behaviors.

I think you want TerminateProcess:
The TerminateProcess function is used to unconditionally cause a process to exit. The state of global data maintained by dynamic-link libraries (DLLs) may be compromised if TerminateProcess is used rather than ExitProcess.

http://msdn2.microsoft.com/en-us/library/ms686714.aspx

Comment 16

[Mike Shaver (:shaver emeritus)]

TerminateProcess is probably better, yeah; wanna whip up a patch for that in a separate bug, timeless?  I'll review.