Check Swaps and MergeDictionary for base::Value

base/values.cc

Index: base/values.cc
diff --git a/base/values.cc b/base/values.cc
index 9aa04f33c64b1cc1eee031f1e89b03ed7b5bb358..8172c48c5938b3e870e83e8d0c012de880ff6681 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -535,7 +535,7 @@ void Value::InternalMoveConstructFrom(Value&& that) {
 }
 
 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:
@@ -565,7 +565,7 @@ void Value::InternalCopyAssignFrom(const Value& that) {
 }
 
 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:
@@ -1038,6 +1038,7 @@ std::unique_ptr<DictionaryValue> DictionaryValue::DeepCopyWithoutEmptyChildren()
 }
 
 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.
@@ -1056,6 +1057,7 @@ void DictionaryValue::MergeDictionary(const DictionaryValue* dictionary) {
 }
 
 void DictionaryValue::Swap(DictionaryValue* other) {
+  CHECK(other->is_dict());
   dict_ptr_->swap(*(other->dict_ptr_));
 }
 
@@ -1330,6 +1332,7 @@ ListValue::const_iterator ListValue::Find(const Value& value) const {
 }
 
 void ListValue::Swap(ListValue* other) {
+  CHECK(other->is_list());
   list_->swap(*(other->list_));
 }