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

base/win/registry_unittest.cc

 // Copyright (c) 2011 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 "base/win/registry.h"
 
 #include <cstring>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/stl_util.h"
+#include "base/win/windows_version.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 namespace win {
 
 namespace {
 
 const wchar_t kRootKey[] = L"Base_Registry_Unittest";
 
+#if defined(_WIN64)
+const REGSAM kNativeViewMask = KEY_WOW64_64KEY;

[grt (UTC plus 2), 2014/05/17 19:31:53]

make these static const protected members of the test fixture.
may as well do the same to kRootKey. i can't think of any advantage to having it
hanging out in namespace scope like that.

[Will Harris, 2014/05/19 21:49:03]

Done.

+const REGSAM kRedirectedViewMask = KEY_WOW64_32KEY;
+#else
+const REGSAM kNativeViewMask = KEY_WOW64_32KEY;
+const REGSAM kRedirectedViewMask = KEY_WOW64_64KEY;
+#endif  //  _WIN64
+
 class RegistryTest : public testing::Test {
  public:
-  RegistryTest() {}
+  RegistryTest() {

[grt (UTC plus 2), 2014/05/17 19:31:53]

while you're here, please move this ctor into the protected: section.

[Will Harris, 2014/05/19 21:49:03]

Done.

+    foo_software_key_ = L"Software\\";

[grt (UTC plus 2), 2014/05/17 19:31:53]

may as well do this work in SetUp so that the fixture initialization isn't
divided between the ctor and SetUp methods.

[Will Harris, 2014/05/19 21:49:03]

Done.

+    foo_software_key_ += kRootKey;
+    foo_software_key_ += L"\\Foo";
+    foo_software_wow64_key_ = L"Software\\Wow6432Node\\";
+    foo_software_wow64_key_ += kRootKey;
+    foo_software_wow64_key_ += L"\\Foo";
+  }
 
  protected:
   virtual void SetUp() OVERRIDE {
     // Create a temporary key.
     RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
     key.DeleteKey(kRootKey);
     ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, kRootKey, KEY_READ));
     ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, kRootKey, KEY_READ));
   }
 
   virtual void TearDown() OVERRIDE {
     // 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));
+    key.DeleteKey(kRootKey);

[grt (UTC plus 2), 2014/05/17 19:31:53]

?    EXPECT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));?
below, too?

+
+    ASSERT_EQ(
+        ERROR_SUCCESS,
+        key.Open(
+            HKEY_LOCAL_MACHINE, L"Software", KEY_SET_VALUE | KEY_WOW64_64KEY));
+    key.DeleteKey(kRootKey);
+  }
+
+  std::wstring foo_software_key_;

[grt (UTC plus 2), 2014/05/17 19:31:53]

move these down to the end of the protected: section (non-const data members
come last;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order).

[Will Harris, 2014/05/19 21:49:03]

Done.

+  std::wstring foo_software_wow64_key_;
+
+  static bool IsRedirectorPresent() {
+#if defined(_WIN64)
+    return true;
+#else
+    OSInfo* os_info = OSInfo::GetInstance();
+
+    return os_info->wow64_status() == OSInfo::WOW64_ENABLED;

[grt (UTC plus 2), 2014/05/17 19:31:53]

    return OSInfo::GetInstance()->wow64_status() == OSInfo::WOW64_ENABLED;

[Will Harris, 2014/05/19 21:49:03]

Done.

+#endif
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(RegistryTest);
 };
 
 TEST_F(RegistryTest, ValueTest) {
   RegKey key;
 
   std::wstring foo_key(kRootKey);
@@ skipping 103 matching lines @@
   ASSERT_EQ(arraysize(kData), iterator.ValueSize());
   // Value() is NUL terminated.
   int end = (iterator.ValueSize() + sizeof(wchar_t) - 1) / sizeof(wchar_t);
   EXPECT_NE(L'\0', iterator.Value()[end-1]);
   EXPECT_EQ(L'\0', iterator.Value()[end]);
   EXPECT_EQ(0, std::memcmp(kData, iterator.Value(), arraysize(kData)));
   ++iterator;
   EXPECT_FALSE(iterator.Valid());
 }
 
+TEST_F(RegistryTest, RecursiveDelete) {
+  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
+// http://msdn.microsoft.com/en-us/library/windows/desktop/aa384253.aspx
+TEST_F(RegistryTest, Wow64RedirectedFromNative) {
+  if (IsRedirectorPresent()) {

[grt (UTC plus 2), 2014/05/17 19:31:53]

please have these early-exit in the case where the test doesn't apply. it avoids
having the whole body of the test indented an extra two spaces and makes it more
clear that the whole test is skipped when there's no redirection.

[Will Harris, 2014/05/19 21:49:03]

Done.

+    RegKey key;
+
+    // Test redirected key access from non-redirected.
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(HKEY_LOCAL_MACHINE,
+                         foo_software_key_.c_str(),
+                         KEY_WRITE | kRedirectedViewMask));
+    ASSERT_NE(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_key_.c_str(),
+                       KEY_READ));
+    ASSERT_NE(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_key_.c_str(),
+                       KEY_READ | kNativeViewMask));
+
+    // Open the non-redirected 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 | kNativeViewMask));
+    ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+

[grt (UTC plus 2), 2014/05/17 19:31:53]

    // Open the redirected view and delete the key created above.

[Will Harris, 2014/05/19 21:49:03]

Done.

+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       L"Software",
+                       KEY_SET_VALUE | kRedirectedViewMask));
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+  }
+}
+
+TEST_F(RegistryTest, Wow64NativeFromRedirected) {
+  if (IsRedirectorPresent()) {
+    RegKey key;
+
+    // Test non-redirected key access from redirected.
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(HKEY_LOCAL_MACHINE,
+                         foo_software_key_.c_str(),
+                         KEY_WRITE | kNativeViewMask));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_key_.c_str(),
+                       KEY_READ));
+    ASSERT_NE(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_key_.c_str(),
+                       KEY_READ | kRedirectedViewMask));
+
+    // Open the redirected view of the parent and try to delete the test key
+    // from the non-redirected view.
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       L"Software",
+                       KEY_SET_VALUE | kRedirectedViewMask));
+    ASSERT_NE(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       L"Software",
+                       KEY_SET_VALUE | kNativeViewMask));
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(kRootKey));
+  }
+}
+
+TEST_F(RegistryTest, Wow6432NodeFromRedirected) {
+  if (IsRedirectorPresent()) {
+    RegKey key;
+    // Test access to 32-bit values on 64-bit via the Wow6432Node key.

[grt (UTC plus 2), 2014/05/17 19:31:53]

is this worth testing? i thought Wow6432Node was an implementation detail and
that apps aren't supposed to try to use it directly.

[Will Harris, 2014/05/19 21:49:03]

earlier tests just confirm that when KEY_WOW64_32KEY is passed, the keys go into
a different place in the registry, but don't actually test that they go into the
right place in the registry...  I think this test is still worth having.

+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(HKEY_LOCAL_MACHINE,
+                         foo_software_key_.c_str(),
+                         KEY_WRITE | KEY_WOW64_32KEY));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_wow64_key_.c_str(),
+                       KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_wow64_key_.c_str(),
+                       KEY_READ | KEY_WOW64_64KEY));
+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_wow64_key_.c_str(),
+                       KEY_READ | KEY_WOW64_32KEY));
+    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_F(RegistryTest, NativeNotRedirected) {
+  if (!IsRedirectorPresent()) {
+    RegKey key;
+    // 32-bit on 32-bit shouldn't even see a Wow6432Node key.

[grt (UTC plus 2), 2014/05/17 19:31:53]

this also seems to be testing an implementation detail if windows rather than
correct behavior of RegKey

[Will Harris, 2014/05/19 21:49:03]

Happy for this to be removed.

+    ASSERT_EQ(ERROR_SUCCESS,
+              key.Create(HKEY_LOCAL_MACHINE,
+                         foo_software_key_.c_str(),
+                         KEY_READ));
+    ASSERT_NE(ERROR_SUCCESS,
+              key.Open(HKEY_LOCAL_MACHINE,
+                       foo_software_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));
+  }
+}
+
 }  // namespace
 
 }  // namespace win
 }  // namespace base