Closed Bug 426163 Opened 12 years ago Closed 7 years ago

MOZ_CRASH needs to use TerminateProcess instead of exit

Categories

(Core :: MFBT, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: timeless, Assigned: Waldo)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Attached patch change exit method (obsolete) — Splinter Review
as suggested
Attachment #312703 - Flags: review?(shaver)
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.
Whoops.  Sorry about the page wwwidening, I'm too used to markdown.
as opposed to crashing because we reentered? resulting in crashes which confuse the heck out of QA? absolutely.
Status: NEW → ASSIGNED
Depends on: 345118
(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.
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?
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 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)
I'll try it now.
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
Attached patch PatchSplinter 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+
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
https://hg.mozilla.org/mozilla-central/rev/a7fce1dd2b58
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.