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 dc32d9fd94961e2ea40ef9fd1bc756349b5fd999..7abff15196453b90eaec6c697edbcc2f03196cc9 100644
--- a/content/child/v8_value_converter_impl.cc
+++ b/content/child/v8_value_converter_impl.cc
@@ -91,33 +91,85 @@ 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) {
-    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)
-        return false;
-    }
+  bool AddToUniquenessCheck(v8::Local<v8::Object> handle) {
+    int hash;
+    Iterator iter = GetIteratorInMap(handle, &hash);
+    if (iter != unique_map_.end())
+      return false;
+
     unique_map_.insert(std::make_pair(hash, handle));
     return true;
   }
 
+  bool RemoveFromUniquenessCheck(v8::Local<v8::Object> handle) {
+    int unused_hash;
+    Iterator iter = GetIteratorInMap(handle, &unused_hash);
+    if (iter == unique_map_.end())
+      return false;
+    unique_map_.erase(iter);
+    return true;
+  }
+
   bool HasReachedMaxRecursionDepth() {
     return max_recursion_depth_ < 0;
   }
 
  private:
-  typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap;
+  using HashToHandleMap = std::multimap<int, v8::Local<v8::Object>>;
+  using Iterator = HashToHandleMap::const_iterator;
+
+  Iterator GetIteratorInMap(v8::Local<v8::Object> handle, int* hash) {
+    *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)
+        return it;
+    }
+    // Not found.
+    return unique_map_.end();
+  }
+
   HashToHandleMap unique_map_;
 
   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() {
@@ -373,7 +425,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;
@@ -458,7 +511,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;