Add rollback support to DeleteRegKeyWorkItem.

BUG=none
TEST=new test added to installer_util_unittests.exe

Review URL: http://codereview.chromium.org/6598065

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

5 files changed, 494 insertions, 19 deletions

diff --git a/chrome/chrome_installer.gypi b/chrome/chrome_installer.gypi
index c366613..d566ef9 100644
--- a/chrome/chrome_installer.gypi
+++ b/chrome/chrome_installer.gypi
@@ -83,6 +83,7 @@
             'installer/util/create_dir_work_item_unittest.cc',
             'installer/util/create_reg_key_work_item_unittest.cc',
             'installer/util/delete_after_reboot_helper_unittest.cc',
+            'installer/util/delete_reg_key_work_item_unittest.cc',
             'installer/util/delete_reg_value_work_item_unittest.cc',
             'installer/util/delete_tree_work_item_unittest.cc',
             'installer/util/google_chrome_distribution_unittest.cc',
diff --git a/chrome/installer/util/delete_reg_key_work_item.cc b/chrome/installer/util/delete_reg_key_work_item.cc
index 6c3096d..b044f44 100644
--- a/chrome/installer/util/delete_reg_key_work_item.cc
+++ b/chrome/installer/util/delete_reg_key_work_item.cc
@@ -2,13 +2,267 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <shlwapi.h>
-
 #include "chrome/installer/util/delete_reg_key_work_item.h"
 
+#include <shlwapi.h>
+#include <algorithm>
+#include <limits>
+#include <vector>
+
 #include "base/logging.h"
+#include "base/rand_util.h"
+#include "base/stringprintf.h"
+#include "base/win/registry.h"
 #include "chrome/installer/util/logging_installer.h"
 
+using base::win::RegKey;
+
+namespace {
+const REGSAM kKeyReadNoNotify = (KEY_READ) & ~(KEY_NOTIFY);
+}
+
+// A container for a registry key, its values, and its subkeys.  We don't use
+// more obvious methods for various reasons:
+// - RegCopyTree isn't supported pre-Vista, so we'd have to do something
+//   different for XP anyway.
+// - SHCopyKey can't copy subkeys into a volatile destination, so we'd have to
+//   worry about polluting the registry.
+// We don't persist security attributes since we only delete keys that we own,
+// and we don't set custom attributes on them anyway.
+class DeleteRegKeyWorkItem::RegKeyBackup {
+ public:
+  RegKeyBackup();
+  bool Initialize(const RegKey& key);
+  bool WriteTo(RegKey* key) const;
+
+ private:
+  // A container for a registry value.
+  class RegValueBackup {
+   public:
+    RegValueBackup();
+    void Initialize(const wchar_t* name_buffer, DWORD name_size,
+                    DWORD type, const uint8* data, DWORD data_size);
+    const std::wstring& name_str() const { return name_; }
+    const wchar_t* name() const { return name_.empty() ? NULL : name_.c_str(); }
+    DWORD type() const { return type_; }
+    const uint8* data() const { return data_.empty() ? NULL : &data_[0]; }
+    DWORD data_len() const { return static_cast<DWORD>(data_.size()); }
+
+   private:
+    std::wstring name_;
+    std::vector<uint8> data_;
+    DWORD type_;
+
+    DISALLOW_COPY_AND_ASSIGN(RegValueBackup);
+  };
+
+  scoped_array<RegValueBackup> values_;
+  scoped_array<std::wstring> subkey_names_;
+  scoped_array<RegKeyBackup> subkeys_;
+  ptrdiff_t num_values_;
+  ptrdiff_t num_subkeys_;
+
+  DISALLOW_COPY_AND_ASSIGN(RegKeyBackup);
+};
+
+DeleteRegKeyWorkItem::RegKeyBackup::RegValueBackup::RegValueBackup()
+    : type_(REG_NONE) {
+}
+
+void DeleteRegKeyWorkItem::RegKeyBackup::RegValueBackup::Initialize(
+    const wchar_t* name_buffer,
+    DWORD name_size,
+    DWORD type, const uint8* data,
+    DWORD data_size) {
+  name_.assign(name_buffer, name_size);
+  type_ = type;
+  data_.assign(data, data + data_size);
+}
+
+DeleteRegKeyWorkItem::RegKeyBackup::RegKeyBackup()
+    : num_values_(0),
+      num_subkeys_(0) {
+}
+
+// Initializes this object by reading the values and subkeys of |key|.
+// Security descriptors are not backed up.
+bool DeleteRegKeyWorkItem::RegKeyBackup::Initialize(const RegKey& key) {
+  DCHECK(key.Valid());
+
+  scoped_array<RegValueBackup> values;
+  scoped_array<std::wstring> subkey_names;
+  scoped_array<RegKeyBackup> subkeys;
+
+  DWORD num_subkeys = 0;
+  DWORD max_subkey_name_len = 0;
+  DWORD num_values = 0;
+  DWORD max_value_name_len = 0;
+  DWORD max_value_len = 0;
+  LONG result = RegQueryInfoKey(key.Handle(), NULL, NULL, NULL,
+                                &num_subkeys, &max_subkey_name_len, NULL,
+                                &num_values, &max_value_name_len,
+                                &max_value_len, NULL, NULL);
+  if (result != ERROR_SUCCESS) {
+    LOG(ERROR) << "Failed getting info of key to backup, result: " << result;
+    return false;
+  }
+  if (max_subkey_name_len >= std::numeric_limits<DWORD>::max() - 1 ||
+      max_value_name_len >= std::numeric_limits<DWORD>::max() - 1) {
+    LOG(ERROR)
+        << "Failed backing up key; subkeys and/or names are out of range.";
+    return false;
+  }
+  DWORD max_name_len = std::max(max_subkey_name_len, max_value_name_len) + 1;
+  scoped_array<wchar_t> name_buffer(new wchar_t[max_name_len]);
+
+  // Backup the values.
+  if (num_values != 0) {
+    values.reset(new RegValueBackup[num_values]);
+    scoped_array<uint8> value_buffer(new uint8[max_value_len]);
+    DWORD name_size = 0;
+    DWORD value_type = REG_NONE;
+    DWORD value_size = 0;
+
+    for (DWORD i = 0; i < num_values; ) {
+      name_size = max_name_len;
+      value_size = max_value_len;
+      result = RegEnumValue(key.Handle(), i, name_buffer.get(), &name_size,
+                            NULL, &value_type, value_buffer.get(), &value_size);
+      switch (result) {
+        case ERROR_NO_MORE_ITEMS:
+          num_values = i;
+          break;
+        case ERROR_SUCCESS:
+          values[i].Initialize(name_buffer.get(), name_size, value_type,
+                               value_buffer.get(), value_size);
+          ++i;
+          break;
+        case ERROR_MORE_DATA:
+          if (value_size > max_value_len) {
+            max_value_len = value_size;
+            value_buffer.reset(new uint8[max_value_len]);
+          } else {
+            DCHECK(max_name_len - 1 < name_size);
+            if (name_size >= std::numeric_limits<DWORD>::max() - 1) {
+              LOG(ERROR) << "Failed backing up key; value name out of range.";
+              return false;
+            }
+            max_name_len = name_size + 1;
+            name_buffer.reset(new wchar_t[max_name_len]);
+          }
+          break;
+        default:
+          LOG(ERROR) << "Failed backing up value " << i << ", result: "
+                     << result;
+          return false;
+      }
+    }
+    DLOG_IF(WARNING, RegEnumValue(key.Handle(), num_values, name_buffer.get(),
+                                  &name_size, NULL, &value_type, NULL,
+                                  NULL) != ERROR_NO_MORE_ITEMS)
+        << "Concurrent modifications to registry key during backup operation.";
+  }
+
+  // Backup the subkeys.
+  if (num_subkeys != 0) {
+    subkey_names.reset(new std::wstring[num_subkeys]);
+    subkeys.reset(new RegKeyBackup[num_subkeys]);
+    DWORD name_size = 0;
+
+    // Get the names of them.
+    for (DWORD i = 0; i < num_subkeys; ) {
+      name_size = max_name_len;
+      result = RegEnumKeyEx(key.Handle(), i, name_buffer.get(), &name_size,
+                            NULL, NULL, NULL, NULL);
+      switch (result) {
+        case ERROR_NO_MORE_ITEMS:
+          num_subkeys = i;
+          break;
+        case ERROR_SUCCESS:
+          subkey_names[i].assign(name_buffer.get(), name_size);
+          ++i;
+          break;
+        case ERROR_MORE_DATA:
+          if (name_size >= std::numeric_limits<DWORD>::max() - 1) {
+            LOG(ERROR) << "Failed backing up key; subkey name out of range.";
+            return false;
+          }
+          max_name_len = name_size + 1;
+          name_buffer.reset(new wchar_t[max_name_len]);
+          break;
+        default:
+          LOG(ERROR) << "Failed getting name of subkey " << i
+                     << " for backup, result: " << result;
+          return false;
+      }
+    }
+    DLOG_IF(WARNING,
+            RegEnumKeyEx(key.Handle(), num_subkeys, NULL, &name_size, NULL,
+                         NULL, NULL, NULL) != ERROR_NO_MORE_ITEMS)
+        << "Concurrent modifications to registry key during backup operation.";
+
+    // Get their values.
+    RegKey subkey;
+    for (DWORD i = 0; i < num_subkeys; ++i) {
+      result = subkey.Open(key.Handle(), subkey_names[i].c_str(),
+                           kKeyReadNoNotify);
+      if (result != ERROR_SUCCESS) {
+        LOG(ERROR) << "Failed opening subkey \"" << subkey_names[i]
+                   << "\" for backup, result: " << result;
+        return false;
+      }
+      if (!subkeys[i].Initialize(subkey)) {
+        LOG(ERROR) << "Failed backing up subkey \"" << subkey_names[i] << "\"";
+        return false;
+      }
+    }
+  }
+
+  values_.swap(values);
+  subkey_names_.swap(subkey_names);
+  subkeys_.swap(subkeys);
+  num_values_ = num_values;
+  num_subkeys_ = num_subkeys;
+
+  return true;
+}
+
+// Writes the values and subkeys of this object into |key|.
+bool DeleteRegKeyWorkItem::RegKeyBackup::WriteTo(RegKey* key) const {
+  LONG result = ERROR_SUCCESS;
+
+  // Write the values.
+  for (int i = 0; i < num_values_; ++i) {
+    const RegValueBackup& value = values_[i];
+    result = RegSetValueEx(key->Handle(), value.name(), 0, value.type(),
+                           value.data(), value.data_len());
+    if (result != ERROR_SUCCESS) {
+      LOG(ERROR) << "Failed writing value \"" << value.name_str()
+                 << "\", result: " << result;
+      return false;
+    }
+  }
+
+  // Write the subkeys.
+  RegKey subkey;
+  for (int i = 0; i < num_subkeys_; ++i) {
+    const std::wstring& name = subkey_names_[i];
+
+    result = subkey.Create(key->Handle(), name.c_str(), KEY_WRITE);
+    if (result != ERROR_SUCCESS) {
+      LOG(ERROR) << "Failed creating subkey \"" << name << "\", result: "
+                 << result;
+      return false;
+    }
+    if (!subkeys_[i].WriteTo(&subkey)) {
+      LOG(ERROR) << "Failed writing subkey \"" << name << "\", result: "
+                 << result;
+      return false;
+    }
+  }
+
+  return true;
+}
 
 DeleteRegKeyWorkItem::~DeleteRegKeyWorkItem() {
 }
@@ -17,28 +271,68 @@ DeleteRegKeyWorkItem::DeleteRegKeyWorkItem(HKEY predefined_root,
                                            const std::wstring& path)
     : predefined_root_(predefined_root),
       path_(path) {
+  // It's a safe bet that we don't want to delete one of the root trees.
+  DCHECK(!path.empty());
 }
 
 bool DeleteRegKeyWorkItem::Do() {
-  // TODO(robertshield): Implement rollback for this work item. Consider using
-  // RegSaveKey or RegCopyKey.
+  scoped_ptr<RegKeyBackup> backup;
+
+  // Only try to make a backup if we're not configured to ignore failures.
   if (!ignore_failure_) {
-    NOTREACHED();
-    LOG(ERROR) << "Unexpected configuration in DeleteRegKeyWorkItem.";
-    return false;
+    RegKey original_key;
+
+    // Does the key exist?
+    LONG result = original_key.Open(predefined_root_, path_.c_str(),
+                                    kKeyReadNoNotify);
+    if (result == ERROR_SUCCESS) {
+      backup.reset(new RegKeyBackup());
+      if (!backup->Initialize(original_key)) {
+        LOG(ERROR) << "Failed to backup key at " << path_;
+        return ignore_failure_;
+      }
+    } else if (result != ERROR_FILE_NOT_FOUND) {
+      LOG(ERROR) << "Failed to open key at " << path_
+                 << " to create backup, result: " << result;
+      return ignore_failure_;
+    }
   }
 
-  LSTATUS result = SHDeleteKey(predefined_root_, path_.c_str());
-  if (result != ERROR_SUCCESS) {
-    LOG(ERROR) << "Failed to delete key at " << path_ << ", result: " << result;
+  // Delete the key.
+  LONG result = SHDeleteKey(predefined_root_, path_.c_str());
+  if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) {
+    LOG(ERROR) << "Failed to delete key at " << path_ << ", result: "
+               << result;
+    return ignore_failure_;
   }
 
+  // We've succeeded, so remember any backup we may have made.
+  backup_.swap(backup);
+
   return true;
 }
 
 void DeleteRegKeyWorkItem::Rollback() {
-  if (ignore_failure_)
+  if (ignore_failure_ || backup_.get() == NULL)
     return;
-  NOTREACHED();
-}
 
+  // Delete anything in the key before restoring the backup in case someone else
+  // put new data in the key after Do().
+  LONG result = SHDeleteKey(predefined_root_, path_.c_str());
+  if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) {
+    LOG(ERROR) << "Failed to delete key at " << path_ << " in rollback, "
+                  "result: " << result;
+  }
+
+  // Restore the old contents.  The restoration takes on its default security
+  // attributes; any custom attributes are lost.
+  RegKey original_key;
+  result = original_key.Create(predefined_root_, path_.c_str(), KEY_WRITE);
+  if (result != ERROR_SUCCESS) {
+    LOG(ERROR) << "Failed to create original key at " << path_
+               << " in rollback, result: " << result;
+  } else {
+    if (!backup_->WriteTo(&original_key))
+      LOG(ERROR) << "Failed to restore key in rollback, result: " << result;
+  }
+}
diff --git a/chrome/installer/util/delete_reg_key_work_item.h b/chrome/installer/util/delete_reg_key_work_item.h
index 3d42bf2..74806ed 100644
--- a/chrome/installer/util/delete_reg_key_work_item.h
+++ b/chrome/installer/util/delete_reg_key_work_item.h
@@ -11,9 +11,12 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/scoped_ptr.h"
 #include "chrome/installer/util/work_item.h"
 
-// A WorkItem subclass that deletes a registry key at the given path.
+// A WorkItem subclass that deletes a registry key at the given path.  Be aware
+// that in the event of rollback the key's values and subkeys are restored but
+// the key and its subkeys take on their default security descriptors.
 class DeleteRegKeyWorkItem : public WorkItem {
  public:
   virtual ~DeleteRegKeyWorkItem();
@@ -23,6 +26,7 @@ class DeleteRegKeyWorkItem : public WorkItem {
   virtual void Rollback();
 
  private:
+  class RegKeyBackup;
   friend class WorkItem;
 
   DeleteRegKeyWorkItem(HKEY predefined_root, const std::wstring& path);
@@ -34,10 +38,9 @@ class DeleteRegKeyWorkItem : public WorkItem {
   // Path of the key to be deleted.
   std::wstring path_;
 
-  // Path in the registry that the key is backed up to.
-  std::wstring backup_path_;
+  // Backup of the deleted key.
+  scoped_ptr<RegKeyBackup> backup_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(DeleteRegKeyWorkItem);
 };
 
diff --git a/chrome/installer/util/delete_reg_key_work_item_unittest.cc b/chrome/installer/util/delete_reg_key_work_item_unittest.cc
new file mode 100644
index 0000000..7e80a9a
--- /dev/null
+++ b/chrome/installer/util/delete_reg_key_work_item_unittest.cc
@@ -0,0 +1,177 @@
+// 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 <windows.h>
+#include <atlsecurity.h>  // NOLINT
+#include "base/logging.h"
+#include "base/scoped_ptr.h"
+#include "base/win/registry.h"
+#include "chrome/installer/util/delete_reg_key_work_item.h"
+#include "chrome/installer/util/work_item.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::win::RegKey;
+
+namespace {
+wchar_t test_root[] = L"SOFTWARE\\TmpTmp";
+}
+
+class DeleteRegKeyWorkItemTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
+    // Create a temporary key for testing
+    RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
+    key.DeleteKey(test_root);
+    ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, test_root, KEY_READ));
+    ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, test_root,
+                                        KEY_READ));
+  }
+
+  virtual void TearDown() {
+    logging::CloseLogFile();
+    // Clean up the temporary key
+    RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
+    ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(test_root));
+  }
+};
+
+// Test that deleting a key that doesn't exist succeeds, and that rollback does
+// nothing.
+TEST_F(DeleteRegKeyWorkItemTest, TestNoKey) {
+  RegKey key;
+  std::wstring key_name(std::wstring(test_root) + L"\\NoKeyHere");
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  scoped_ptr<DeleteRegKeyWorkItem> item(
+      WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name));
+  EXPECT_TRUE(item->Do());
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  item->Rollback();
+  item.reset();
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+}
+
+// Test that deleting a subkey of a key that doesn't exist succeeds, and that
+// rollback does nothing.
+TEST_F(DeleteRegKeyWorkItemTest, TestNoSubkey) {
+  RegKey key;
+  std::wstring key_name(std::wstring(test_root) + L"\\NoKeyHere\\OrHere");
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  scoped_ptr<DeleteRegKeyWorkItem> item(
+      WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name));
+  EXPECT_TRUE(item->Do());
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  item->Rollback();
+  item.reset();
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+}
+
+// Test that deleting an empty key succeeds, and that rollback brings it back.
+TEST_F(DeleteRegKeyWorkItemTest, TestEmptyKey) {
+  RegKey key;
+  std::wstring key_name(std::wstring(test_root) + L"\\EmptyKey");
+  EXPECT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, key_name.c_str(),
+                                      KEY_WRITE));
+  key.Close();
+  scoped_ptr<DeleteRegKeyWorkItem> item(
+      WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name));
+  EXPECT_TRUE(item->Do());
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  item->Rollback();
+  item.reset();
+  EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+}
+
+// Test that deleting a key with subkeys and values succeeds, and that rollback
+// brings them all back.
+TEST_F(DeleteRegKeyWorkItemTest, TestNonEmptyKey) {
+  RegKey key;
+  std::wstring key_name(std::wstring(test_root) + L"\\NonEmptyKey");
+  EXPECT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, key_name.c_str(),
+                                      KEY_WRITE));
+  EXPECT_EQ(ERROR_SUCCESS, key.WriteValue(NULL, key_name.c_str()));
+  EXPECT_EQ(ERROR_SUCCESS, key.CreateKey(L"Subkey", KEY_WRITE));
+  EXPECT_EQ(ERROR_SUCCESS, key.WriteValue(L"SomeValue", 1U));
+  key.Close();
+  scoped_ptr<DeleteRegKeyWorkItem> item(
+      WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name));
+  EXPECT_TRUE(item->Do());
+  EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  item->Rollback();
+  item.reset();
+  EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_READ));
+  std::wstring str_value;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &str_value));
+  EXPECT_EQ(key_name, str_value);
+  EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey", KEY_READ));
+  DWORD dw_value = 0;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"SomeValue", &dw_value));
+  EXPECT_EQ(1U, dw_value);
+}
+
+// Test that deleting a key with subkeys we can't delete fails, and that
+// everything is there after rollback.
+TEST_F(DeleteRegKeyWorkItemTest, TestUndeletableKey) {
+  RegKey key;
+  std::wstring key_name(std::wstring(test_root) + L"\\UndeletableKey");
+  EXPECT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, key_name.c_str(),
+                                      KEY_WRITE));
+  EXPECT_EQ(ERROR_SUCCESS, key.WriteValue(NULL, key_name.c_str()));
+  DWORD dw_value = 1;
+  RegKey subkey;
+  RegKey subkey2;
+  EXPECT_EQ(ERROR_SUCCESS, subkey.Create(key.Handle(), L"Subkey",
+                                         KEY_WRITE | WRITE_DAC));
+  EXPECT_EQ(ERROR_SUCCESS, subkey.WriteValue(L"SomeValue", 1U));
+  EXPECT_EQ(ERROR_SUCCESS, subkey2.Create(subkey.Handle(), L"Subkey2",
+                                          KEY_WRITE | WRITE_DAC));
+  EXPECT_EQ(ERROR_SUCCESS, subkey2.WriteValue(L"", 2U));
+  CSecurityDesc sec_desc;
+  sec_desc.FromString(L"D:PAI(A;OICI;KR;;;BU)");  // builtin users read
+  EXPECT_EQ(ERROR_SUCCESS,
+            RegSetKeySecurity(subkey.Handle(), DACL_SECURITY_INFORMATION,
+                              const_cast<SECURITY_DESCRIPTOR*>(
+                                  sec_desc.GetPSECURITY_DESCRIPTOR())));
+  sec_desc.FromString(L"D:PAI(A;OICI;KA;;;BU)");  // builtin users all access
+  EXPECT_EQ(ERROR_SUCCESS,
+            RegSetKeySecurity(subkey2.Handle(), DACL_SECURITY_INFORMATION,
+                              const_cast<SECURITY_DESCRIPTOR*>(
+                                  sec_desc.GetPSECURITY_DESCRIPTOR())));
+  subkey2.Close();
+  subkey.Close();
+  key.Close();
+  scoped_ptr<DeleteRegKeyWorkItem> item(
+      WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name));
+  EXPECT_FALSE(item->Do());
+  EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_QUERY_VALUE));
+  item->Rollback();
+  item.reset();
+  EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(),
+                                    KEY_QUERY_VALUE));
+  std::wstring str_value;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &str_value));
+  EXPECT_EQ(key_name, str_value);
+  EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey", KEY_READ | WRITE_DAC));
+  dw_value = 0;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"SomeValue", &dw_value));
+  EXPECT_EQ(1U, dw_value);
+  // Give users all access to the subkey so it can be deleted.
+  EXPECT_EQ(ERROR_SUCCESS,
+      RegSetKeySecurity(key.Handle(), DACL_SECURITY_INFORMATION,
+                        const_cast<SECURITY_DESCRIPTOR*>(
+                            sec_desc.GetPSECURITY_DESCRIPTOR())));
+  EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey2", KEY_QUERY_VALUE));
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"", &dw_value));
+  EXPECT_EQ(2U, dw_value);
+}
diff --git a/chrome/installer/util/work_item_list.cc b/chrome/installer/util/work_item_list.cc
index 3156f7d..958d478 100644
--- a/chrome/installer/util/work_item_list.cc
+++ b/chrome/installer/util/work_item_list.cc
@@ -208,7 +208,8 @@ bool NoRollbackWorkItemList::Do() {
     executed_list_.push_front(work_item);
     work_item->set_ignore_failure(true);
     if (!work_item->Do()) {
-      LOG(ERROR) << "NoRollbackWorkItemList: item execution failed.";
+      LOG(ERROR) << "NoRollbackWorkItemList: item execution failed "
+                 << work_item->log_message();
       result = false;
     }
   }
@@ -223,4 +224,3 @@ bool NoRollbackWorkItemList::Do() {
 void NoRollbackWorkItemList::Rollback() {
   NOTREACHED() << "Can't rollback a NoRollbackWorkItemList";
 }
-