Closed Bug 587982 Opened 11 years ago Closed 9 years ago

Use _CrtSetReportHook2 to replace MS CRT assert dialogs


(Testing :: General, enhancement)

Windows 7
Not set


(Not tracked)



(Reporter: bc, Assigned: bc)





(2 files, 1 obsolete file)

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.

18:37 <    shaver> call _CrtSetReportMode(_CRT_ASSERT, _CRT_MODE_FILE)
18:37 <    shaver> then
18:38 <    shaver> _CrtSetReportFile(_CRT_ASSERT, filehandle)
18:38 <    shaver>
18:39 <@        bc> cool.
18:40 <@        bc> i completely didn't find that.
Attached patch patch (obsolete) — Splinter Review
Attachment #552686 - Flags: feedback?(ted.mielczarek)
Attached patch patch v2Splinter Review
Attachment #552686 - Attachment is obsolete: true
Attachment #553024 - Flags: review?(benjamin)
Attachment #553024 - Flags: feedback?(ted.mielczarek)
Attachment #552686 - Flags: feedback?(ted.mielczarek)
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
(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.

Depends on: 681704
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.
Blocks: 774657
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

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).
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.
passed try. pushed to inbound
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.