Check Swaps and MergeDictionary for base::Value

base/values.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/values.h"
 
 #include <string.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 517 matching lines @@
     case Type::DICTIONARY:
       dict_ptr_.InitFromMove(std::move(that.dict_ptr_));
       return;
     case Type::LIST:
       list_.InitFromMove(std::move(that.list_));
       return;
   }
 }
 
 void Value::InternalCopyAssignFrom(const Value& that) {
-  type_ = that.type_;
+  CHECK_EQ(type_, that.type_);

[jdoerrie, 2017/03/06 12:34:43]

You could add a comment here that this does not prevent copy assigning between
Values of different types (we do a CleanUp followed by a CopyConstructFrom in
that case, see line 180). This confused me the first time I skimmed through this
CL. Another option would be to rename this method to
|InternalCopyAssignFromSameType|, WDYT?

[vabr (Chromium), 2017/03/06 17:09:32]

Yes, it is confusing. In fact, as I studied the crashes and found this method, I
was enthusiastic about having found the cause, until I realised there is the
CleanUp+construct path as you described.
I think changing the name is a good idea.
Done.

 
   switch (type_) {
     case Type::NONE:
     case Type::BOOLEAN:
     case Type::INTEGER:
     case Type::DOUBLE:
       InternalCopyFundamentalValue(that);
       return;
 
     case Type::STRING:
       *string_value_ = *that.string_value_;
       return;
     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.
     case Type::DICTIONARY:
       *dict_ptr_ = std::move(*that.CreateDeepCopy()->dict_ptr_);
       return;
     case Type::LIST:
       *list_ = std::move(*that.CreateDeepCopy()->list_);
       return;
   }
 }
 
 void Value::InternalMoveAssignFrom(Value&& that) {
-  type_ = that.type_;
+  CHECK_EQ(type_, that.type_);

[jdoerrie, 2017/03/06 12:34:43]

See above (s/copy/move).

[vabr (Chromium), 2017/03/06 17:09:32]

Done.

 
   switch (type_) {
     case Type::NONE:
     case Type::BOOLEAN:
     case Type::INTEGER:
     case Type::DOUBLE:
       InternalCopyFundamentalValue(that);
       return;
 
     case Type::STRING:
@@ skipping 452 matching lines @@
 std::unique_ptr<DictionaryValue> DictionaryValue::DeepCopyWithoutEmptyChildren()
     const {
   std::unique_ptr<DictionaryValue> copy =
       CopyDictionaryWithoutEmptyChildren(*this);
   if (!copy)
     copy.reset(new DictionaryValue);
   return copy;
 }
 
 void DictionaryValue::MergeDictionary(const DictionaryValue* dictionary) {
+  CHECK(dictionary->is_dict());
   for (DictionaryValue::Iterator it(*dictionary); !it.IsAtEnd(); it.Advance()) {
     const Value* merge_value = &it.value();
     // Check whether we have to merge dictionaries.
     if (merge_value->IsType(Value::Type::DICTIONARY)) {
       DictionaryValue* sub_dict;
       if (GetDictionaryWithoutPathExpansion(it.key(), &sub_dict)) {
         sub_dict->MergeDictionary(
             static_cast<const DictionaryValue*>(merge_value));
         continue;
       }
     }
     // All other cases: Make a copy and hook it up.
     SetWithoutPathExpansion(it.key(),
                             base::WrapUnique(merge_value->DeepCopy()));
   }
 }
 
 void DictionaryValue::Swap(DictionaryValue* other) {
+  CHECK(other->is_dict());
   dict_ptr_->swap(*(other->dict_ptr_));
 }
 
 DictionaryValue::Iterator::Iterator(const DictionaryValue& target)
     : target_(target), it_((*target.dict_ptr_)->begin()) {}
 
 DictionaryValue::Iterator::Iterator(const Iterator& other) = default;
 
 DictionaryValue::Iterator::~Iterator() {}
 
@@ skipping 254 matching lines @@
 }
 
 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);
                       });
 }
 
 void ListValue::Swap(ListValue* other) {
+  CHECK(other->is_list());
   list_->swap(*(other->list_));
 }
 
 ListValue* ListValue::DeepCopy() const {
   return static_cast<ListValue*>(Value::DeepCopy());
 }
 
 std::unique_ptr<ListValue> ListValue::CreateDeepCopy() const {
   return WrapUnique(DeepCopy());
 }
@@ skipping 11 matching lines @@
 }
 
 std::ostream& operator<<(std::ostream& out, const Value::Type& type) {
   if (static_cast<int>(type) < 0 ||
       static_cast<size_t>(type) >= arraysize(kTypeNames))
     return out << "Invalid Type (index = " << static_cast<int>(type) << ")";
   return out << Value::GetTypeName(type);
 }
 
 }  // namespace base