Change base::Value::ListStorage to std::vector<base::Value>

base/values.cc

Index: base/values.cc
diff --git a/base/values.cc b/base/values.cc
index 25a25fba6e489ce8420954941598dace98da8f38..050afc330a8a418d9d0fcaa1c15c15a84deb7fa2 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -35,7 +35,7 @@ std::unique_ptr<Value> CopyWithoutEmptyChildren(const Value& node);
 std::unique_ptr<ListValue> CopyListWithoutEmptyChildren(const ListValue& list) {
   std::unique_ptr<ListValue> copy;
   for (const auto& entry : list) {
-    std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(*entry);
+    std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(entry);
     if (child_copy) {
       if (!copy)
         copy.reset(new ListValue);
@@ -172,6 +172,14 @@ Value::Value(std::vector<char>&& in_blob) : type_(Type::BINARY) {
   binary_value_.Init(std::move(in_blob));
 }
 
+Value::Value(const ListStorage& in_list) : type_(Type::LIST) {
+  list_.Init(in_list);
+}
+
+Value::Value(ListStorage&& in_list) : type_(Type::LIST) {
+  list_.Init(std::move(in_list));
+}
+
 Value& Value::operator=(const Value& that) {
   if (this != &that) {
     if (type_ == that.type_) {
@@ -380,14 +388,8 @@ Value* Value::DeepCopy() const {
       return result;
     }
 
-    case Type::LIST: {
-      ListValue* result = new ListValue;
-
-      for (const auto& entry : *list_)
-        result->Append(entry->CreateDeepCopy());
-
-      return result;
-    }
+    case Type::LIST:
+      return new Value(*list_);
 
     default:
       NOTREACHED();
@@ -436,12 +438,10 @@ bool Value::Equals(const Value* other) const {
       if (list_->size() != other->list_->size())
         return false;
 
-      return std::equal(std::begin(*list_), std::end(*list_),
-                        std::begin(*(other->list_)),
-                        [](const ListStorage::value_type& lhs,
-                           const ListStorage::value_type& rhs) {
-                          return lhs->Equals(rhs.get());
-                        });
+      return std::equal(
+          std::begin(*list_), std::end(*list_), std::begin(*(other->list_)),
+          [](const ListStorage::value_type& lhs,
+             const ListStorage::value_type& rhs) { return lhs.Equals(&rhs); });
     }
   }
 
@@ -494,15 +494,15 @@ void Value::InternalCopyConstructFrom(const Value& that) {
     case Type::BINARY:
       binary_value_.Init(*that.binary_value_);
       return;
-    // DictStorage and ListStorage are move-only types due to the presence of
-    // unique_ptrs. This is why the call to |CreateDeepCopy| is necessary here.
-    // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
-    // can be copied directly.
+    // DictStorage is a move-only type due to the presence of unique_ptrs. This
+    // is why the call to |CreateDeepCopy| is necessary here.
+    // TODO(crbug.com/646113): Clean this up when DictStorage can be copied
+    // directly.
     case Type::DICTIONARY:
       dict_ptr_.Init(std::move(*that.CreateDeepCopy()->dict_ptr_));

[brettw, 2017/03/17 05:39:19]

I suspect this line is causing the Android crash. CreateDeepCopy creates a
temporary unique_ptr, we get the dict_ptr out of that, but then the unique_ptr
is destroyed and we pass a dangling pointer to std::move.

Try making a named unique_ptr<Value> to hold the deep copy on the previous line
and see if that makes the problem go away.

[jdoerrie, 2017/03/23 18:11:17]

Unfortunately this didn't solve the issue. After more fruitless printf debugging
I am now trying (and currently still failing) to get the test running with an
ASAN build. This will hopefully make the issue obvious.

       return;
     case Type::LIST:
-      list_.Init(std::move(*that.CreateDeepCopy()->list_));
+      list_.Init(*that.list_);
       return;
   }
 }
@@ -550,15 +550,15 @@ void Value::InternalCopyAssignFromSameType(const Value& that) {
     case Type::BINARY:
       *binary_value_ = *that.binary_value_;
       return;
-    // DictStorage and ListStorage are move-only types due to the presence of
-    // unique_ptrs. This is why the call to |CreateDeepCopy| is necessary here.
-    // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
-    // can be copied directly.
+    // DictStorage is a move-only type due to the presence of unique_ptrs. This
+    // is why the call to |CreateDeepCopy| is necessary here.
+    // TODO(crbug.com/646113): Clean this up when DictStorage can be copied
+    // directly.
     case Type::DICTIONARY:
       *dict_ptr_ = std::move(*that.CreateDeepCopy()->dict_ptr_);

[brettw, 2017/03/17 05:39:19]

Ditto here.

       return;
     case Type::LIST:
-      *list_ = std::move(*that.CreateDeepCopy()->list_);
+      *list_ = *that.list_;
       return;
   }
 }
@@ -1095,6 +1095,10 @@ void ListValue::Clear() {
   list_->clear();
 }
 
+void ListValue::Reserve(size_t n) {
+  list_->reserve(n);
+}
+
 bool ListValue::Set(size_t index, Value* in_value) {
   return Set(index, WrapUnique(in_value));
 }
@@ -1109,9 +1113,7 @@ bool ListValue::Set(size_t index, std::unique_ptr<Value> in_value) {
       Append(CreateNullValue());
     Append(std::move(in_value));
   } else {
-    // TODO(dcheng): remove this DCHECK once the raw pointer version is removed?
-    DCHECK((*list_)[index] != in_value);
-    (*list_)[index] = std::move(in_value);
+    (*list_)[index] = std::move(*in_value);
   }
   return true;
 }
@@ -1121,7 +1123,7 @@ bool ListValue::Get(size_t index, const Value** out_value) const {
     return false;
 
   if (out_value)
-    *out_value = (*list_)[index].get();
+    *out_value = &(*list_)[index];
 
   return true;
 }
@@ -1232,36 +1234,37 @@ bool ListValue::Remove(size_t index, std::unique_ptr<Value>* out_value) {
     return false;
 
   if (out_value)
-    *out_value = std::move((*list_)[index]);
+    *out_value = MakeUnique<Value>(std::move((*list_)[index]));
 
   list_->erase(list_->begin() + index);
   return true;
 }
 
 bool ListValue::Remove(const Value& value, size_t* index) {
-  for (auto it = list_->begin(); it != list_->end(); ++it) {
-    if ((*it)->Equals(&value)) {
-      size_t previous_index = it - list_->begin();
-      list_->erase(it);
+  auto it = std::find_if(
+      list_->begin(), list_->end(),
+      [&value](const Value& entry) { return entry.Equals(&value); });
 
-      if (index)
-        *index = previous_index;
-      return true;
-    }
-  }
-  return false;
+  if (it == list_->end())
+    return false;
+
+  if (index)
+    *index = std::distance(list_->begin(), it);
+
+  list_->erase(it);
+  return true;
 }
 
 ListValue::iterator ListValue::Erase(iterator iter,
                                      std::unique_ptr<Value>* out_value) {
   if (out_value)
-    *out_value = std::move(*ListStorage::iterator(iter));
+    *out_value = MakeUnique<Value>(std::move(*iter));
 
   return list_->erase(iter);
 }
 
 void ListValue::Append(std::unique_ptr<Value> in_value) {
-  list_->push_back(std::move(in_value));
+  list_->push_back(std::move(*in_value));
 }
 
 #if !defined(OS_LINUX)
@@ -1308,11 +1311,11 @@ void ListValue::AppendStrings(const std::vector<string16>& in_values) {
 bool ListValue::AppendIfNotPresent(std::unique_ptr<Value> in_value) {
   DCHECK(in_value);
   for (const auto& entry : *list_) {
-    if (entry->Equals(in_value.get())) {
+    if (entry.Equals(in_value.get())) {
       return false;
     }
   }
-  list_->push_back(std::move(in_value));
+  list_->push_back(std::move(*in_value));
   return true;
 }
 
@@ -1321,15 +1324,14 @@ bool ListValue::Insert(size_t index, std::unique_ptr<Value> in_value) {
   if (index > list_->size())
     return false;
 
-  list_->insert(list_->begin() + index, std::move(in_value));
+  list_->insert(list_->begin() + index, std::move(*in_value));
   return true;
 }
 
 ListValue::const_iterator ListValue::Find(const Value& value) const {
-  return std::find_if(list_->begin(), list_->end(),
-                      [&value](const std::unique_ptr<Value>& entry) {
-                        return entry->Equals(&value);
-                      });
+  return std::find_if(
+      list_->begin(), list_->end(),
+      [&value](const Value& entry) { return entry.Equals(&value); });
 }
 
 void ListValue::Swap(ListValue* other) {