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

sandbox/win/src/target_process.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sandbox/win/src/target_process.h"
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/win/pe_image.h"
 #include "base/win/startup_information.h"
@@ skipping 77 matching lines @@
   // from showing up in purify.
   if (sandbox_process_info_.IsValid()) {
     ::WaitForSingleObject(sandbox_process_info_.process_handle(), 50);
     if (!::GetExitCodeProcess(sandbox_process_info_.process_handle(),
                               &exit_code) || (STILL_ACTIVE == exit_code)) {
       // It is an error to destroy this object while the target process is still
       // alive because we need to destroy the IPC subsystem and cannot risk to
       // 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;
     }
   }
 
   // ipc_server_ references our process handle, so make sure the former is shut
   // down before the latter is closed (by ScopedProcessInformation).
   ipc_server_.reset();
 }
 
@@ skipping 219 matching lines @@
 TargetProcess* MakeTestTargetProcess(HANDLE process, HMODULE base_address) {
   TargetProcess* target = new TargetProcess(NULL, NULL, NULL, NULL);
   PROCESS_INFORMATION process_info = {};
   process_info.hProcess = process;
   target->sandbox_process_info_.Set(process_info);
   target->base_address_ = base_address;
   return target;
 }
 
 }  // namespace sandbox