JS_Assert DebugBreak|abort windows foo

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: bc, Assigned: bc)

Tracking

({fixed1.8.0.7, fixed1.8.1})

Trunk
x86
Windows XP
fixed1.8.0.7, fixed1.8.1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
Attachment #229724 - Flags: review?(benjamin)
Comment on attachment 229724 [details] [diff] [review]
patch

I am not a js/ peer.
Attachment #229724 - Flags: review?(benjamin) → review?(mrbkap)
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).
Attachment #229724 - Flags: superreview?(brendan)
Attachment #229724 - Flags: review?(mrbkap)
Attachment #229724 - Flags: review+
(Assignee)

Comment 4

11 years ago
Maybe _exit would be closer to abort
<http://msdn2.microsoft.com/en-us/library/6wdz5232.aspx>
Comment on attachment 229724 [details] [diff] [review]
patch

Windows, sigh.

/be
Attachment #229724 - Flags: superreview?(brendan) → superreview+
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
Attachment #229724 - Flags: approval1.8.1?
Comment on attachment 229724 [details] [diff] [review]
patch

a=drivers. Please land on MOZILLA_1_8_BRANCH.
Attachment #229724 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

11 years ago
Assignee: general → bclary
(Assignee)

Comment 8

11 years ago
fixed on trunk

Checking in jsutil.c;
/cvsroot/mozilla/js/src/jsutil.c,v  <--  jsutil.c
new revision: 3.27; previous revision: 3.26
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Assignee)

Comment 9

11 years ago
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
Keywords: fixed1.8.1
(Assignee)

Comment 10

11 years ago
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.
Attachment #229724 - Flags: approval1.8.0.6?
Comment on attachment 229724 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #229724 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 12

11 years ago
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?
> 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.
(Assignee)

Comment 14

11 years ago
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. :-(
Keywords: fixed1.8.0.7

Comment 15

9 years ago
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
TerminateProcess is probably better, yeah; wanna whip up a patch for that in a separate bug, timeless?  I'll review.

Updated

9 years ago
Blocks: 426163
You need to log in before you can comment on or make changes to this bug.