Recurse to a maximum depth of 10 in v8_value_converter_impl.cc. There are

objects that get passed in (canonically <input> elements apparently) which
recurse infinitely (as opposed to having a self-referential loop).

The previous solution to this problem (r150035) was to disable getters, which
apparently were the main cause, but this is no longer appropriate - we now use
this mechanism for all extension messaging, and this has become a problem (see
bug 246213).

TBR=jamesr@chromium.org, mpcomplete@chromium.org

BUG=246213,139933

Review URL: https://codereview.chromium.org/16295013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204057 0039d316-1c4b-4281-b951-d872f2087c98

chrome/test/data/extensions/api_test/executescript/callback/test.js

@@ -86,7 +86,7 @@ chrome.test.getConfig(function(config) {
         });
       },
 
-      // Non-pure ojbects (like DOM nodes) will get converted as best they can.
+      // Non-pure object (like DOM nodes) will get converted as best they can.
       function executeCallbackDOMObjShouldSucceed() {
         var scriptDetails = {};
         scriptDetails.code = 'var a = document.getElementById("testDiv"); a;';

content/renderer/v8_value_converter_impl.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/float_util.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/values.h"
@@ -15,6 +16,67 @@
 
 namespace content {
 
+namespace {
+const int kMaxRecursionDepth = 10;
+}
+
+// The state of a call to FromV8Value.
+class V8ValueConverterImpl::FromV8ValueState {
+ public:
+  // Level scope which updates the current depth of some FromV8ValueState.
+  class Level {
+   public:
+    explicit Level(FromV8ValueState* state) : state_(state) {
+      state_->max_recursion_depth_--;
+    }
+    ~Level() {
+      state_->max_recursion_depth_++;
+    }
+
+   private:
+    FromV8ValueState* state_;
+  };
+
+  explicit FromV8ValueState(bool avoid_identity_hash_for_testing)
+      : 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::Handle<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 HasReachedMaxRecursionDepth() {
+    return max_recursion_depth_ < 0;
+  }
+
+ private:
+  typedef std::multimap<int, v8::Handle<v8::Object> > HashToHandleMap;
+  HashToHandleMap unique_map_;
+
+  int max_recursion_depth_;
+
+  bool avoid_identity_hash_for_testing_;
+};
+
 V8ValueConverter* V8ValueConverter::create() {
   return new V8ValueConverterImpl();
 }
@@ -54,8 +116,8 @@ Value* V8ValueConverterImpl::FromV8Value(
     v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
   v8::HandleScope handle_scope;
-  HashToHandleMap unique_map;
-  return FromV8ValueImpl(val, &unique_map);
+  FromV8ValueState state(avoid_identity_hash_for_testing_);
+  return FromV8ValueImpl(val, &state);
 }
 
 v8::Handle<v8::Value> V8ValueConverterImpl::ToV8ValueImpl(
@@ -153,10 +215,15 @@ v8::Handle<v8::Value> V8ValueConverterImpl::ToArrayBuffer(
   return buffer.toV8Value();
 }
 
-Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
-    HashToHandleMap* unique_map) const {
+Value* V8ValueConverterImpl::FromV8ValueImpl(
+    v8::Handle<v8::Value> val,
+    FromV8ValueState* state) const {
   CHECK(!val.IsEmpty());
 
+  FromV8ValueState::Level state_level(state);
+  if (state->HasReachedMaxRecursionDepth())
+    return NULL;
+
   if (val->IsNull())
     return base::Value::CreateNullValue();
 
@@ -166,8 +233,12 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
   if (val->IsInt32())
     return new base::FundamentalValue(val->ToInt32()->Value());
 
-  if (val->IsNumber())
-    return new base::FundamentalValue(val->ToNumber()->Value());
+  if (val->IsNumber()) {
+    double val_as_double = val->ToNumber()->Value();
+    if (!base::IsFinite(val_as_double))
+      return NULL;
+    return new base::FundamentalValue(val_as_double);
+  }
 
   if (val->IsString()) {
     v8::String::Utf8Value utf8(val->ToString());
@@ -182,7 +253,7 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
     if (!date_allowed_)
       // JSON.stringify would convert this to a string, but an object is more
       // consistent within this class.
-      return FromV8Object(val->ToObject(), unique_map);
+      return FromV8Object(val->ToObject(), state);
     v8::Date* date = v8::Date::Cast(*val);
     return new base::FundamentalValue(date->NumberValue() / 1000.0);
   }
@@ -190,19 +261,19 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
   if (val->IsRegExp()) {
     if (!reg_exp_allowed_)
       // JSON.stringify converts to an object.
-      return FromV8Object(val->ToObject(), unique_map);
+      return FromV8Object(val->ToObject(), state);
     return new base::StringValue(*v8::String::Utf8Value(val->ToString()));
   }
 
   // v8::Value doesn't have a ToArray() method for some reason.
   if (val->IsArray())
-    return FromV8Array(val.As<v8::Array>(), unique_map);
+    return FromV8Array(val.As<v8::Array>(), state);
 
   if (val->IsFunction()) {
     if (!function_allowed_)
       // JSON.stringify refuses to convert function(){}.
       return NULL;
-    return FromV8Object(val->ToObject(), unique_map);
+    return FromV8Object(val->ToObject(), state);
   }
 
   if (val->IsObject()) {
@@ -210,7 +281,7 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
     if (binary_value) {
       return binary_value;
     } else {
-      return FromV8Object(val->ToObject(), unique_map);
+      return FromV8Object(val->ToObject(), state);
     }
   }
 
@@ -218,9 +289,10 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
   return NULL;
 }
 
-Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
-    HashToHandleMap* unique_map) const {
-  if (!UpdateAndCheckUniqueness(unique_map, val))
+Value* V8ValueConverterImpl::FromV8Array(
+    v8::Handle<v8::Array> val,
+    FromV8ValueState* state) const {
+  if (!state->UpdateAndCheckUniqueness(val))
     return base::Value::CreateNullValue();
 
   scoped_ptr<v8::Context::Scope> scope;
@@ -244,7 +316,7 @@ Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
     if (!val->HasRealIndexedProperty(i))
       continue;
 
-    base::Value* child = FromV8ValueImpl(child_v8, unique_map);
+    base::Value* child = FromV8ValueImpl(child_v8, state);
     if (child)
       result->Append(child);
     else
@@ -282,8 +354,8 @@ base::BinaryValue* V8ValueConverterImpl::FromV8Buffer(
 
 Value* V8ValueConverterImpl::FromV8Object(
     v8::Handle<v8::Object> val,
-    HashToHandleMap* unique_map) const {
-  if (!UpdateAndCheckUniqueness(unique_map, val))
+    FromV8ValueState* state) const {
+  if (!state->UpdateAndCheckUniqueness(val))
     return base::Value::CreateNullValue();
   scoped_ptr<v8::Context::Scope> scope;
   // If val was created in a different context than our current one, change to
@@ -306,10 +378,6 @@ Value* V8ValueConverterImpl::FromV8Object(
       continue;
     }
 
-    // Skip all callbacks: crbug.com/139933
-    if (val->HasRealNamedCallbackProperty(key->ToString()))
-      continue;
-
     v8::String::Utf8Value name_utf8(key->ToString());
 
     v8::TryCatch try_catch;
@@ -321,7 +389,7 @@ Value* V8ValueConverterImpl::FromV8Object(
       child_v8 = v8::Null();
     }
 
-    scoped_ptr<base::Value> child(FromV8ValueImpl(child_v8, unique_map));
+    scoped_ptr<base::Value> child(FromV8ValueImpl(child_v8, state));
     if (!child)
       // JSON.stringify skips properties whose values don't serialize, for
       // example undefined and functions. Emulate that behavior.
@@ -357,24 +425,4 @@ Value* V8ValueConverterImpl::FromV8Object(
   return result.release();
 }
 
-bool V8ValueConverterImpl::UpdateAndCheckUniqueness(
-    HashToHandleMap* map,
-    v8::Handle<v8::Object> handle) const {
-  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 = 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;
-  }
-
-  map->insert(std::pair<int, v8::Handle<v8::Object> >(hash, handle));
-  return true;
-}
-
 }  // namespace content

content/renderer/v8_value_converter_impl.h

@@ -39,7 +39,8 @@ class CONTENT_EXPORT V8ValueConverterImpl : public V8ValueConverter {
 
  private:
   friend class ScopedAvoidIdentityHashForTesting;
-  typedef std::multimap<int, v8::Handle<v8::Object> > HashToHandleMap;
+
+  class FromV8ValueState;
 
   v8::Handle<v8::Value> ToV8ValueImpl(const base::Value* value) const;
   v8::Handle<v8::Value> ToV8Array(const base::ListValue* list) const;
@@ -48,9 +49,9 @@ class CONTENT_EXPORT V8ValueConverterImpl : public V8ValueConverter {
   v8::Handle<v8::Value> ToArrayBuffer(const base::BinaryValue* value) const;
 
   base::Value* FromV8ValueImpl(v8::Handle<v8::Value> value,
-                               HashToHandleMap* unique_map) const;
+                               FromV8ValueState* state) const;
   base::Value* FromV8Array(v8::Handle<v8::Array> array,
-                           HashToHandleMap* unique_map) const;
+                           FromV8ValueState* state) const;
 
   // This will convert objects of type ArrayBuffer or any of the
   // ArrayBufferView subclasses. The return value will be NULL if |value| is
@@ -58,15 +59,7 @@ class CONTENT_EXPORT V8ValueConverterImpl : public V8ValueConverter {
   base::BinaryValue* FromV8Buffer(v8::Handle<v8::Value> value) const;
 
   base::Value* FromV8Object(v8::Handle<v8::Object> object,
-                            HashToHandleMap* unique_map) const;
-
-  // If |handle| is not in |map|, then add it to |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(HashToHandleMap* map,
-                                v8::Handle<v8::Object> handle) const;
+                            FromV8ValueState* state) const;
 
   // If true, we will convert Date JavaScript objects to doubles.
   bool date_allowed_;

content/renderer/v8_value_converter_impl_unittest.cc

@@ -447,49 +447,6 @@ TEST_F(V8ValueConverterImplTest, RecursiveObjects) {
   EXPECT_TRUE(IsNull(list_result.get(), 1));
 }
 
-// Do not try and convert any named callbacks including getters.
-TEST_F(V8ValueConverterImplTest, ObjectGetters) {
-  v8::Context::Scope context_scope(context_);
-  v8::HandleScope handle_scope;
-
-  const char* source = "(function() {"
-      "var a = {};"
-      "a.__defineGetter__('foo', function() { return 'bar'; });"
-      "return a;"
-      "})();";
-
-  v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
-  v8::Handle<v8::Object> object = script->Run().As<v8::Object>();
-  ASSERT_FALSE(object.IsEmpty());
-
-  V8ValueConverterImpl converter;
-  scoped_ptr<base::DictionaryValue> result(
-      static_cast<base::DictionaryValue*>(
-          converter.FromV8Value(object, context_)));
-  ASSERT_TRUE(result.get());
-  EXPECT_EQ(0u, result->size());
-}
-
-// Do not try and convert any named callbacks including getters.
-TEST_F(V8ValueConverterImplTest, ObjectWithInternalFieldsGetters) {
-  v8::Context::Scope context_scope(context_);
-  v8::HandleScope handle_scope;
-
-  v8::Handle<v8::ObjectTemplate> object_template = v8::ObjectTemplate::New();
-  object_template->SetInternalFieldCount(1);
-  object_template->SetAccessor(v8::String::New("foo"), NamedCallbackGetter);
-  v8::Handle<v8::Object> object = object_template->NewInstance();
-  ASSERT_FALSE(object.IsEmpty());
-  object->Set(v8::String::New("a"), v8::String::New("b"));
-
-  V8ValueConverterImpl converter;
-  scoped_ptr<base::DictionaryValue> result(
-      static_cast<base::DictionaryValue*>(
-          converter.FromV8Value(object, context_)));
-  ASSERT_TRUE(result.get());
-  EXPECT_EQ(1u, result->size());
-}
-
 TEST_F(V8ValueConverterImplTest, WeirdProperties) {
   v8::Context::Scope context_scope(context_);
   v8::HandleScope handle_scope;
@@ -650,4 +607,40 @@ TEST_F(V8ValueConverterImplTest, DetectCycles) {
   EXPECT_TRUE(expected_dictionary.Equals(actual_dictionary.get()));
 }
 
+TEST_F(V8ValueConverterImplTest, MaxRecursionDepth) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  // Must larger than kMaxRecursionDepth in v8_value_converter_impl.cc.
+  int kDepth = 100;
+  const char kKey[] = "key";
+
+  v8::Local<v8::Object> deep_object = v8::Object::New();
+
+  v8::Local<v8::Object> leaf = deep_object;
+  for (int i = 0; i < kDepth; ++i) {
+    v8::Local<v8::Object> new_object = v8::Object::New();
+    leaf->Set(v8::String::New(kKey), new_object);
+    leaf = new_object;
+  }
+
+  V8ValueConverterImpl converter;
+  scoped_ptr<base::Value> value(converter.FromV8Value(deep_object, context_));
+  ASSERT_TRUE(value);
+
+  // Expected depth is kMaxRecursionDepth in v8_value_converter_impl.cc.
+  int kExpectedDepth = 10;
+
+  base::Value* current = value.get();
+  for (int i = 1; i < kExpectedDepth; ++i) {
+    base::DictionaryValue* current_as_object = NULL;
+    ASSERT_TRUE(current->GetAsDictionary(&current_as_object)) << i;
+    ASSERT_TRUE(current_as_object->Get(kKey, &current)) << i;
+  }
+
+  // The leaf node shouldn't have any properties.
+  base::DictionaryValue empty;
+  EXPECT_TRUE(Value::Equals(&empty, current)) << *current;
+}
+
 }  // namespace content