Delete Chrome state in registry on uninstall when the user profile is being deleted.

chrome/installer/setup/setup_util.cc

Index: chrome/installer/setup/setup_util.cc
diff --git a/chrome/installer/setup/setup_util.cc b/chrome/installer/setup/setup_util.cc
index 0565488cc33bd1ba9913fbf74dc578d8ea2dec38..af474401c9709d9dbd29faca40c870ee59937a4a 100644
--- a/chrome/installer/setup/setup_util.cc
+++ b/chrome/installer/setup/setup_util.cc
@@ -8,12 +8,17 @@
 
 #include <windows.h>
 
+#include <algorithm>
+#include <iterator>
+#include <set>
+
 #include "base/command_line.h"
 #include "base/cpu.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/process/kill.h"
 #include "base/process/launch.h"
 #include "base/process/process_handle.h"
@@ -478,6 +483,152 @@ base::string16 GetRegistrationDataCommandKey(
   return cmd_key;
 }
 
+void DeleteRegistryKeyPartial(
+    HKEY root,
+    const base::string16& path,
+    const std::vector<base::string16>& keys_to_preserve) {
+  // Downcase the list of keys to preserve (all must be ASCII strings).
+  std::set<base::string16> lowered_keys_to_preserve;
+  std::transform(
+      keys_to_preserve.begin(), keys_to_preserve.end(),
+      std::inserter(lowered_keys_to_preserve, lowered_keys_to_preserve.begin()),
+      [](const base::string16& str) {
+        DCHECK(!str.empty());
+        DCHECK(base::IsStringASCII(str));
+        return base::ToLowerASCII(str);
+      });
+  base::win::RegKey key;
+  LONG result = key.Open(root, path.c_str(), (KEY_ENUMERATE_SUB_KEYS |
+                                              KEY_QUERY_VALUE | KEY_SET_VALUE));
+  if (result != ERROR_SUCCESS) {
+    LOG_IF(ERROR, result != ERROR_FILE_NOT_FOUND) << "Failed to open " << path
+                                                  << "; result = " << result;
+    return;
+  }
+
+  // Repeatedly iterate over all subkeys deleting those that should not be
+  // preserved until only those remain. Multiple passes are needed since
+  // deleting one key may change the enumeration order of all remaining keys.
+
+  // Subkeys or values to be skipped on subsequent passes.
+  std::set<base::string16> to_skip;
+  DWORD index = 0;
+  const size_t kMaxKeyNameLength = 256;  // MSDN says 255; +1 for terminator.
+  base::string16 name(kMaxKeyNameLength, base::char16());
+  bool did_delete = false;  // True if at least one item was deleted.
+  while (true) {
+    DWORD name_length = base::saturated_cast<DWORD>(name.capacity());
+    name.resize(name_length);
+    result = ::RegEnumKeyEx(key.Handle(), index, &name[0], &name_length,
+                            nullptr, nullptr, nullptr, nullptr);

[robertshield, 2015/09/03 20:23:33]

This may work, but the thing that gives me pause is this snippet from MSDN:

"While an application is using the RegEnumKeyEx function, it should not make
calls to any registration functions that might change the key being enumerated."
(source: https://msdn.microsoft.com/en-us/library/windows/desktop/ms724862)

Would it work to RegEnumKeyEx to build a snapshot of the registry sub tree under
|key|, then build up a list of things to delete, then apply that?

This would race against concurrent modifications, but given that this is (I
infer) best effort clean up code I think that should be ok. 

[grt (UTC plus 2), 2015/09/04 02:22:47]

That sentence is saying "hey, the enumeration order isn't stable, so if you
add/delete a subkey, the indexing will be all wacko!" There's no state
associated with EnumKeyEx aside from the index, so there is no possibility for
any strange behavior if you don't have any expectations about the ordering of
subkeys after adding/removing them.

As you point out, MSDN is misleading: even if you don't add/remove while
enumerating, another proc could, so you really have no hope of making a single
pass (let along two) work 100% of the time.

The approach I took here does multiple iterations until the desired end state is
reached: all subkeys are removed except for the ones we really want to preserve
and the ones we fail to delete.
If you push me to switch to that, I will. Otherwise, what I have here is more
efficient and more correct. Simplifying it would sacrifice both of those.

+    if (result == ERROR_MORE_DATA) {
+      // Unexpected, but perhaps the max key name length was raised. MSDN
+      // doesn't clearly say that name_length will contain the necessary
+      // length in this case, so double the buffer and try again.
+      name.reserve(name.capacity() * 2);
+      continue;
+    }
+    if (result == ERROR_NO_MORE_ITEMS) {
+      if (!did_delete)
+        break;  // All subkeys were deleted. The job is done.
+      // Otherwise, loop again.
+      did_delete = false;
+      index = 0;
+      continue;
+    }
+    if (result != ERROR_SUCCESS)
+      break;
+    // Shrink the string to the actual length of the name.
+    name.resize(name_length);
+
+    // Skip over this key if it couldn't be deleted on a previous iteration.
+    if (to_skip.count(name)) {
+      ++index;
+      continue;
+    }
+
+    // Skip over this key if it is one of the keys to preserve.
+    if (base::IsStringASCII(name) &&
+        lowered_keys_to_preserve.count(base::ToLowerASCII(name))) {
+      // Add the true name of the key to the list of keys to skip for subsequent
+      // iterations.
+      to_skip.insert(name);
+      ++index;
+      continue;
+    }
+
+    // Delete this key.
+    result = key.DeleteKey(name.c_str());
+    if (result != ERROR_SUCCESS) {
+      LOG(ERROR) << "Failed to delete subkey " << name << " under path "
+                 << path;
+      // Skip over this key on subsequent iterations.
+      to_skip.insert(name);

[robertshield, 2015/09/03 20:23:33]

Hrmm, needing to preserve a list of items to skip to account for errors makes me
think that the snapshot-then-remove approach will be easier to reason about. 

+      ++index;
+      continue;
+    }
+    did_delete = true;
+  }
+
+  // Delete the key if it no longer has any subkeys.
+  if (to_skip.empty()) {

[robertshield, 2015/09/03 20:23:34]

doesn't DeleteEmptyKey already check whether the key is empty?

[grt (UTC plus 2), 2015/09/04 02:22:48]

Yes, though there's no need to call it if you know that the key isn't empty.

+    result = key.DeleteEmptyKey(L"");
+    LOG_IF(ERROR, result != ERROR_SUCCESS) << "Failed to delete empty key "
+                                           << path << "; result: " << result;
+    return;
+  }
+
+  // Delete all values since subkeys are being preserved.
+  to_skip.clear();
+  did_delete = false;
+  index = 0;
+  while (true) {
+    DWORD name_length = base::saturated_cast<int16_t>(name.capacity());
+    name.resize(name_length);
+    result = ::RegEnumValue(key.Handle(), index, &name[0], &name_length,
+                            nullptr, nullptr, nullptr, nullptr);
+    if (result == ERROR_MORE_DATA) {
+      if (name_length <
+          static_cast<DWORD>(std::numeric_limits<int16_t>::max())) {
+        // Double the space to hold the value name and try again.
+        name.reserve(name.capacity() * 2);
+        continue;
+      }
+      // Otherwise, the max has been exceeded. Nothing more to be done.
+      break;
+    }
+    if (result == ERROR_NO_MORE_ITEMS) {
+      if (!did_delete)
+        break;  // All values were deleted. The job is done.
+      // Otherwise, loop again.
+      did_delete = false;
+      index = 0;
+      continue;
+    }
+    if (result != ERROR_SUCCESS)
+      break;
+    // Shrink the string to the actual length of the name.
+    name.resize(name_length);
+
+    // Skip over this value if it couldn't be deleted on a previous iteration.
+    if (to_skip.count(name)) {
+      ++index;
+      continue;
+    }
+
+    // Delete this value.
+    result = key.DeleteValue(name.c_str());
+    if (result != ERROR_SUCCESS) {
+      LOG(ERROR) << "Failed to delete value " << name << " in key " << path;
+      // Skip over this value on subsequent iterations.
+      to_skip.insert(name);
+      ++index;
+      continue;
+    }
+    did_delete = true;
+  }
+}
+
 ScopedTokenPrivilege::ScopedTokenPrivilege(const wchar_t* privilege_name)
     : is_enabled_(false) {
   HANDLE temp_handle;