Closed Bug 345118 Opened 13 years ago Closed 13 years ago

JS_Assert DebugBreak|abort windows foo

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bc, Assigned: bc)

References

()

Details

(Keywords: fixed1.8.0.7, fixed1.8.1)

Attachments

(2 files)

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.
Attached patch patchSplinter Review
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+
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: general → bclary
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
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
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
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+
Attached patch patch for 1.8.0Splinter Review
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.
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
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.
Blocks: 426163
You need to log in before you can comment on or make changes to this bug.