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

content/child/v8_value_converter_impl.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 "content/child/v8_value_converter_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <cmath>
@@ skipping 73 matching lines @@
       : max_recursion_depth_(kMaxRecursionDepth),
         avoid_identity_hash_for_testing_(avoid_identity_hash_for_testing) {}
 
   // If |handle| is not in |unique_map_|, then add it to |unique_map_| and
   // return true.
   //
   // Otherwise do nothing and return false. Here "A is unique" means that no
   // 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
     // 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;
     }
     unique_map_.insert(std::make_pair(hash, handle));
     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;
   }
 
  private:
   typedef std::multimap<int, v8::Local<v8::Object> > HashToHandleMap;
   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() {
   return new V8ValueConverterImpl();
 }
 
 V8ValueConverterImpl::V8ValueConverterImpl()
     : date_allowed_(false),
       reg_exp_allowed_(false),
       function_allowed_(false),
@@ skipping 239 matching lines @@
     return FromV8Object(val.As<v8::Object>(), state, isolate);
 
   LOG(ERROR) << "Unexpected v8 value type encountered.";
   return NULL;
 }
 
 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;
   // If val was created in a different context than our current one, change to
   // that context, but change back after val is converted.
   if (!val->CreationContext().IsEmpty() &&
       val->CreationContext() != isolate->GetCurrentContext())
     scope.reset(new v8::Context::Scope(val->CreationContext()));
 
   if (strategy_) {
@@ skipping 64 matching lines @@
   if (data)
     return base::BinaryValue::CreateWithCopiedBuffer(data, length);
   else
     return NULL;
 }
 
 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;
   // If val was created in a different context than our current one, change to
   // that context, but change back after val is converted.
   if (!val->CreationContext().IsEmpty() &&
       val->CreationContext() != isolate->GetCurrentContext())
     scope.reset(new v8::Context::Scope(val->CreationContext()));
 
   if (strategy_) {
@@ skipping 80 matching lines @@
       continue;
 
     result->SetWithoutPathExpansion(std::string(*name_utf8, name_utf8.length()),
                                     child.release());
   }
 
   return result.release();
 }
 
 }  // namespace content