Forces V8ValueConverter to switch context when converting objects.

Objects passed into the V8ValueConverter may have fields that reference
objects in a v8::Context different than the one that was passed in.
This is very common for objects that were created in an isolated world and
reference globals in that world (such as window). This fix will check to see if
the current v8::Context is the same as the converting objects CreationContext().
If they are different, they will be swaped during the object's conversion.

In addition, the V8ValueConverter has been expanded to have the ability to omit duplicate
fileds in an object. This is very important when something with a graph structure (like a DOM
node) is converted.

Extra checks for integer/string fields have been added as in respone to Jakob's observation:
https://code.google.com/p/chromium/issues/detail?id=134661#c5

BUG=134661, 135104
TEST=content_unittests::V8ValueConverterImplTest.RecursiveObject

Review URL: https://chromiumcodereview.appspot.com/10704030

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

4 files changed, 129 insertions, 24 deletions

diff --git a/content/renderer/v8_value_converter_impl.cc b/content/renderer/v8_value_converter_impl.cc
index 993ee37..20923cc 100644
--- a/content/renderer/v8_value_converter_impl.cc
+++ b/content/renderer/v8_value_converter_impl.cc
@@ -19,14 +19,12 @@ using base::ListValue;
 using base::StringValue;
 using base::Value;
 
-
 namespace content {
 
 V8ValueConverter* V8ValueConverter::create() {
   return new V8ValueConverterImpl();
 }
-
-}
+}  // namespace content
 
 V8ValueConverterImpl::V8ValueConverterImpl()
     : undefined_allowed_(false),
@@ -79,7 +77,8 @@ Value* V8ValueConverterImpl::FromV8Value(
     v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
   v8::HandleScope handle_scope;
-  return FromV8ValueImpl(val);
+  std::set<int> unique_set;
+  return FromV8ValueImpl(val, &unique_set);
 }
 
 v8::Handle<v8::Value> V8ValueConverterImpl::ToV8ValueImpl(
@@ -180,7 +179,8 @@ v8::Handle<v8::Value> V8ValueConverterImpl::ToArrayBuffer(
   return buffer.toV8Value();
 }
 
-Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val) const {
+Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
+    std::set<int>* unique_set) const {
   CHECK(!val.IsEmpty());
 
   if (val->IsNull())
@@ -215,22 +215,37 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val) const {
 
   // v8::Value doesn't have a ToArray() method for some reason.
   if (val->IsArray())
-    return FromV8Array(val.As<v8::Array>());
+    return FromV8Array(val.As<v8::Array>(), unique_set);
 
   if (val->IsObject()) {
     BinaryValue* binary_value = FromV8Buffer(val);
     if (binary_value) {
       return binary_value;
     } else {
-      return FromV8Object(val->ToObject());
+      return FromV8Object(val->ToObject(), unique_set);
     }
   }
   LOG(ERROR) << "Unexpected v8 value type encountered.";
   return Value::CreateNullValue();
 }
 
-ListValue* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val) const {
+Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
+    std::set<int>* unique_set) const {
+  if (unique_set && unique_set->count(val->GetIdentityHash()))
+    return Value::CreateNullValue();
+
+  scoped_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() != v8::Context::GetCurrent())
+    scope.reset(new v8::Context::Scope(val->CreationContext()));
+
   ListValue* result = new ListValue();
+
+  if (unique_set)
+    unique_set->insert(val->GetIdentityHash());
+  // Only fields with integer keys are carried over to the ListValue.
   for (uint32 i = 0; i < val->Length(); ++i) {
     v8::TryCatch try_catch;
     v8::Handle<v8::Value> child_v8 = val->Get(i);
@@ -239,12 +254,10 @@ ListValue* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val) const {
       child_v8 = v8::Null();
     }
 
-    // TODO(aa): It would be nice to support getters, but we need
-    // http://code.google.com/p/v8/issues/detail?id=1342 to do it properly.
     if (!val->HasRealIndexedProperty(i))
       continue;
 
-    Value* child = FromV8ValueImpl(child_v8);
+    Value* child = FromV8ValueImpl(child_v8, unique_set);
     CHECK(child);
 
     result->Append(child);
@@ -277,29 +290,42 @@ base::BinaryValue* V8ValueConverterImpl::FromV8Buffer(
     return NULL;
 }
 
-DictionaryValue* V8ValueConverterImpl::FromV8Object(
-    v8::Handle<v8::Object> val) const {
+Value* V8ValueConverterImpl::FromV8Object(
+    v8::Handle<v8::Object> val,
+    std::set<int>* unique_set) const {
+  if (unique_set && unique_set->count(val->GetIdentityHash()))
+    return Value::CreateNullValue();
+  scoped_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() != v8::Context::GetCurrent())
+    scope.reset(new v8::Context::Scope(val->CreationContext()));
+
   scoped_ptr<DictionaryValue> result(new DictionaryValue());
   v8::Handle<v8::Array> property_names(val->GetPropertyNames());
+
+  if (unique_set)
+    unique_set->insert(val->GetIdentityHash());
+
   for (uint32 i = 0; i < property_names->Length(); ++i) {
-    v8::Handle<v8::String> name(property_names->Get(i).As<v8::String>());
+    v8::Handle<v8::Value> key(property_names->Get(i));
 
-    // TODO(aa): It would be nice to support getters, but we need
-    // http://code.google.com/p/v8/issues/detail?id=1342 to do it properly.
-    if (!val->HasRealNamedProperty(name))
+    if (!key->IsString() || !val->HasRealNamedProperty(key->ToString()))
       continue;
 
-    v8::String::Utf8Value name_utf8(name->ToString());
+    v8::String::Utf8Value name_utf8(key->ToString());
 
     v8::TryCatch try_catch;
-    v8::Handle<v8::Value> child_v8 = val->Get(name);
+    v8::Handle<v8::Value> child_v8 = val->Get(key);
+
     if (try_catch.HasCaught()) {
       LOG(ERROR) << "Getter for property " << *name_utf8
                  << " threw an exception.";
       child_v8 = v8::Null();
     }
 
-    scoped_ptr<Value> child(FromV8ValueImpl(child_v8));
+    scoped_ptr<Value> child(FromV8ValueImpl(child_v8, unique_set));
     CHECK(child.get());
 
     // Strip null if asked (and since undefined is turned into null, undefined
@@ -328,5 +354,6 @@ DictionaryValue* V8ValueConverterImpl::FromV8Object(
     result->SetWithoutPathExpansion(std::string(*name_utf8, name_utf8.length()),
                                     child.release());
   }
+
   return result.release();
 }
diff --git a/content/renderer/v8_value_converter_impl.h b/content/renderer/v8_value_converter_impl.h
index 7f75899..baf061a 100644
--- a/content/renderer/v8_value_converter_impl.h
+++ b/content/renderer/v8_value_converter_impl.h
@@ -5,6 +5,8 @@
 #ifndef CONTENT_RENDERER_V8_VALUE_CONVERTER_IMPL_H_
 #define CONTENT_RENDERER_V8_VALUE_CONVERTER_IMPL_H_
 
+#include <set>
+
 #include "base/compiler_specific.h"
 #include "content/common/content_export.h"
 #include "content/public/renderer/v8_value_converter.h"
@@ -43,15 +45,18 @@ class CONTENT_EXPORT V8ValueConverterImpl : public content::V8ValueConverter {
       const base::DictionaryValue* dictionary) const;
   v8::Handle<v8::Value> ToArrayBuffer(const base::BinaryValue* value) const;
 
-  base::Value* FromV8ValueImpl(v8::Handle<v8::Value> value) const;
-  base::ListValue* FromV8Array(v8::Handle<v8::Array> array) const;
+  base::Value* FromV8ValueImpl(v8::Handle<v8::Value> value,
+                               std::set<int>* unique_set) const;
+  base::Value* FromV8Array(v8::Handle<v8::Array> array,
+                           std::set<int>* unique_set) const;
 
   // This will convert objects of type ArrayBuffer or any of the
   // ArrayBufferView subclasses. The return value will be NULL if |value| is
   // not one of these types.
   base::BinaryValue* FromV8Buffer(v8::Handle<v8::Value> value) const;
 
-  base::DictionaryValue* FromV8Object(v8::Handle<v8::Object> object) const;
+  base::Value* FromV8Object(v8::Handle<v8::Object> object,
+                            std::set<int>* unique_set) const;
 
   // If true, we will convert undefined JavaScript values to null.
   bool undefined_allowed_;
diff --git a/content/renderer/v8_value_converter_impl_unittest.cc b/content/renderer/v8_value_converter_impl_unittest.cc
index a56941a..a4e461f 100644
--- a/content/renderer/v8_value_converter_impl_unittest.cc
+++ b/content/renderer/v8_value_converter_impl_unittest.cc
@@ -113,6 +113,7 @@ class V8ValueConverterImplTest : public testing::Test {
         static_cast<DictionaryValue*>(
             converter.FromV8Value(object, context_)));
     ASSERT_TRUE(dictionary.get());
+
     Value* temp = NULL;
     ASSERT_TRUE(dictionary->Get("test", &temp));
     EXPECT_EQ(expected_type, temp->GetType());
@@ -346,3 +347,74 @@ TEST_F(V8ValueConverterImplTest, StripNullFromObjects) {
   ASSERT_TRUE(result.get());
   EXPECT_EQ(0u, result->size());
 }
+
+TEST_F(V8ValueConverterImplTest, RecursiveObjects) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  V8ValueConverterImpl converter;
+
+  v8::Handle<v8::Object> object = v8::Object::New().As<v8::Object>();
+  ASSERT_FALSE(object.IsEmpty());
+  object->Set(v8::String::New("foo"), v8::String::New("bar"));
+  object->Set(v8::String::New("obj"), object);
+
+  scoped_ptr<DictionaryValue> object_result(
+      static_cast<DictionaryValue*>(converter.FromV8Value(object, context_)));
+  ASSERT_TRUE(object_result.get());
+  EXPECT_EQ(2u, object_result->size());
+  EXPECT_TRUE(IsNull(object_result.get(), "obj"));
+
+  v8::Handle<v8::Array> array = v8::Array::New().As<v8::Array>();
+  ASSERT_FALSE(array.IsEmpty());
+  array->Set(0, v8::String::New("1"));
+  array->Set(1, array);
+
+  scoped_ptr<ListValue> list_result(
+      static_cast<ListValue*>(converter.FromV8Value(array, context_)));
+  ASSERT_TRUE(list_result.get());
+  EXPECT_EQ(2u, list_result->GetSize());
+  EXPECT_TRUE(IsNull(list_result.get(), 1));
+}
+
+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<DictionaryValue> result(
+      static_cast<DictionaryValue*>(converter.FromV8Value(object, context_)));
+  ASSERT_TRUE(result.get());
+  EXPECT_EQ(1u, result->size());
+}
+
+TEST_F(V8ValueConverterImplTest, ArrayGetters) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  const char* source = "(function() {"
+      "var a = [0];"
+      "a.__defineGetter__(1, function() { return 'bar'; });"
+      "return a;"
+      "})();";
+
+  v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
+  v8::Handle<v8::Array> array = script->Run().As<v8::Array>();
+  ASSERT_FALSE(array.IsEmpty());
+
+  V8ValueConverterImpl converter;
+  scoped_ptr<ListValue> result(
+      static_cast<ListValue*>(converter.FromV8Value(array, context_)));
+  ASSERT_TRUE(result.get());
+  EXPECT_EQ(2u, result->GetSize());
+}
diff --git a/content/test/unittest_test_suite.cc b/content/test/unittest_test_suite.cc
index 85597d5..fdcd49a 100644
--- a/content/test/unittest_test_suite.cc
+++ b/content/test/unittest_test_suite.cc
@@ -5,6 +5,7 @@
 #include "content/public/test/unittest_test_suite.h"
 
 #include "base/logging.h"
+#include "base/rand_util.h"
 #include "base/test/test_suite.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebKit.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h"
@@ -19,7 +20,7 @@ class UnitTestTestSuite::UnitTestWebKitPlatformSupport
   virtual ~UnitTestWebKitPlatformSupport() {}
   virtual void cryptographicallyRandomValues(unsigned char* buffer,
                                              size_t length) OVERRIDE {
-    memset(buffer, 0, length);
+    base::RandBytes(buffer, length);
   }
   virtual const unsigned char* getTraceCategoryEnabledFlag(
       const char* categoryName) {