426163 - MOZ_CRASH needs to use TerminateProcess instead of exit

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[timeless]

Attachment: change exit method

as suggested

Comment 1

[Thomas K. (:tom)]

Are you sure that we (and I use the term we very loosely here) would want to do this?

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

>TerminateProcess initiates termination and returns immediately. This stops execution of all threads within the process and requests cancellation of all pending I/O. The terminated process cannot exit until all pending I/O has been completed or canceled.

>A process cannot prevent itself from being terminated.

Comment 2

[Thomas K. (:tom)]

Whoops.  Sorry about the page wwwidening, I'm too used to markdown.

Comment 3

[timeless]

as opposed to crashing because we reentered? resulting in crashes which confuse the heck out of QA? absolutely.

Comment 4

[Thomas K. (:tom)]

(In reply to comment #3)
> as opposed to crashing because we reentered? resulting in crashes which confuse
> the heck out of QA? absolutely.
> 

Sorry, I'm not trying to be rude (far from it), I am just not familiar with the setup.  The way I read it, it looked like TerminateProcess = kill -9 on the process, causing a quick exit that didn't do any cleanup and state saving.  If that is not the case, ok.  Sorry for asking.

Comment 5

[timeless]

yes, it does. but this is for assert() failures, and that's precisely what we want. this isn't for the standard exit-point which actually uses return 0;

Comment 6

[Mike Shaver (:shaver emeritus)]

Does this preserve the behaviour of the crash detection code in the test suite?

Comment 7

[timeless]

shaver, from the documentation (which is the bug's url)

TerminateProcess Function

Terminates the specified process and all of its threads.
Syntax

BOOL WINAPI TerminateProcess(
  __in  HANDLE hProcess,
  __in  UINT uExitCode
);

Parameters
...
uExitCode

    The exit code to be used by the process and threads terminated as a result of this call. Use the GetExitCodeProcess function to retrieve a process's exit value. Use the GetExitCodeThread function to retrieve a thread's exit value.

http://msdn.microsoft.com/en-us/library/ms683189(VS.85).aspx

GetExitCodeProcess Function

Retrieves the termination status of the specified process.
Syntax

BOOL WINAPI GetExitCodeProcess(
  __in   HANDLE hProcess,
  __out  LPDWORD lpExitCode
);

Parameters
...
lpExitCode

    A pointer to a variable to receive the process termination status. For more information, see Remarks.
...
Remarks

This function returns immediately. If the process has not terminted and the function succeeds, the status returned is STILL_ACTIVE. If the process has terminated and the function succeeds, the status returned is one of the following values:

    * The exit value specified in the ExitProcess or TerminateProcess function.
    * The return value from the main or WinMain function of the process.
    * The exception value for an unhandled exception that caused the process to terminate.

[note to self: file bug against spelling@msdn]

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 312703 [details] [diff] [review]
change exit method

Is that your way of saying "I didn't bother to run the test suite"?  Docs are nice, but are no substitute for data.

Request review when you've confirmed that the test suite runs correctly with the patch.

Comment 9

[Bob Clary [:bc] (inactive)]

I'll try it now.

Comment 10

[Bob Clary [:bc] (inactive)]

The suite for 1.9.0 has identical results on windows xp with an without the patch although none of the abnormal exits with value 3 are due to assertions. checking individual debug builds with and without the patch show both appear to exit with values of 3. I don't think this will cause any problems for the test suite.

In the future, would it be ok to drop the exit code of 3 as that is easily confused with the exit code of 3 that the shell returns when a js error causes a script to fail?

Comment 11

[Jeff Walden [:Waldo]]

This stuff lives in mfbt now.  Patch shortly.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Patch

Regarding comment 6, there's actually a test for Breakpad behavior for this stuff, and Breakpad actually trips over the __debugbreak() earlier in MOZ_CRASH(), not over the exit/TerminateProcess, so we're good here.

Comment 13

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79f9a61a1e43

Comment 14

[Ed Morley [:emorley]]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c710654ee747

Comment 15

[Jeff Walden [:Waldo]]

Sigh, third time (and none of these times has the patch been wrong, it's just been guilt by association :-) ):

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fce1dd2b58

Comment 16

[Ed Morley [:emorley]]

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