Revert 146852 - Forces V8ValueConverter to switch context when converting objects.

[This is a speculative revert. This is the largest patch in the blamelist and we have no idea how to proceed.

http://build.chromium.org/p/chromium.memory/builders/Chromium%20OS%20ASAN%20Tests%20%281%29/builds/19

]

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

TBR=eaugusti@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10789024

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

3 files changed, 23 insertions, 127 deletions

diff --git a/content/renderer/v8_value_converter_impl.cc b/content/renderer/v8_value_converter_impl.cc
index 20923cc..993ee37 100644
--- a/content/renderer/v8_value_converter_impl.cc
+++ b/content/renderer/v8_value_converter_impl.cc
@@ -19,12 +19,14 @@ using base::ListValue;
 using base::StringValue;
 using base::Value;
 
+
 namespace content {
 
 V8ValueConverter* V8ValueConverter::create() {
   return new V8ValueConverterImpl();
 }
-}  // namespace content
+
+}
 
 V8ValueConverterImpl::V8ValueConverterImpl()
     : undefined_allowed_(false),
@@ -77,8 +79,7 @@ Value* V8ValueConverterImpl::FromV8Value(
     v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
   v8::HandleScope handle_scope;
-  std::set<int> unique_set;
-  return FromV8ValueImpl(val, &unique_set);
+  return FromV8ValueImpl(val);
 }
 
 v8::Handle<v8::Value> V8ValueConverterImpl::ToV8ValueImpl(
@@ -179,8 +180,7 @@ v8::Handle<v8::Value> V8ValueConverterImpl::ToArrayBuffer(
   return buffer.toV8Value();
 }
 
-Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
-    std::set<int>* unique_set) const {
+Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val) const {
   CHECK(!val.IsEmpty());
 
   if (val->IsNull())
@@ -215,37 +215,22 @@ Value* V8ValueConverterImpl::FromV8ValueImpl(v8::Handle<v8::Value> val,
 
   // v8::Value doesn't have a ToArray() method for some reason.
   if (val->IsArray())
-    return FromV8Array(val.As<v8::Array>(), unique_set);
+    return FromV8Array(val.As<v8::Array>());
 
   if (val->IsObject()) {
     BinaryValue* binary_value = FromV8Buffer(val);
     if (binary_value) {
       return binary_value;
     } else {
-      return FromV8Object(val->ToObject(), unique_set);
+      return FromV8Object(val->ToObject());
     }
   }
   LOG(ERROR) << "Unexpected v8 value type encountered.";
   return Value::CreateNullValue();
 }
 
-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* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val) const {
   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);
@@ -254,10 +239,12 @@ Value* V8ValueConverterImpl::FromV8Array(v8::Handle<v8::Array> val,
       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, unique_set);
+    Value* child = FromV8ValueImpl(child_v8);
     CHECK(child);
 
     result->Append(child);
@@ -290,42 +277,29 @@ base::BinaryValue* V8ValueConverterImpl::FromV8Buffer(
     return NULL;
 }
 
-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()));
-
+DictionaryValue* V8ValueConverterImpl::FromV8Object(
+    v8::Handle<v8::Object> val) const {
   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::Value> key(property_names->Get(i));
+    v8::Handle<v8::String> name(property_names->Get(i).As<v8::String>());
 
-    if (!key->IsString() || !val->HasRealNamedProperty(key->ToString()))
+    // 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))
       continue;
 
-    v8::String::Utf8Value name_utf8(key->ToString());
+    v8::String::Utf8Value name_utf8(name->ToString());
 
     v8::TryCatch try_catch;
-    v8::Handle<v8::Value> child_v8 = val->Get(key);
-
+    v8::Handle<v8::Value> child_v8 = val->Get(name);
     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, unique_set));
+    scoped_ptr<Value> child(FromV8ValueImpl(child_v8));
     CHECK(child.get());
 
     // Strip null if asked (and since undefined is turned into null, undefined
@@ -354,6 +328,5 @@ Value* 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 baf061a..7f75899 100644
--- a/content/renderer/v8_value_converter_impl.h
+++ b/content/renderer/v8_value_converter_impl.h
@@ -5,8 +5,6 @@
 #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"
@@ -45,18 +43,15 @@ 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,
-                               std::set<int>* unique_set) const;
-  base::Value* FromV8Array(v8::Handle<v8::Array> array,
-                           std::set<int>* unique_set) const;
+  base::Value* FromV8ValueImpl(v8::Handle<v8::Value> value) const;
+  base::ListValue* FromV8Array(v8::Handle<v8::Array> array) 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::Value* FromV8Object(v8::Handle<v8::Object> object,
-                            std::set<int>* unique_set) const;
+  base::DictionaryValue* FromV8Object(v8::Handle<v8::Object> object) 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 a4e461f..a56941a 100644
--- a/content/renderer/v8_value_converter_impl_unittest.cc
+++ b/content/renderer/v8_value_converter_impl_unittest.cc
@@ -113,7 +113,6 @@ 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());
@@ -347,74 +346,3 @@ 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());
-}