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

base/win/registry.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 "base/win/registry.h"
 
 #include <shlwapi.h>
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_restrictions.h"
-
-#pragma comment(lib, "shlwapi.lib")  // for SHDeleteKey

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

w00t!

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

:D

+#include "base/win/windows_version.h"
 
 namespace base {
 namespace win {
 
 namespace {
 
 // RegEnumValue() reports the number of characters from the name that were
 // written to the buffer, not how many there are. This constant is the maximum
 // name size, such that a buffer with this size should read any name.
 const DWORD MAX_REGISTRY_NAME_SIZE = 16384;
@@ skipping 44 matching lines @@
 LONG RegKey::CreateWithDisposition(HKEY rootkey, const wchar_t* subkey,
                                    DWORD* disposition, REGSAM access) {
   DCHECK(rootkey && subkey && access && disposition);
   HKEY subhkey = NULL;
   LONG result = RegCreateKeyEx(rootkey, subkey, 0, NULL,
                                REG_OPTION_NON_VOLATILE, access, NULL, &subhkey,
                                disposition);
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subhkey;
+    wow64access_ = access & KEY_WOW64_RES;

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

looks like this is an undocumented constant in winnt.h. for safety's sake, wdyt
of making a constant in the unnamed namespace for this:

const REGSAM kWow64AccessMask = KEY_WOW64_32KEY | KEY_WOW64_64KEY;

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

Done.

   }
 
   return result;
 }
 
 LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) {
   DCHECK(name && access);
   HKEY subkey = NULL;
   LONG result = RegCreateKeyEx(key_, name, 0, NULL, REG_OPTION_NON_VOLATILE,
                                access, NULL, &subkey, NULL);
- if (result == ERROR_SUCCESS) {
-   Close();
-
-   key_ = subkey;
+  if (result == ERROR_SUCCESS) {
+    Close();
+    key_ = subkey;
+    wow64access_ = access & KEY_WOW64_RES;
   }
 
   return result;
 }
 
 LONG RegKey::Open(HKEY rootkey, const wchar_t* subkey, REGSAM access) {
   DCHECK(rootkey && subkey && access);
   HKEY subhkey = NULL;
 
   LONG result = RegOpenKeyEx(rootkey, subkey, 0, access, &subhkey);
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subhkey;
+    wow64access_ = access & KEY_WOW64_RES;
   }
 
   return result;
 }
 
 LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) {
   DCHECK(relative_key_name && access);
   HKEY subkey = NULL;
   LONG result = RegOpenKeyEx(key_, relative_key_name, 0, access, &subkey);
 
   // We have to close the current opened key before replacing it with the new
   // one.
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subkey;
+    wow64access_ = access & KEY_WOW64_RES;
   }
   return result;
 }
 
 void RegKey::Close() {
   StopWatching();
   if (key_) {
     ::RegCloseKey(key_);
     key_ = NULL;
   }
@@ skipping 30 matching lines @@
   LONG r = ::RegEnumValue(key_, index, buf, &bufsize, NULL, NULL, NULL, NULL);
   if (r == ERROR_SUCCESS)
     *name = buf;
 
   return r;
 }
 
 LONG RegKey::DeleteKey(const wchar_t* name) {
   DCHECK(key_);
   DCHECK(name);
-  LONG result = SHDeleteKey(key_, name);
-  return result;
+  HKEY subkey;
+
+  // Verify the key exists before attempting delete to replicate previous
+  // behavior.
+  LONG result = RegOpenKeyEx(key_, name, 0, KEY_READ | wow64access_, &subkey);

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

if this is just an existence test, i think READ_CONTROL is the minimal access
needed.

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

Done.

+  if (result == ERROR_SUCCESS) {

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

suggestion:
  if (result != ERROR_SUCCESS)
    return result;
  ...

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

Done.

+    RegCloseKey(subkey);
+    std::wstring keyname(name);

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

new code should use string16. would you mind switching
registry.{h,cc,_unittest.cc} wholesale in a separate change?

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

will do in another CL

+
+    return RegKey::RegDelRecurse(key_, keyname, wow64access_);

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

remove RegKey::

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

Done.

+  } else {
+    return result;
+  }
 }
 
 LONG RegKey::DeleteValue(const wchar_t* value_name) {
   DCHECK(key_);
   LONG result = RegDeleteValue(key_, value_name);
   return result;
 }
 
 LONG RegKey::ReadValueDW(const wchar_t* name, DWORD* out_value) const {
   DCHECK(out_value);
@@ skipping 152 matching lines @@
 LONG RegKey::StopWatching() {
   LONG result = ERROR_INVALID_HANDLE;
   if (watch_event_) {
     CloseHandle(watch_event_);
     watch_event_ = 0;
     result = ERROR_SUCCESS;
   }
   return result;
 }
 
+// static
+LONG RegKey::RegDeleteKeyExWrapper(HKEY hKey,

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

use chromium naming style rather than hungarian

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

Was intentionally using these since it's a wrapper to an MS function.

+                                   const wchar_t* lpSubKey,
+                                   REGSAM samDesired,
+                                   DWORD Reserved) {
+  typedef LSTATUS(WINAPI *RegDeleteKeyExPtr)(
+      HKEY hKey, LPCWSTR lpSubKey, REGSAM samDesired, DWORD Reserved);
+
+  if (base::win::GetVersion() >= VERSION_VISTA) {

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

some versions prior to vista have RegDeleteKeyEx, so i think it's better to
unconditionally try GetProcAddress and fallback to RegDeleteKey any time it
isn't found.

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

Done.  We need to support these Windows XP 64-bit users! :)

+    RegDeleteKeyExPtr reg_delete_key_ex_func =
+        reinterpret_cast<RegDeleteKeyExPtr>(
+            GetProcAddress(GetModuleHandleA("advapi32.dll"),
+                "RegDeleteKeyExW"));
+    if (reg_delete_key_ex_func)
+      return reg_delete_key_ex_func(hKey, lpSubKey, samDesired, Reserved);
+  } else {
+    // Windows XP does not support RegDeleteKeyEx.
+    return RegDeleteKey(hKey, lpSubKey);
+  }
+
+  return ERROR_GEN_FAILURE;
+}
+
+// static

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

this isn't a static method according to the header.

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

Good spot - the static keyword was accidently dropped when I changed the
function prototype.

+LONG RegKey::RegDelRecurse(HKEY root_key, std::wstring name, REGSAM access) {

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

pass name by const ref

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

Done.

+  LONG result;
+  DWORD key_size;
+  wchar_t key_name[MAX_PATH];
+  HKEY hKey;

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

no hungarians allowed. i suggest target_key or something since this is a handle
to the key to be deleted.

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

Done.

+  FILETIME ft;

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

is this needed? msdn says that the last param to EnumKeyEx can be NULL.

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

Done.

+
+  // First, see if the key can be deleted without having to recurse.
+  result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0);
+
+  if (result == ERROR_SUCCESS)
+    return result;
+
+  result = RegOpenKeyEx(root_key, name.c_str(), 0, KEY_READ | access, &hKey);

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

request the least required access:
KEY_READ -> KEY_ENUMERATE_SUB_KEYS

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

Done.

+
+  if (result != ERROR_SUCCESS) {

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

  if (result == ERROR_FILE_NOT_FOUND)
    return ERROR_SUCCESS
  if (result != ERROR_SUCCESS)
    return result;

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

Done.

+    if (result == ERROR_FILE_NOT_FOUND) {
+      return ERROR_SUCCESS;
+    } else {
+      return result;
+    }
+  }
+
+  // Check for an ending slash and add one if it is missing.
+  if (name[name.length() - 1] != L'\\')

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

what if |name| is empty? perhaps the top of the function should have:
  if (name.empty()) {
    NOTREACHED();
    return ERROR_INVALID_PARAMETER;
  }

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

Done.

+    name += L"\\";
+
+  // Enumerate the keys
+  key_size = MAX_PATH;
+  result = RegEnumKeyEx(hKey, 0, key_name, &key_size, NULL, NULL, NULL, &ft);

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

i think cpu already mentioned this, but do something like this to have the key
name on the heap instead of the stack:
  const DWORD kMaxKeyNameLength = MAX_PATH;
  string16 key_name;
  DWORD key_name_length = kMaxKeyNameLength;
  result = RegEnumKeyEx(hKey, 0, WriteInto(&key_name, kMaxKeyNameLength),
                        &key_name_length, NULL, NULL, NULL, NULL);
  if (result == ERROR_SUCCESS)
    key_name.resize(key_name_length);

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

Done.

+
+  if (result == ERROR_SUCCESS) {

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

can you get rid of if { do { with:
  for (; result == ERROR_SUCCESS; ) {

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

Done.

+    do {
+      std::wstring full_name(name);

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

pull full_name's definition out of the loop

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

Done.

+
+      full_name += key_name;
+
+      if (RegDelRecurse(root_key, full_name, access) != ERROR_SUCCESS)
+        break;
+
+      key_size = MAX_PATH;
+      result = RegEnumKeyEx(hKey, 0, key_name, &key_size, NULL,
+          NULL, NULL, &ft);

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

shouldn't this be aligned with hKey above? did "git cl format" format this?

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

Done.

+    } while (result == ERROR_SUCCESS);
+  }
+
+  RegCloseKey(hKey);
+
+  // Try again to delete the key.
+  result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0);
+
+  return result;
+}
+
 // RegistryValueIterator ------------------------------------------------------
 
 RegistryValueIterator::RegistryValueIterator(HKEY root_key,
                                              const wchar_t* folder_key)
     : name_(MAX_PATH, L'\0'),
       value_(MAX_PATH, L'\0') {
   LONG result = RegOpenKeyEx(root_key, folder_key, 0, KEY_READ, &key_);
   if (result != ERROR_SUCCESS) {
     key_ = NULL;
   } else {
@@ skipping 132 matching lines @@
     if (ERROR_SUCCESS == r)
       return true;
   }
 
   name_[0] = '\0';
   return false;
 }
 
 }  // namespace win
 }  // namespace base