Check for CloseHandle failures even when not debugging

Bug 524267 was a handle closing failure that only showed up when running
renderer processes under a debugger at startup, so it was not discovered
for a while.

Similarly, bug 520305 is a long-standing base_unittests bug that only
shows up under a debugger.

This change fixes 520305 and checks for many handle closing failures
always so that subsequent bugs of this type will be detected
immediately.

Most of the CloseHandle calls in base are now checked.

This replaces the uncommitted https://codereview.chromium.org/1343873002/

R=grt@chromium.org,rvargas@chromium.org,dalecurtis@chromium.org
BUG=524267,520305

Review URL: https://codereview.chromium.org/1350493002

Cr-Commit-Position: refs/heads/master@{#349333}

base/files/file_util_win.cc

@@ -231,15 +231,11 @@ bool PathExists(const FilePath& path) {
 
 bool PathIsWritable(const FilePath& path) {
   ThreadRestrictions::AssertIOAllowed();
-  HANDLE dir =
+  win::ScopedHandle dir(
       CreateFile(path.value().c_str(), FILE_ADD_FILE, kFileShareAll,
-                 NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
+                 NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL));
 
-  if (dir == INVALID_HANDLE_VALUE)
-    return false;
-
-  CloseHandle(dir);
-  return true;
+  return dir.IsValid();
 }
 
 bool DirectoryExists(const FilePath& path) {

base/memory/shared_memory_win.cc

@@ -85,7 +85,8 @@ SharedMemoryHandle SharedMemory::NULLHandle() {
 // static
 void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
   DCHECK(handle != NULL);
-  ::CloseHandle(handle);
+  const BOOL result = ::CloseHandle(handle);
+  CHECK(result);
 }
 
 // static

base/process/kill_win.cc

@@ -159,11 +159,8 @@ bool WaitForProcessesToExit(const FilePath::StringType& executable_name,
     DWORD remaining_wait = static_cast<DWORD>(std::max(
         static_cast<int64>(0),
         wait.InMilliseconds() - (GetTickCount() - start_time)));
-    HANDLE process = OpenProcess(SYNCHRONIZE,
-                                 FALSE,
-                                 entry->th32ProcessID);
-    DWORD wait_result = WaitForSingleObject(process, remaining_wait);
-    CloseHandle(process);
+    Process process(Process::OpenWithAccess(entry->th32ProcessID, SYNCHRONIZE));
+    DWORD wait_result = WaitForSingleObject(process.Handle(), remaining_wait);
     result &= (wait_result == WAIT_OBJECT_0);
   }
 

base/sync_socket_unittest.cc

@@ -89,34 +89,16 @@ void NormalSendReceivePeek() {
   SendReceivePeek(&socket_a, &socket_b);
 }
 
-template <class SocketType>
-void ClonedSendReceivePeek() {
-  SocketType socket_a, socket_b;
-  ASSERT_TRUE(SocketType::CreatePair(&socket_a, &socket_b));
-
-  // Create new SyncSockets from the paired handles.
-  SocketType socket_c(socket_a.handle()), socket_d(socket_b.handle());
-  SendReceivePeek(&socket_c, &socket_d);
-}
-
 }  // namespace
 
 TEST(SyncSocket, NormalSendReceivePeek) {
   NormalSendReceivePeek<base::SyncSocket>();
 }
 
-TEST(SyncSocket, ClonedSendReceivePeek) {
-  ClonedSendReceivePeek<base::SyncSocket>();
-}
-
 TEST(CancelableSyncSocket, NormalSendReceivePeek) {
   NormalSendReceivePeek<base::CancelableSyncSocket>();
 }
 
-TEST(CancelableSyncSocket, ClonedSendReceivePeek) {
-  ClonedSendReceivePeek<base::CancelableSyncSocket>();
-}
-
 TEST(CancelableSyncSocket, CancelReceiveShutdown) {
 // TODO(ellyjones): This test fails on iOS 7 devices. http://crbug.com/523296
 #if defined(OS_IOS) && !TARGET_IPHONE_SIMULATOR

base/sync_socket_win.cc

@@ -235,7 +235,8 @@ bool SyncSocket::Close() {
   if (handle_ == kInvalidHandle)
     return true;
 
-  const BOOL result = CloseHandle(handle_);
+  const BOOL result = ::CloseHandle(handle_);
+  CHECK(result);
   handle_ = kInvalidHandle;
   return result == TRUE;
 }

base/threading/platform_thread_win.cc

@@ -114,17 +114,15 @@ bool CreateThreadInternal(size_t stack_size,
   // have to work running on CreateThread() threads anyway, since we run code
   // on the Windows thread pool, etc.  For some background on the difference:
   //   http://www.microsoft.com/msj/1099/win32/win321099.aspx
-  void* thread_handle =
-      ::CreateThread(nullptr, stack_size, ThreadFunc, params, flags, nullptr);
-  if (!thread_handle) {
+  base::win::ScopedHandle thread_handle(
+      ::CreateThread(nullptr, stack_size, ThreadFunc, params, flags, nullptr));
+  if (!thread_handle.IsValid()) {
     delete params;
     return false;
   }
 
   if (out_thread_handle)
-    *out_thread_handle = PlatformThreadHandle(thread_handle);
-  else
-    CloseHandle(thread_handle);
+    *out_thread_handle = PlatformThreadHandle(thread_handle.Take());
   return true;
 }
 

base/time/time_win_unittest.cc

@@ -12,6 +12,7 @@
 
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
+#include "base/win/scoped_handle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::Time;
@@ -86,30 +87,29 @@ TEST(TimeTicks, WinRollover) {
     // Setup
     MockTimeTicks::InstallTicker();
     g_rollover_test_start = CreateEvent(0, TRUE, FALSE, 0);
-    HANDLE threads[kThreads];
+    // Since using _beginthreadex() (as opposed to _beginthread), make sure
+    // CloseHandle is called.
+    base::win::ScopedHandle threads[kThreads];
 
     for (int index = 0; index < kThreads; index++) {
       void* argument = reinterpret_cast<void*>(kChecks);
       unsigned thread_id;
-      threads[index] = reinterpret_cast<HANDLE>(
+      threads[index].Set(reinterpret_cast<HANDLE>(
         _beginthreadex(NULL, 0, RolloverTestThreadMain, argument, 0,
-          &thread_id));
-      EXPECT_NE((HANDLE)NULL, threads[index]);
+          &thread_id)));
+      EXPECT_EQ(true, threads[index].IsValid());
     }
 
     // Start!
     SetEvent(g_rollover_test_start);
 
     // Wait for threads to finish
     for (int index = 0; index < kThreads; index++) {
-      DWORD rv = WaitForSingleObject(threads[index], INFINITE);
+      DWORD rv = WaitForSingleObject(threads[index].Get(), INFINITE);
       EXPECT_EQ(rv, WAIT_OBJECT_0);
-      // Since using _beginthreadex() (as opposed to _beginthread),
-      // an explicit CloseHandle() is supposed to be called.
-      CloseHandle(threads[index]);
     }
 
-    CloseHandle(g_rollover_test_start);
+    CHECK(::CloseHandle(g_rollover_test_start));
 
     // Teardown
     MockTimeTicks::UninstallTicker();

base/win/object_watcher_unittest.cc

@@ -39,20 +39,19 @@ void RunTest_BasicSignal(MessageLoop::Type message_loop_type) {
   EXPECT_FALSE(watcher.IsWatching());
 
   // A manual-reset event that is not yet signaled.
-  HANDLE event = CreateEvent(NULL, TRUE, FALSE, NULL);
+  base::win::ScopedHandle event(CreateEvent(NULL, TRUE, FALSE, NULL));
 
   QuitDelegate delegate;
-  bool ok = watcher.StartWatching(event, &delegate);
+  bool ok = watcher.StartWatching(event.Get(), &delegate);
   EXPECT_TRUE(ok);
   EXPECT_TRUE(watcher.IsWatching());
-  EXPECT_EQ(event, watcher.GetWatchedObject());
+  EXPECT_EQ(event.Get(), watcher.GetWatchedObject());
 
-  SetEvent(event);
+  SetEvent(event.Get());
 
   MessageLoop::current()->Run();
 
   EXPECT_FALSE(watcher.IsWatching());
-  CloseHandle(event);
 }
 
 void RunTest_BasicCancel(MessageLoop::Type message_loop_type) {
@@ -61,15 +60,13 @@ void RunTest_BasicCancel(MessageLoop::Type message_loop_type) {
   ObjectWatcher watcher;
 
   // A manual-reset event that is not yet signaled.
-  HANDLE event = CreateEvent(NULL, TRUE, FALSE, NULL);
+  base::win::ScopedHandle event(CreateEvent(NULL, TRUE, FALSE, NULL));
 
   QuitDelegate delegate;
-  bool ok = watcher.StartWatching(event, &delegate);
+  bool ok = watcher.StartWatching(event.Get(), &delegate);
   EXPECT_TRUE(ok);
 
   watcher.StopWatching();
-
-  CloseHandle(event);
 }
 
 void RunTest_CancelAfterSet(MessageLoop::Type message_loop_type) {
@@ -81,12 +78,12 @@ void RunTest_CancelAfterSet(MessageLoop::Type message_loop_type) {
   DecrementCountDelegate delegate(&counter);
 
   // A manual-reset event that is not yet signaled.
-  HANDLE event = CreateEvent(NULL, TRUE, FALSE, NULL);
+  base::win::ScopedHandle event(CreateEvent(NULL, TRUE, FALSE, NULL));
 
-  bool ok = watcher.StartWatching(event, &delegate);
+  bool ok = watcher.StartWatching(event.Get(), &delegate);
   EXPECT_TRUE(ok);
 
-  SetEvent(event);
+  SetEvent(event.Get());
 
   // Let the background thread do its business
   Sleep(30);
@@ -97,8 +94,6 @@ void RunTest_CancelAfterSet(MessageLoop::Type message_loop_type) {
 
   // Our delegate should not have fired.
   EXPECT_EQ(1, counter);
-
-  CloseHandle(event);
 }
 
 void RunTest_SignalBeforeWatch(MessageLoop::Type message_loop_type) {
@@ -107,33 +102,32 @@ void RunTest_SignalBeforeWatch(MessageLoop::Type message_loop_type) {
   ObjectWatcher watcher;
 
   // A manual-reset event that is signaled before we begin watching.
-  HANDLE event = CreateEvent(NULL, TRUE, TRUE, NULL);
+  base::win::ScopedHandle event(CreateEvent(NULL, TRUE, TRUE, NULL));
 
   QuitDelegate delegate;
-  bool ok = watcher.StartWatching(event, &delegate);
+  bool ok = watcher.StartWatching(event.Get(), &delegate);
   EXPECT_TRUE(ok);
 
   MessageLoop::current()->Run();
 
   EXPECT_FALSE(watcher.IsWatching());
-  CloseHandle(event);
 }
 
 void RunTest_OutlivesMessageLoop(MessageLoop::Type message_loop_type) {
   // Simulate a MessageLoop that dies before an ObjectWatcher.  This ordinarily
   // doesn't happen when people use the Thread class, but it can happen when
   // people use the Singleton pattern or atexit.
-  HANDLE event = CreateEvent(NULL, TRUE, FALSE, NULL);  // not signaled
+  // Note that |event| is not signaled
+  base::win::ScopedHandle event(CreateEvent(NULL, TRUE, FALSE, NULL));
   {
     ObjectWatcher watcher;
     {
       MessageLoop message_loop(message_loop_type);
 
       QuitDelegate delegate;
-      watcher.StartWatching(event, &delegate);
+      watcher.StartWatching(event.Get(), &delegate);
     }
   }
-  CloseHandle(event);
 }
 
 }  // namespace