Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 426163 - MOZ_CRASH needs to use TerminateProcess instead of exit
: MOZ_CRASH needs to use TerminateProcess instead of exit
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)
Depends on: 345118
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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.

BOOL WINAPI TerminateProcess(
  __in  HANDLE hProcess,
  __in  UINT 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.

GetExitCodeProcess Function

Retrieves the termination status of the specified process.

BOOL WINAPI GetExitCodeProcess(
  __in   HANDLE hProcess,
  __out  LPDWORD lpExitCode


    A pointer to a variable to receive the process termination status. For more information, see 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]

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
Comment 14 Ed Morley [:emorley] 2012-06-21 02:07:58 PDT
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 :-) ):
Comment 16 Ed Morley [:emorley] 2012-06-22 03:46:47 PDT

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