MOZ_CRASH needs to use TerminateProcess instead of exit

RESOLVED FIXED in mozilla16



10 years ago
5 years ago


(Reporter: timeless, Assigned: Waldo)


Windows XP

Firefox Tracking Flags

(Not tracked)




(1 attachment, 1 obsolete attachment)



10 years ago
Created attachment 312703 [details] [diff] [review]
change exit method

as suggested
Attachment #312703 - Flags: review?(shaver)

Comment 1

10 years ago
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

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

Comment 3

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


10 years ago
Depends on: 345118

Comment 4

10 years ago
(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

10 years ago
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;
Summary: use TerminateProcess instead of exit → JS_Assert needs to use TerminateProcess instead of exit
Does this preserve the behaviour of the crash detection code in the test suite?

Comment 7

9 years ago
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 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.
Attachment #312703 - Flags: review?(shaver)

Comment 9

9 years ago
I'll try it now.

Comment 10

9 years ago
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?
This stuff lives in mfbt now.  Patch shortly.
Assignee: timeless → jwalden+bmo
Component: JavaScript Engine → MFBT
QA Contact: general → mfbt
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.
Attachment #312703 - Attachment is obsolete: true
Attachment #634147 - Flags: review?(ted.mielczarek)
Summary: JS_Assert needs to use TerminateProcess instead of exit → MOZ_CRASH needs to use TerminateProcess instead of exit
Attachment #634147 - Flags: review?(ted.mielczarek) → review+
Target Milestone: --- → mozilla16
Sigh, third time (and none of these times has the patch been wrong, it's just been guilt by association :-) ):
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.