Cleanup sandbox/ in advance of re-enabling some MSVC warnings.

sandbox/win/src/target_process.cc

Index: sandbox/win/src/target_process.cc
diff --git a/sandbox/win/src/target_process.cc b/sandbox/win/src/target_process.cc
index fea52b2a6885459b29e6fa671d32ba5705a331ea..c524b7e41a86aedf1498e882914944b66a540ede 100644
--- a/sandbox/win/src/target_process.cc
+++ b/sandbox/win/src/target_process.cc
@@ -95,7 +95,9 @@ TargetProcess::~TargetProcess() {
       // have an IPC reach us after this point.
       if (shared_section_.IsValid())
         shared_section_.Take();
-      SharedMemIPCServer* server = ipc_server_.release();
+      // !!! Does |ipc_server_| get cleaned up properly somewhere else, or is
+      // leaking it OK?

[Peter Kasting, 2014/09/29 23:09:26]

This question must be answered before this change can be committed.

[cpu_(ooo_6.6-7.5), 2014/09/30 20:23:45]

The theory is that the job object has been closed and that caused the target
process (that sits at the other end of the ipc) to be killed. Now killing
processes is done via a kernel apc (async) so we should arrive at 91 with the
process dead, but GetExitCodeProcess is telling us that is not, thusly we enter
in panic mode and leak the ipc_server_ because we fear that if the target
somehow manages to do another ipc call we would crash the broker (browser)

A know case of this is if the process is killed while a thread is stuck doing IO
in the kernel, win2k dealt with this poorly as a supposedly dead process has a
thread that emerges from kernel to do damage.

So it is doing the right thing but I am not agreeing with my previous self about
the signals; since WaitForSingleObject could have just timed out and 5ms later
it would have been ok to call GetExitCodeProcess.  Maybe it would be better to
wait for 1500ms and just read the result from the wait, if is a timeout then
take this path.

[Peter Kasting, 2014/09/30 20:35:39]

It sounds like I should basically leave this code as-is, expand the comment, and
let it be up to you as to whether you want to make a further behavior change.

[Peter Kasting, 2014/10/01 02:32:13]

I attempted to clarify the comments here.

+      ipc_server_.release();
       sandbox_process_info_.TakeProcessHandle();
       return;
     }