Last Comment Bug 345118 - JS_Assert DebugBreak|abort windows foo
: JS_Assert DebugBreak|abort windows foo
Status: RESOLVED FIXED
: fixed1.8.0.7, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Bob Clary [:bc:]
:
Mentors:
http://lxr.mozilla.org/mozilla/source...
Depends on:
Blocks: 426163
  Show dependency treegraph
 
Reported: 2006-07-18 14:51 PDT by Bob Clary [:bc:]
Modified: 2008-04-07 06:21 PDT (History)
4 users (show)
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (499 bytes, patch)
2006-07-18 14:54 PDT, Bob Clary [:bc:]
mrbkap: review+
brendan: superreview+
dveditz: approval1.8.0.7+
mbeltzner: approval1.8.1+
Details | Diff | Splinter Review
patch for 1.8.0 (685 bytes, patch)
2006-08-15 20:07 PDT, Bob Clary [:bc:]
no flags Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2006-07-18 14:51:07 PDT
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:] 2006-07-18 14:54:07 PDT
Created attachment 229724 [details] [diff] [review]
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 [:bsmedberg] 2006-07-24 13:13:36 PDT
Comment on attachment 229724 [details] [diff] [review]
patch

I am not a js/ peer.
Comment 3 Blake Kaplan (:mrbkap) 2006-07-24 13:30:08 PDT
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:] 2006-07-24 13:37:18 PDT
Maybe _exit would be closer to abort
<http://msdn2.microsoft.com/en-us/library/6wdz5232.aspx>
Comment 5 Brendan Eich [:brendan] 2006-07-24 15:32:45 PDT
Comment on attachment 229724 [details] [diff] [review]
patch

Windows, sigh.

/be
Comment 6 Brendan Eich [:brendan] 2006-07-24 15:33:25 PDT
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] 2006-07-25 10:40:22 PDT
Comment on attachment 229724 [details] [diff] [review]
patch

a=drivers. Please land on MOZILLA_1_8_BRANCH.
Comment 8 Bob Clary [:bc:] 2006-07-25 19:40:06 PDT
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:] 2006-07-25 19:49:58 PDT
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:] 2006-07-25 19:51:48 PDT
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] 2006-08-15 15:13:25 PDT
Comment on attachment 229724 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 12 Bob Clary [:bc:] 2006-08-15 20:07:39 PDT
Created attachment 233923 [details] [diff] [review]
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] 2006-08-18 12:39:32 PDT
> 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:] 2006-08-21 12:08:34 PDT
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 2008-02-25 05:50:56 PST
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 -- probably not reading bugmail closely) 2008-02-27 06:09:06 PST
TerminateProcess is probably better, yeah; wanna whip up a patch for that in a separate bug, timeless?  I'll review.

Note You need to log in before you can comment on or make changes to this bug.