[windows] Make calls to exit(), _exit(), abort(), and ExitProcess() from the renderer process resul…

[windows] Make calls to exit(), _exit(), abort(), and ExitProcess() from the renderer process result in a crash.

BUG=118665
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128701

[eroman, 2012-03-21 02:45:46]

@rvargas: please review everything

@jam: Please review renderer_main.cc; I was not sure if this is the right place
to put the code...

Context: I would like to extend this to other process types later, but for now
it just applies to renderers. Since trying to understand the renderer crash rate
on M18.

I experimented with several other implementations before settling on this
approach:

  - register atexit() with CRT. This worked poorly; the CRT is very mysterious.

  - Patch the import address table for kernel32!ExitProcess,
kernel32!TerminateProcess. This worked reasonably well and had the benefit of
catching the TerminateProcess exits. However it didn't allow a generic mechanism
for catching process terminations initiated by other modules. Also, the code was
much scarier.

[rvargas (doing something else), 2012-03-21 03:13:45]

http://codereview.chromium.org/9803002/diff/1/base/win/dllmain.cc
File base/win/dllmain.cc (right):

http://codereview.chromium.org/9803002/diff/1/base/win/dllmain.cc#newcode129
base/win/dllmain.cc:129: void DLLMain_SetCrashOnProcessDetach(bool crash) {
This is problematic because this file is not part of base.dll. It actually lives
in chrome land, so the namespace is kind of funky.

How about placing the static variable in its own file (living and exported from
base), and having this file just reading the value? That also means that the
breakpad code can freely call the method to reset the value.

http://codereview.chromium.org/9803002/diff/1/base/win/dllmain.h
File base/win/dllmain.h (right):

http://codereview.chromium.org/9803002/diff/1/base/win/dllmain.h#newcode6
base/win/dllmain.h:6: #define BASE_WIN_DLLMAIN_H_
This file should be added to base.gyp. (or some form of it)

http://codereview.chromium.org/9803002/diff/1/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/9803002/diff/1/chrome/app/breakpad_win.cc#newc...
chrome/app/breakpad_win.cc:523: ::TerminateProcess(::GetCurrentProcess(),
Don't you want to reset the flag from this place instead?

http://codereview.chromium.org/9803002/diff/1/chrome/common/child_process_log...
File chrome/common/child_process_logging.h (right):

http://codereview.chromium.org/9803002/diff/1/chrome/common/child_process_log...
chrome/common/child_process_logging.h:82: // Sets whether the process should be
forcibly crashed when it is exitted. This
nit: 'when it exits' ?

http://codereview.chromium.org/9803002/diff/1/content/renderer/renderer_main.cc
File content/renderer/renderer_main.cc (right):

http://codereview.chromium.org/9803002/diff/1/content/renderer/renderer_main....
content/renderer/renderer_main.cc:21: #include
"chrome/common/child_process_logging.h"
content cannot depend on chrome

[eroman, 2012-03-21 04:14:30]

@jam: Please hold off on this review, it is not ready yet :)

https://chromiumcodereview.appspot.com/9803002/diff/1/base/win/dllmain.cc
File base/win/dllmain.cc (right):

https://chromiumcodereview.appspot.com/9803002/diff/1/base/win/dllmain.cc#new...
base/win/dllmain.cc:129: void DLLMain_SetCrashOnProcessDetach(bool crash) {
On 2012/03/21 03:13:45, rvargas wrote:
> This is problematic because this file is not part of base.dll. It actually
lives
> in chrome land, so the namespace is kind of funky.
> 
> How about placing the static variable in its own file (living and exported
from
> base), and having this file just reading the value? That also means that the
> breakpad code can freely call the method to reset the value.

Good point.

How about if I add exported methods to win_util.h, say:
  bool win::base::GetShouldCrashOnExit();
  void win::base::SetShouldCrashOnExit(bool);

dllmain.cc would then depend on GetShouldCrashOnExit().

https://chromiumcodereview.appspot.com/9803002/diff/1/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/9803002/diff/1/chrome/app/breakpad_win...
chrome/app/breakpad_win.cc:523: ::TerminateProcess(::GetCurrentProcess(),
On 2012/03/21 03:13:45, rvargas wrote:
> Don't you want to reset the flag from this place instead?

(Didn't reset because was harder; this code is running in chrome.exe)

I think as far as correctness is concerned TerminateProcess() is good, since I
can't imagine wanting to run the shutdown code when badness has happened.

But I can certainly change that.

[rvargas (doing something else), 2012-03-21 17:46:11]

https://chromiumcodereview.appspot.com/9803002/diff/1/base/win/dllmain.cc
File base/win/dllmain.cc (right):

https://chromiumcodereview.appspot.com/9803002/diff/1/base/win/dllmain.cc#new...
base/win/dllmain.cc:129: void DLLMain_SetCrashOnProcessDetach(bool crash) {
Sounds good.

https://chromiumcodereview.appspot.com/9803002/diff/1/chrome/app/breakpad_win.cc
File chrome/app/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/9803002/diff/1/chrome/app/breakpad_win...
chrome/app/breakpad_win.cc:523: ::TerminateProcess(::GetCurrentProcess(),
On 2012/03/21 04:14:31, eroman wrote:
> On 2012/03/21 03:13:45, rvargas wrote:
> > Don't you want to reset the flag from this place instead?
> 
> (Didn't reset because was harder; this code is running in chrome.exe)
> 

Right, the exe... TerminateProcess sounds good. (There's still some concern
about people copying this code and going around your patch, but hopefully they
won't)


> I think as far as correctness is concerned TerminateProcess() is good, since I
> can't imagine wanting to run the shutdown code when badness has happened.
> 
> But I can certainly change that.

[eroman, 2012-03-22 01:44:36]

@rvargas: please take another look.

[rvargas (doing something else), 2012-03-22 02:55:58]

LGTM

https://chromiumcodereview.appspot.com/9803002/diff/8002/content/renderer/ren...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://chromiumcodereview.appspot.com/9803002/diff/8002/content/renderer/ren...
content/renderer/renderer_main_platform_delegate_win.cc:86: // Also disable
the_CALL_REPORTFAULT behavior, since it has the sideffect of
nit: the <space> _CALL_RE...

https://chromiumcodereview.appspot.com/9803002/diff/8002/content/renderer/ren...
content/renderer/renderer_main_platform_delegate_win.cc:89:
_set_abort_behavior(0, _CALL_REPORTFAULT);
_set_abort_bahavior(0, _WRITE_A_M | _CALL_R); ? (I would expect that to work)

[eroman, 2012-03-22 04:30:23]

https://chromiumcodereview.appspot.com/9803002/diff/8002/content/renderer/ren...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://chromiumcodereview.appspot.com/9803002/diff/8002/content/renderer/ren...
content/renderer/renderer_main_platform_delegate_win.cc:86: // Also disable
the_CALL_REPORTFAULT behavior, since it has the sideffect of
On 2012/03/22 02:55:59, rvargas wrote:
> nit: the <space> _CALL_RE...

Done.

https://chromiumcodereview.appspot.com/9803002/diff/8002/content/renderer/ren...
content/renderer/renderer_main_platform_delegate_win.cc:89:
_set_abort_behavior(0, _CALL_REPORTFAULT);
On 2012/03/22 02:55:59, rvargas wrote:
> _set_abort_bahavior(0, _WRITE_A_M | _CALL_R); ? (I would expect that to work)

Done.

[eroman, 2012-03-22 04:33:03]

@jam: This is ready for your review now, please take a look.

[jam, 2012-03-22 21:09:48]

On 2012/03/22 04:33:03, eroman wrote:
> @jam: This is ready for your review now, please take a look.

lgtm++

this would have made life much simpler when tracking pdf issues :)

[eroman, 2012-03-22 21:26:33]

@brettw: Could you please do an OWNERs review for base/* ?

[brettw, 2012-03-23 21:16:37]

lgtm

http://codereview.chromium.org/9803002/diff/14005/content/renderer/renderer_m...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

http://codereview.chromium.org/9803002/diff/14005/content/renderer/renderer_m...
content/renderer/renderer_main_platform_delegate_win.cc:5: #include <signal.h>
I'd put this after the first header file.

http://codereview.chromium.org/9803002/diff/14005/content/renderer/renderer_m...
content/renderer/renderer_main_platform_delegate_win.cc:75: void __cdecl
ForceCrashOnAbort(int) {
Can you call this ...SigAbort or something? At first I was confused why you have
two versions of this function (the other being in base).

[eroman, 2012-03-23 21:26:23]

Thanks for the reviews!

http://codereview.chromium.org/9803002/diff/14005/content/renderer/renderer_m...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

http://codereview.chromium.org/9803002/diff/14005/content/renderer/renderer_m...
content/renderer/renderer_main_platform_delegate_win.cc:5: #include <signal.h>
On 2012/03/23 21:16:37, brettw wrote:
> I'd put this after the first header file.

Done.

http://codereview.chromium.org/9803002/diff/14005/content/renderer/renderer_m...
content/renderer/renderer_main_platform_delegate_win.cc:75: void __cdecl
ForceCrashOnAbort(int) {
On 2012/03/23 21:16:37, brettw wrote:
> Can you call this ...SigAbort or something? At first I was confused why you
have
> two versions of this function (the other being in base).

Done.

[commit-bot: I haz the power, 2012-03-23 21:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9803002/12002

[commit-bot: I haz the power, 2012-03-24 02:40:27]

Try job failure for 9803002-12002 (retry) on win_rel for steps "ui_tests,
browser_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-24 02:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9803002/12002

[commit-bot: I haz the power, 2012-03-24 06:03:19]

Change committed as 128701