587982 - Use _CrtSetReportHook2 to replace MS CRT assert dialogs

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Bob Clary [:bc] (inactive)]

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

Comment 1

[Bob Clary [:bc] (inactive)]

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.

Comment 2

[(not currently active) Ted Mielczarek]

You probably want to put these calls right here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3859

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: patch

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

Comment 5

[(not currently active) 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.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: 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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can this land?

Comment 8

[Bob Clary [:bc] (inactive)]

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.

Comment 9

[Bob Clary [:bc] (inactive)]

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.

Comment 10

[(not currently active) Ted Mielczarek]

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).

Comment 11

[Bob Clary [:bc] (inactive)]

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.

Comment 12

[Bob Clary [:bc] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=262dd3bdd46e

Comment 13

[Bob Clary [:bc] (inactive)]

passed try. pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceb5b26840e0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ceb5b26840e0