Revert 92563 - Had a bug in the handle table unit test. Added GetHandleName to fix the bug and make handle management easier.

TEST=sbox_unittests --gtest_filter=HandleTable.*
BUG=89325
Review URL: http://codereview.chromium.org/7346027

TBR=jschuh@chromium.org
Review URL: http://codereview.chromium.org/7379001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92568 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 23 insertions, 63 deletions

diff --git a/sandbox/src/handle_table.cc b/sandbox/src/handle_table.cc
index ab079ea..c7fcf0a 100644
--- a/sandbox/src/handle_table.cc
+++ b/sandbox/src/handle_table.cc
@@ -17,8 +17,6 @@ bool CompareHandleEntries(const SYSTEM_HANDLE_INFORMATION& a,
   return a.ProcessId < b.ProcessId;
 }
 
-static NtQueryObject QueryObject = NULL;
-
 }  // namespace
 
 namespace sandbox {
@@ -70,11 +68,12 @@ HandleTable::Iterator HandleTable::HandlesForProcess(ULONG process_id) const {
   key.ProcessId = process_id;
 
   const SYSTEM_HANDLE_INFORMATION* start = handle_info()->Information;
-  const SYSTEM_HANDLE_INFORMATION* finish = end();
+  const SYSTEM_HANDLE_INFORMATION* finish =
+      &handle_info()->Information[handle_info()->NumberOfHandles];
 
   start = std::lower_bound(start, finish, key, CompareHandleEntries);
   if (start->ProcessId != process_id)
-    return Iterator(*this, end(), end());
+    return Iterator(*this, finish, finish);
   finish = std::upper_bound(start, finish, key, CompareHandleEntries);
   return Iterator(*this, start, finish);
 }
@@ -85,6 +84,7 @@ HandleTable::HandleEntry::HandleEntry(
 }
 
 void HandleTable::HandleEntry::UpdateInfo(UpdateType flag) {
+  static NtQueryObject QueryObject = NULL;
   if (!QueryObject)
     ResolveNTFunctionPtr("NtQueryObject", &QueryObject);
 
@@ -119,8 +119,18 @@ void HandleTable::HandleEntry::UpdateInfo(UpdateType flag) {
   switch (flag) {
     case UPDATE_INFO_AND_NAME:
       if (type_info_buffer_.size() && handle_name_.empty()) {
-        GetHandleName(reinterpret_cast<HANDLE>(handle_entry_->Handle),
-            &handle_name_);
+        ULONG size = MAX_PATH;
+        scoped_ptr<UNICODE_STRING> name;
+        do {
+          name.reset(reinterpret_cast<UNICODE_STRING*>(new BYTE[size]));
+          result = QueryObject(reinterpret_cast<HANDLE>(
+              handle_entry_->Handle), ObjectNameInformation, name.get(),
+              size, &size);
+        } while (result == STATUS_INFO_LENGTH_MISMATCH);
+
+        if (NT_SUCCESS(result)) {
+          handle_name_.assign(name->Buffer, name->Length / sizeof(wchar_t));
+        }
       }
       break;
 
@@ -134,27 +144,6 @@ void HandleTable::HandleEntry::UpdateInfo(UpdateType flag) {
   }
 }
 
-// Returns the object manager's name associated with a handle
-BOOL GetHandleName(HANDLE handle, string16* handle_name) {
-  if (!QueryObject)
-    ResolveNTFunctionPtr("NtQueryObject", &QueryObject);
-
-  ULONG size = MAX_PATH;
-  scoped_ptr<UNICODE_STRING> name;
-  NTSTATUS result;
-
-  do {
-    name.reset(reinterpret_cast<UNICODE_STRING*>(new BYTE[size]));
-    result = QueryObject(handle, ObjectNameInformation, name.get(),
-                         size, &size);
-  } while (result == STATUS_INFO_LENGTH_MISMATCH);
-
-  if (NT_SUCCESS(result))
-    handle_name->assign(name->Buffer, name->Length / sizeof(wchar_t));
-
-  return NT_SUCCESS(result);
-}
-
 const OBJECT_TYPE_INFORMATION* HandleTable::HandleEntry::TypeInfo() {
   UpdateInfo(UPDATE_INFO_ONLY);
   return type_info_buffer_.empty() ? NULL : type_info_internal();
diff --git a/sandbox/src/handle_table.h b/sandbox/src/handle_table.h
index 4814aab..9b1fc66 100644
--- a/sandbox/src/handle_table.h
+++ b/sandbox/src/handle_table.h
@@ -155,10 +155,6 @@ class HandleTable {
   DISALLOW_COPY_AND_ASSIGN(HandleTable);
 };
 
-// Returns the object manager's name associated with a handle
-BOOL GetHandleName(HANDLE handle, string16* handle_name);
-
-
 }  // namespace sandbox
 
 #endif  // SANDBOX_SRC_HANDLE_TABLE_H_
diff --git a/sandbox/src/handle_table_unittest.cc b/sandbox/src/handle_table_unittest.cc
index 051cad2..696037f 100644
--- a/sandbox/src/handle_table_unittest.cc
+++ b/sandbox/src/handle_table_unittest.cc
@@ -8,21 +8,16 @@
 #include "sandbox/tests/common/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-// Flaky on the Vista bots: crbug.com/89325
-TEST(HandleTable, FLAKY_IsTableValid) {
+TEST(HandleTable, IsTableValid) {
   using sandbox::HandleTable;
   const ULONG my_process_id = ::GetCurrentProcessId();
   ULONG last_process_id = 0;
-  USHORT last_handle = -1;
   ULONG total_handles = 0;
   bool found_my_process = false;
   bool found_other_process = false;
 
   HandleTable handles;
-  // Verify that we have some number of handles.
   EXPECT_NE(0u, handles.handle_info()->NumberOfHandles);
-  // Verify that an invalid process ID returns no handles.
-  EXPECT_TRUE(handles.HandlesForProcess(-1) == handles.end());
 
   for (HandleTable::Iterator it = handles.begin(); it != handles.end(); ++it) {
     ULONG process_id = it->handle_entry()->ProcessId;
@@ -30,14 +25,6 @@ TEST(HandleTable, FLAKY_IsTableValid) {
     found_my_process |= is_current_process;
     found_other_process |= !is_current_process;
 
-    // Duplicate handles in a process mean a corrupt table.
-    if (last_process_id == process_id) {
-      EXPECT_NE(last_handle, it->handle_entry()->Handle);
-      if (last_handle == it->handle_entry()->Handle)
-        return;  // Just bail rather than spew errors.
-    }
-
-    // Verify the sort order.
     EXPECT_LE(last_process_id, process_id);
     last_process_id = process_id;
     total_handles++;
@@ -48,8 +35,7 @@ TEST(HandleTable, FLAKY_IsTableValid) {
   EXPECT_TRUE(found_other_process);
 }
 
-// Flaky on the Vista bots: crbug.com/89325
-TEST(HandleTable, FLAKY_FindHandle) {
+TEST(HandleTable, FindHandle) {
   using sandbox::HandleTable;
 
   // Create a temp file so we have a handle to find.
@@ -61,30 +47,19 @@ TEST(HandleTable, FLAKY_FindHandle) {
                              FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
                              OPEN_EXISTING, FILE_FLAG_DELETE_ON_CLOSE, NULL);
   EXPECT_NE(INVALID_HANDLE_VALUE, file);
-  string16 handle_name;
-  ASSERT_NE(sandbox::GetHandleName(file, &handle_name), FALSE);
 
-  // Look for the handle in our process.
-  int total_handles = 0;
-  int type_handles = 0;
+  // Look for the handle in our process
   bool handle_found = false;
   HandleTable handles;
   for (HandleTable::Iterator it =
       handles.HandlesForProcess(::GetCurrentProcessId());
       it != handles.end(); ++it) {
-    ++total_handles;
-    if (it->IsType(HandleTable::kTypeFile)) {
-      ++type_handles;
-      if (it->Name() == handle_name) {
-        handle_found = true;
-        break;
-      }
+    if (it->IsType(HandleTable::kTypeFile) && it->Name().compare(my_file)) {
+      handle_found = true;
+      break;
     }
   }
-
-  EXPECT_TRUE(handle_found) << "File not found in " << total_handles
-      << " handles (includes " << type_handles << " " << HandleTable::kTypeFile
-      << " handles):\n\t" << handle_name << "\n\t[" << my_file << "]";
+  EXPECT_TRUE(handle_found);
 
   // Clean up the file we made.
   EXPECT_TRUE(::CloseHandle(file));