Fix *RegValueWorkItem to handle a case where previous value is empty.

Not handling correctly caused a crash. Added unit tests to cover those cases.

BUG=71953
TEST=covered by installer util unit tests

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

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

4 files changed, 70 insertions, 20 deletions

diff --git a/chrome/installer/util/delete_reg_value_work_item.cc b/chrome/installer/util/delete_reg_value_work_item.cc
index 143dae8..6efb2d8 100644
--- a/chrome/installer/util/delete_reg_value_work_item.cc
+++ b/chrome/installer/util/delete_reg_value_work_item.cc
@@ -49,12 +49,16 @@ bool DeleteRegValueWorkItem::Do() {
   }
 
   if (result == ERROR_SUCCESS) {
-    previous_value_.resize(size);
-    result = key.ReadValue(value_name_.c_str(), &previous_value_[0], &size,
-                           &previous_type_);
-    if (result != ERROR_SUCCESS) {
-      previous_value_.erase();
-      VLOG(1) << "Failed to save original value. Error: " << result;
+    if (!size) {
+      previous_type_ = type;
+    } else {
+      previous_value_.resize(size);
+      result = key.ReadValue(value_name_.c_str(), &previous_value_[0], &size,
+                             &previous_type_);
+      if (result != ERROR_SUCCESS) {
+        previous_value_.erase();
+        VLOG(1) << "Failed to save original value. Error: " << result;
+      }
     }
   }
 
diff --git a/chrome/installer/util/delete_reg_value_work_item_unittest.cc b/chrome/installer/util/delete_reg_value_work_item_unittest.cc
index 98f37d15..2491557 100644
--- a/chrome/installer/util/delete_reg_value_work_item_unittest.cc
+++ b/chrome/installer/util/delete_reg_value_work_item_unittest.cc
@@ -53,21 +53,31 @@ TEST_F(DeleteRegValueWorkItemTest, DeleteExistingValue) {
   DWORD data_dword = 100;
   ASSERT_EQ(ERROR_SUCCESS, key.WriteValue(name_dword.c_str(), data_dword));
 
+  std::wstring name_empty(L"name_empty");
+  ASSERT_EQ(ERROR_SUCCESS, RegSetValueEx(key.Handle(), name_empty.c_str(), NULL,
+                                         REG_SZ, NULL, 0));
+
   scoped_ptr<DeleteRegValueWorkItem> work_item1(
       WorkItem::CreateDeleteRegValueWorkItem(HKEY_CURRENT_USER, parent_key,
                                              name_str));
   scoped_ptr<DeleteRegValueWorkItem> work_item2(
       WorkItem::CreateDeleteRegValueWorkItem(HKEY_CURRENT_USER, parent_key,
                                              name_dword));
+  scoped_ptr<DeleteRegValueWorkItem> work_item3(
+      WorkItem::CreateDeleteRegValueWorkItem(HKEY_CURRENT_USER, parent_key,
+                                             name_empty));
 
   EXPECT_TRUE(work_item1->Do());
   EXPECT_TRUE(work_item2->Do());
+  EXPECT_TRUE(work_item3->Do());
 
   EXPECT_FALSE(key.ValueExists(name_str.c_str()));
   EXPECT_FALSE(key.ValueExists(name_dword.c_str()));
+  EXPECT_FALSE(key.ValueExists(name_empty.c_str()));
 
   work_item1->Rollback();
   work_item2->Rollback();
+  work_item3->Rollback();
 
   std::wstring read_str;
   DWORD read_dword;
@@ -75,6 +85,14 @@ TEST_F(DeleteRegValueWorkItemTest, DeleteExistingValue) {
   EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(name_dword.c_str(), &read_dword));
   EXPECT_EQ(read_str, data_str);
   EXPECT_EQ(read_dword, data_dword);
+
+  // Verify empty value.
+  DWORD type = 0;
+  DWORD size = 0;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name_empty.c_str(), NULL, &size,
+                                         &type));
+  EXPECT_EQ(REG_SZ, type);
+  EXPECT_EQ(0, size);
 }
 
 // Try deleting a value that doesn't exist.
diff --git a/chrome/installer/util/set_reg_value_work_item.cc b/chrome/installer/util/set_reg_value_work_item.cc
index 9c4d1c0..b944288 100644
--- a/chrome/installer/util/set_reg_value_work_item.cc
+++ b/chrome/installer/util/set_reg_value_work_item.cc
@@ -89,12 +89,16 @@ bool SetRegValueWorkItem::Do() {
 
   // If there's something to be saved, save it.
   if (result == ERROR_SUCCESS) {
-    previous_value_.resize(size);
-    result = key.ReadValue(value_name_.c_str(), &previous_value_[0], &size,
-                           &previous_type_);
-    if (result != ERROR_SUCCESS) {
-      previous_value_.clear();
-      VLOG(1) << "Failed to save original value. Error: " << result;
+    if (!size) {
+      previous_type_ = type;
+    } else {
+      previous_value_.resize(size);
+      result = key.ReadValue(value_name_.c_str(), &previous_value_[0], &size,
+                             &previous_type_);
+      if (result != ERROR_SUCCESS) {
+        previous_value_.clear();
+        VLOG(1) << "Failed to save original value. Error: " << result;
+      }
     }
   }
 
@@ -133,7 +137,9 @@ void SetRegValueWorkItem::Rollback() {
     result = key.DeleteValue(value_name_.c_str());
     VLOG(1) << "rollback: deleting " << value_name_ << " error: " << result;
   } else if (status_ == VALUE_OVERWRITTEN) {
-    result = key.WriteValue(value_name_.c_str(), &previous_value_[0],
+    const unsigned char* previous_value =
+        previous_value_.empty() ? NULL : &previous_value_[0];
+    result = key.WriteValue(value_name_.c_str(), previous_value,
                             static_cast<DWORD>(previous_value_.size()),
                             previous_type_);
     VLOG(1) << "rollback: restoring " << value_name_ << " error: " << result;
diff --git a/chrome/installer/util/set_reg_value_work_item_unittest.cc b/chrome/installer/util/set_reg_value_work_item_unittest.cc
index 892f28d..beb6a3a 100644
--- a/chrome/installer/util/set_reg_value_work_item_unittest.cc
+++ b/chrome/installer/util/set_reg_value_work_item_unittest.cc
@@ -182,34 +182,56 @@ TEST_F(SetRegValueWorkItemTest, WriteExistingOverwrite) {
   std::wstring name(L"name_str");
   ASSERT_EQ(ERROR_SUCCESS, key.WriteValue(name.c_str(), data_str_1));
 
+  std::wstring name_empty(L"name_empty");
+  ASSERT_EQ(ERROR_SUCCESS, RegSetValueEx(key.Handle(), name_empty.c_str(), NULL,
+                                         REG_SZ, NULL, 0));
+
   std::wstring data(data_str_2);
-  scoped_ptr<SetRegValueWorkItem> work_item(
+  scoped_ptr<SetRegValueWorkItem> work_item1(
       WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER, parent_key,
                                           name, data, true));
-  EXPECT_TRUE(work_item->Do());
+  scoped_ptr<SetRegValueWorkItem> work_item2(
+      WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER, parent_key,
+                                          name_empty, data, true));
+
+  EXPECT_TRUE(work_item1->Do());
+  EXPECT_TRUE(work_item2->Do());
 
   std::wstring read_out;
   EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name.c_str(), &read_out));
   EXPECT_EQ(0, read_out.compare(data_str_2));
 
-  work_item->Rollback();
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name_empty.c_str(), &read_out));
+  EXPECT_EQ(0, read_out.compare(data_str_2));
+
+  work_item1->Rollback();
+  work_item2->Rollback();
+
   EXPECT_TRUE(key.ValueExists(name.c_str()));
   EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name.c_str(), &read_out));
   EXPECT_EQ(read_out, data_str_1);
 
+  DWORD type = 0;
+  DWORD size = 0;
+  EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(name_empty.c_str(), NULL, &size,
+                                         &type));
+  EXPECT_EQ(REG_SZ, type);
+  EXPECT_EQ(0, size);
+
   // Now test REG_DWORD value.
   // Write data to the value we are going to set.
   name.assign(L"name_dword");
   ASSERT_EQ(ERROR_SUCCESS, key.WriteValue(name.c_str(), dword1));
-  work_item.reset(WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER,
-      parent_key, name, dword2, true));
-  EXPECT_TRUE(work_item->Do());
+  scoped_ptr<SetRegValueWorkItem> work_item3(
+      WorkItem::CreateSetRegValueWorkItem(HKEY_CURRENT_USER, parent_key, name,
+                                          dword2, true));
+  EXPECT_TRUE(work_item3->Do());
 
   DWORD read_dword;
   EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(name.c_str(), &read_dword));
   EXPECT_EQ(read_dword, dword2);
 
-  work_item->Rollback();
+  work_item3->Rollback();
   EXPECT_TRUE(key.ValueExists(name.c_str()));
   EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(name.c_str(), &read_dword));
   EXPECT_EQ(read_dword, dword1);