Use _CrtSetReportHook2 to replace MS CRT assert dialogs

RESOLVED FIXED in mozilla18

Status

--
enhancement
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
mozilla18
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
When running tests on Windows in debug builds, MS CRT assertions [1] can fire for such errors as heap errors. These dialogs are not automatically dismissible and can cause tests to stop running until they are timed out. 

We could turn off the debug heap by using _NO_DEBUG_HEAP=1 at the loss of information related to heap errors and the availability of the special memory markers in debug builds.

We should create a way to turn these assertions into warnings in the same way we do for NS_ASSERTION and XPCOM_DEBUG_BREAK=warn. _CrtSetReportHook2 and friends [2] can be used for this.

An example can be found at [3].

This affects the automated crash testing where I hit CRT Assertions regularly with more complicated userhook scripts and I believe it may also be a problem for testing in general.

It would be good to have this on all supported branches.


[1] http://msdn.microsoft.com/en-us/library/3ekw7kc1%28v=VS.80%29.aspx
[2] http://msdn.microsoft.com/en-us/library/94a21kwy%28VS.80%29.aspx
[3] http://www.tilander.org/aurora/2008/01/stomping-on-dialog-boxes.html
(Assignee)

Comment 1

8 years ago
18:37 <    shaver> call _CrtSetReportMode(_CRT_ASSERT, _CRT_MODE_FILE)
18:37 <    shaver> then
18:38 <    shaver> _CrtSetReportFile(_CRT_ASSERT, filehandle)
18:38 <    shaver> http://msdn.microsoft.com/en-us/library/a68f826y%28v=VS.71%29.aspx
18:39 <@        bc> cool.
18:40 <@        bc> i completely didn't find that.
(Assignee)

Comment 3

7 years ago
Created attachment 552686 [details] [diff] [review]
patch
Attachment #552686 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 4

7 years ago
Created attachment 553024 [details] [diff] [review]
patch v2
Attachment #552686 - Attachment is obsolete: true
Attachment #553024 - Flags: review?(benjamin)
Attachment #553024 - Flags: feedback?(ted.mielczarek)
Attachment #552686 - Flags: feedback?(ted.mielczarek)

Updated

7 years ago
Attachment #553024 - Flags: review?(ted.mielczarek)
Attachment #553024 - Flags: review?(benjamin)
Attachment #553024 - Flags: feedback?(ted.mielczarek)
Comment on attachment 553024 [details] [diff] [review]
patch v2

Review of attachment 553024 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsAppRunner.cpp
@@ +1347,5 @@
> +    break;
> +  case 2:
> +    fputs("\n###!!! ABORT: CRT ASSERT ", stderr);
> +    mozalloc_abort(aMessage);
> +    break;

Do CRT assertions normally terminate the program?

@@ +3776,5 @@
>    SetErrorMode(realMode);
>  
>  #endif
>  
> +#if defined (DEBUG) && defined(XP_WIN32)

XP_WIN, not XP_WIN32.
Attachment #553024 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → bclary
(Assignee)

Comment 6

7 years ago
Created attachment 553682 [details] [diff] [review]
patch with review comment addressed

(In reply to Ted Mielczarek [:ted, :luser] from comment #5)

> Do CRT assertions normally terminate the program?

No. They display an assertion dialog which allows the user to debug or ignore it.

> XP_WIN, not XP_WIN32.

fixed.
(Assignee)

Updated

7 years ago
Depends on: 681704
(Assignee)

Comment 8

7 years ago
jseng needs to triage bug 681704 first otherwise the test will abort. I'll probably need to run it through try again as well to see what else might have failed.

Updated

6 years ago
Blocks: 774657
(Assignee)

Comment 9

6 years ago
submitted to try:
try: -b d -e -p win32,win64 -u reftest,reftest-no-accel,crashtest,xpcshell,jsreftest,jetpack,mozmill-all,opengl,peptest,mochitestsperfchrome.2,nochrome.2,dirty,tpr_responsiveness,tprow,svg,dromaeo,xx

https://tbpl.mozilla.org/?tree=Try&rev=56e0ecf68e93

I'll file anything untoward and push for fixes.
I'd love to see this landed. Turns out the CRT assertion dialog boxes can re-enter our event loop, which can lead to deadlock (and you don't get to see the dialog either).
(Assignee)

Comment 11

6 years ago
Now that try has settled down and I have permission to disable the test in Bug 681704, I'll land the patch to disable js1_5/extensions/toLocaleFormat-02.js and will do a try run over the weekend. If that is clean I'll land it.
(Assignee)

Comment 13

6 years ago
passed try. pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb5b26840e0
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ceb5b26840e0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.