Add WOW64 support and DeleteEmptyKey to base::win::registry.

base/win/registry_unittest.cc

Index: base/win/registry_unittest.cc
diff --git a/base/win/registry_unittest.cc b/base/win/registry_unittest.cc
index 155402a351fc4593d57f6f8a479c09969d041400..b0d0e7f7a629d8ff9f811cbd372f4dd87676201f 100644
--- a/base/win/registry_unittest.cc
+++ b/base/win/registry_unittest.cc
@@ -9,6 +9,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/stl_util.h"
+#include "base/win/windows_version.h"

[grt (UTC plus 2), 2014/05/16 15:44:43]

unused

[Will Harris, 2014/05/16 22:55:45]

used for OSInfo below

 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -35,6 +36,16 @@ class RegistryTest : public testing::Test {
     // Clean up the temporary key.
     RegKey key(HKEY_CURRENT_USER, L"", KEY_SET_VALUE);
     ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, kRootKey, KEY_READ));
+
+    // Delete all variations of the Software key we created.
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE | KEY_WOW64_32KEY));

[grt (UTC plus 2), 2014/05/16 15:44:43]

indentation. please try "git cl format". it wraps the conditional operator
wrong, though. :-(

[Will Harris, 2014/05/16 22:55:45]

Done.

+    key.DeleteKey(kRootKey);
+
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE | KEY_WOW64_64KEY));
+    key.DeleteKey(kRootKey);
   }
 
  private:
@@ -158,6 +169,150 @@ TEST_F(RegistryTest, TruncatedCharTest) {
   EXPECT_FALSE(iterator.Valid());
 }
 
+TEST_F(RegistryTest, RecursiveDelete) {

[grt (UTC plus 2), 2014/05/16 15:44:43]

DeleteKey was untested before?!??!?

[Will Harris, 2014/05/16 22:55:45]

Yes - scary.  I tested the old SHDeleteKey code with this new test to verify the
new behavior was identical.

+  RegKey key;
+  // Create kRootKey->Foo
+  //                  \->Bar
+  //                     \->Foo
+  //                  \->Moo
+  //                  \->Foo
+  // and delete kRootKey->Foo
+  std::wstring foo_key(kRootKey);
+  foo_key += L"\\Foo";
+  ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, foo_key.c_str(),
+      KEY_WRITE));
+  foo_key += L"\\Bar";
+
+  ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, foo_key.c_str(),
+      KEY_READ));
+  ASSERT_EQ(ERROR_SUCCESS, key.CreateKey(L"Bar", KEY_WRITE));
+  ASSERT_EQ(ERROR_SUCCESS, key.CreateKey(L"Moo", KEY_WRITE));
+  ASSERT_EQ(ERROR_SUCCESS, key.CreateKey(L"Foo", KEY_WRITE));
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, foo_key.c_str(),
+      KEY_WRITE));
+  foo_key += L"\\Foo";
+  ASSERT_EQ(ERROR_SUCCESS, key.CreateKey(L"Foo", KEY_WRITE));
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, foo_key.c_str(),
+      KEY_READ));
+
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, kRootKey, KEY_WRITE));
+  ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(L"Bar"));
+  ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(L"Foo"));
+  ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(L"Foo"));
+  ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, foo_key.c_str(),
+      KEY_READ));
+}
+
+// This test requires running as an Administrator as it tests redirected
+// registry writes to HKLM\Software

[grt (UTC plus 2), 2014/05/16 15:44:43]

can the Administrator requirement be removed by making the test operate in
HKEY_CURRENT_USER\Software\Classes or one of its subkeys that is
redirected/reflected?

[Will Harris, 2014/05/16 22:55:45]

HKLM\Software is the only key that is universally redirected on all versions of
Windows.  HKCU\Software\Classes is reflected below Win7, and so would have
required a load of windows version specific test code.

+// http://msdn.microsoft.com/en-us/library/windows/desktop/aa384253.aspx
+TEST_F(RegistryTest, Wow64) {
+  RegKey key;
+  std::wstring foo_key(L"Software\\");
+  foo_key += kRootKey;
+  foo_key += L"\\Foo";
+  std::wstring foo_wow64_key(L"Software\\Wow6432Node\\");
+  foo_wow64_key += kRootKey;
+  foo_wow64_key += L"\\Foo";
+
+#if defined(_WIN64)
+  // Test 32-bit key access from 64-bit.

[grt (UTC plus 2), 2014/05/16 15:44:43]

it looks like a lot of the _WIN64 part and the non-_WIN64 part are the same
except for some constants. could you split this into a few small TEST functions
that test individual aspects of this? for example, you could do something like:

class RegistryTest {
 protected:
  // Returns true if the current process is influenced by the registry
redirector.
  static bool IsRedirectorPresent() {
#if defined(_WIN64)
    return true;
#else
    return os_info->wow64_status() == OSInfo::WOW64_ENABLED;
#endif
  }
  static REGASM GetOppositeViewMask() {
    DCHECK(IsRedirectoryPresent());
#if defined(_WIN64)
    return KEY_WOW64_32KEY;
#else
    return KEY_WOW64_64KEY;
#endif
  }
};

TEST_F(RegistryTest, AccessOpposite) {
  // early exit
  if (!IsRedirectorPresent())
    return;
  // test accessing either 32-bit from 64-bit, or 64-bit from 32-bit
  // using GetOppositeViewMask()
}

TEST_F(RegistryTest, DeleteOpposite) {
  // early exit
  if (!IsRedirectorPresent())
    return;
  // test deleting either 32-bit from 64-bit, or 64-bit from 32-bit
  // using GetOppositeViewMask()
}

...

[Will Harris, 2014/05/16 22:55:45]

I went ahead and split this test up a bit.

+  ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+      KEY_WRITE | KEY_WOW64_32KEY));
+  ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+      KEY_READ));
+  ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+      KEY_READ | KEY_WOW64_64KEY));
+
+  // Open the 64-bit view of the parent and try to delete the test key.
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE));
+  ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+    KEY_SET_VALUE | KEY_WOW64_64KEY));
+  ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+    KEY_SET_VALUE | KEY_WOW64_32KEY));
+  ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+  // Test 64-bit key access from 64-bit.
+  ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+      KEY_WRITE | KEY_WOW64_64KEY));
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+      KEY_READ));
+  ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+      KEY_READ | KEY_WOW64_32KEY));
+
+  // Open the 32-bit view of the parent and try to delete the test key.
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+    KEY_SET_VALUE | KEY_WOW64_32KEY));
+  ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE | KEY_WOW64_64KEY));
+  ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+#else
+  OSInfo* os_info = OSInfo::GetInstance();
+
+  if (os_info->wow64_status() == OSInfo::WOW64_ENABLED) {
+    // Test 64-bit key access from 32-bit.
+    ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_WRITE | KEY_WOW64_64KEY));
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_READ));
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_READ | KEY_WOW64_32KEY));
+
+    // Open the 32-bit view of the parent and try to delete the test key.
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE));
+    ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE | KEY_WOW64_32KEY));
+    ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+        KEY_SET_VALUE | KEY_WOW64_64KEY));
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+    // Test 32-bit key access from 32-bit.
+    ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_WRITE | KEY_WOW64_32KEY));
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_READ));
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_READ | KEY_WOW64_64KEY));
+
+    // Test accessing the 32-bit values via Wow6432Node.
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE,
+        foo_wow64_key.c_str(), KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE,
+        foo_wow64_key.c_str(), KEY_READ | KEY_WOW64_64KEY));
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE,
+        foo_wow64_key.c_str(), KEY_READ | KEY_WOW64_32KEY));
+
+    // Open the 64-bit view of the parent and try to delete the test key.
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+      KEY_SET_VALUE | KEY_WOW64_64KEY));
+    ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+        KEY_SET_VALUE | KEY_WOW64_32KEY));
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+  } else {
+    ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_LOCAL_MACHINE, foo_key.c_str(),
+        KEY_READ));
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, foo_wow64_key.c_str(),
+        KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, L"Software",
+        KEY_SET_VALUE));
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+  }
+#endif // _WIN64
+}
+
+
 }  // namespace
 
 }  // namespace win