Last Comment Bug 426163 - MOZ_CRASH needs to use TerminateProcess instead of exit
: MOZ_CRASH needs to use TerminateProcess instead of exit
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
http://msdn2.microsoft.com/en-us/libr...
Depends on: 345118
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-31 04:32 PDT by timeless
Modified: 2012-06-22 03:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
change exit method (609 bytes, patch)
2008-03-31 04:32 PDT, timeless
no flags Details | Diff | Splinter Review
Patch (2.66 KB, patch)
2012-06-18 12:51 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
ted: review+
Details | Diff | Splinter Review

Description timeless 2008-03-31 04:32:32 PDT
Created attachment 312703 [details] [diff] [review]
change exit method

as suggested
Comment 1 Thomas K. (:tom) 2008-04-01 00:58:22 PDT
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) 2008-04-01 00:58:59 PDT
Whoops.  Sorry about the page wwwidening, I'm too used to markdown.
Comment 3 timeless 2008-04-07 06:17:45 PDT
as opposed to crashing because we reentered? resulting in crashes which confuse the heck out of QA? absolutely.
Comment 4 Thomas K. (:tom) 2008-04-07 10:43:35 PDT
(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 2008-04-07 11:06:06 PDT
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 -- probably not reading bugmail closely) 2008-06-05 06:50:29 PDT
Does this preserve the behaviour of the crash detection code in the test suite?
Comment 7 timeless 2008-06-05 09:25:23 PDT
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 -- probably not reading bugmail closely) 2008-06-05 10:12:15 PDT
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:] 2008-06-05 10:43:38 PDT
I'll try it now.
Comment 10 Bob Clary [:bc:] 2008-06-07 12:23:55 PDT
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] (remove +bmo to email) 2012-06-18 12:49:46 PDT
This stuff lives in mfbt now.  Patch shortly.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-18 12:51:01 PDT
Created attachment 634147 [details] [diff] [review]
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] (remove +bmo to email) 2012-06-21 01:47:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f9a61a1e43
Comment 14 Ed Morley [:emorley] 2012-06-21 02:07:58 PDT
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c710654ee747
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-21 11:22:51 PDT
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] 2012-06-22 03:46:47 PDT
https://hg.mozilla.org/mozilla-central/rev/a7fce1dd2b58

Note You need to log in before you can comment on or make changes to this bug.