Properly detect cycles in V8ValueConverter, current impl is too strict.

content/child/v8_value_converter_impl.cc

Index: content/child/v8_value_converter_impl.cc
diff --git a/content/child/v8_value_converter_impl.cc b/content/child/v8_value_converter_impl.cc
index c37160fe5979486981024510777ddf779dbb9040..d3abdcda73ac134c34cf18eb2013b6a838babf28 100644
--- a/content/child/v8_value_converter_impl.cc
+++ b/content/child/v8_value_converter_impl.cc
@@ -91,7 +91,7 @@ class V8ValueConverterImpl::FromV8ValueState {
   // other handle B in the map points to the same object as A. Note that A can
   // be unique even if there already is another handle with the same identity
   // hash (key) in the map, because two objects can have the same hash.
-  bool UpdateAndCheckUniqueness(v8::Local<v8::Object> handle) {
+  bool AddToUniquenessCheck(v8::Local<v8::Object> handle) {
     typedef HashToHandleMap::const_iterator Iterator;
     int hash = avoid_identity_hash_for_testing_ ? 0 : handle->GetIdentityHash();
     // We only compare using == with handles to objects with the same identity
@@ -107,6 +107,23 @@ class V8ValueConverterImpl::FromV8ValueState {
     return true;
   }
 
+  bool RemoveFromUniquenessCheck(v8::Local<v8::Object> handle) {
+    typedef HashToHandleMap::const_iterator Iterator;
+    int hash = avoid_identity_hash_for_testing_ ? 0 : handle->GetIdentityHash();
+    // We only compare using == with handles to objects with the same identity
+    // hash. Different hash obviously means different objects, but two objects
+    // in a couple of thousands could have the same identity hash.
+    std::pair<Iterator, Iterator> range = unique_map_.equal_range(hash);
+    for (Iterator it = range.first; it != range.second; ++it) {
+      // Operator == for handles actually compares the underlying objects.
+      if (it->second == handle) {
+        unique_map_.erase(it);
+        return true;
+      }
+    }

[asargent_no_longer_on_chrome, 2016/05/03 23:04:58]

nit: looks all the above code is duplicated from AddToUniquenessCheck - it might
make sense to factor it out into a helper named something like "ContainsObject"
that takes the unique_map and the handle and returns a boolean, which you can
then use in both methods. 

[lazyboy, 2016/05/03 23:57:49]

returning Iterator instead since I'd have lookup twice otherwise.
We also need to return the hash as GetIdentityHash() seems expensive.
Done.

+    return false;
+  }
+
   bool HasReachedMaxRecursionDepth() {
     return max_recursion_depth_ < 0;
   }
@@ -118,6 +135,38 @@ class V8ValueConverterImpl::FromV8ValueState {
   int max_recursion_depth_;
 
   bool avoid_identity_hash_for_testing_;
+
+  DISALLOW_COPY_AND_ASSIGN(FromV8ValueState);
+};
+
+// A class to ensure that objects/arrays that are being converted by
+// this V8ValueConverterImpl do not have cycles.
+//
+// An example of cycle: var v = {}; v = {key: v};
+// Not an example of cycle: var v = {}; a = [v, v]; or w = {a: v, b: v};
+class V8ValueConverterImpl::ScopedUniquenessGuard {
+ public:
+  ScopedUniquenessGuard(V8ValueConverterImpl::FromV8ValueState* state,
+                        v8::Local<v8::Object> value)
+      : state_(state),
+        value_(value),
+        is_valid_(state_->AddToUniquenessCheck(value_)) {}
+  ~ScopedUniquenessGuard() {
+    if (is_valid_) {
+      bool removed = state_->RemoveFromUniquenessCheck(value_);
+      DCHECK(removed);
+    }
+  }
+
+  bool is_valid() const { return is_valid_; }
+
+ private:
+  typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap;
+  V8ValueConverterImpl::FromV8ValueState* state_;
+  v8::Local<v8::Object> value_;
+  bool is_valid_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedUniquenessGuard);
 };
 
 V8ValueConverter* V8ValueConverter::create() {
@@ -377,7 +426,8 @@ base::Value* V8ValueConverterImpl::FromV8Array(
     v8::Local<v8::Array> val,
     FromV8ValueState* state,
     v8::Isolate* isolate) const {
-  if (!state->UpdateAndCheckUniqueness(val))
+  ScopedUniquenessGuard uniqueness_guard(state, val);
+  if (!uniqueness_guard.is_valid())
     return base::Value::CreateNullValue().release();
 
   std::unique_ptr<v8::Context::Scope> scope;
@@ -462,7 +512,8 @@ base::Value* V8ValueConverterImpl::FromV8Object(
     v8::Local<v8::Object> val,
     FromV8ValueState* state,
     v8::Isolate* isolate) const {
-  if (!state->UpdateAndCheckUniqueness(val))
+  ScopedUniquenessGuard uniqueness_guard(state, val);
+  if (!uniqueness_guard.is_valid())
     return base::Value::CreateNullValue().release();
 
   std::unique_ptr<v8::Context::Scope> scope;