Revert "Revert 82671 - V8ValueConverter: Handle exceptions when dealing with objectproperties.Review URL: http://codereview.chromium.org/6878086"

This reverts commit 766b05b73a906cdcba68914a61dff49f398d87ad.

TBR=joshia@chromium.org

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

3 files changed, 332 insertions, 38 deletions

diff --git a/content/renderer/v8_value_converter.cc b/content/renderer/v8_value_converter.cc
index dcbf8db..cefbea8 100644
--- a/content/renderer/v8_value_converter.cc
+++ b/content/renderer/v8_value_converter.cc
@@ -11,22 +11,27 @@
 #include "base/values.h"
 #include "v8/include/v8.h"
 
-V8ValueConverter::V8ValueConverter() {
+V8ValueConverter::V8ValueConverter()
+    : allow_undefined_(false),
+      allow_date_(false),
+      allow_regexp_(false) {
 }
 
 v8::Handle<v8::Value> V8ValueConverter::ToV8Value(
-    Value* value, v8::Handle<v8::Context> context) {
+    Value* value, v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
-  return ToV8ValueImpl(value);
+  v8::HandleScope handle_scope;
+  return handle_scope.Close(ToV8ValueImpl(value));
 }
 
 Value* V8ValueConverter::FromV8Value(v8::Handle<v8::Value> val,
-                                     v8::Handle<v8::Context> context) {
+                                     v8::Handle<v8::Context> context) const {
   v8::Context::Scope context_scope(context);
+  v8::HandleScope handle_scope;
   return FromV8ValueImpl(val);
 }
 
-v8::Handle<v8::Value> V8ValueConverter::ToV8ValueImpl(Value* value) {
+v8::Handle<v8::Value> V8ValueConverter::ToV8ValueImpl(Value* value) const {
   CHECK(value);
   switch (value->GetType()) {
     case Value::TYPE_NULL:
@@ -63,39 +68,56 @@ v8::Handle<v8::Value> V8ValueConverter::ToV8ValueImpl(Value* value) {
       return ToV8Object(static_cast<DictionaryValue*>(value));
 
     default:
-      NOTREACHED() << "Unexpected value type: " << value->GetType();
+      LOG(ERROR) << "Unexpected value type: " << value->GetType();
       return v8::Null();
   }
 }
 
-v8::Handle<v8::Value> V8ValueConverter::ToV8Array(ListValue* val) {
+v8::Handle<v8::Value> V8ValueConverter::ToV8Array(ListValue* val) const {
   v8::Handle<v8::Array> result(v8::Array::New(val->GetSize()));
 
   for (size_t i = 0; i < val->GetSize(); ++i) {
     Value* child = NULL;
     CHECK(val->Get(i, &child));
-    result->Set(static_cast<uint32>(i), ToV8ValueImpl(child));
+
+    v8::Handle<v8::Value> child_v8 = ToV8ValueImpl(child);
+    CHECK(!child_v8.IsEmpty());
+
+    v8::TryCatch try_catch;
+    result->Set(static_cast<uint32>(i), child_v8);
+    if (try_catch.HasCaught())
+      LOG(ERROR) << "Setter for index " << i << " threw an exception.";
   }
 
   return result;
 }
 
-v8::Handle<v8::Value> V8ValueConverter::ToV8Object(DictionaryValue* val) {
+v8::Handle<v8::Value> V8ValueConverter::ToV8Object(DictionaryValue* val) const {
   v8::Handle<v8::Object> result(v8::Object::New());
 
   for (DictionaryValue::key_iterator iter = val->begin_keys();
        iter != val->end_keys(); ++iter) {
     Value* child = NULL;
     CHECK(val->GetWithoutPathExpansion(*iter, &child));
+
     const std::string& key = *iter;
-    result->Set(v8::String::New(key.c_str(), key.length()),
-                ToV8ValueImpl(child));
+    v8::Handle<v8::Value> child_v8 = ToV8ValueImpl(child);
+    CHECK(!child_v8.IsEmpty());
+
+    v8::TryCatch try_catch;
+    result->Set(v8::String::New(key.c_str(), key.length()), child_v8);
+    if (try_catch.HasCaught()) {
+      LOG(ERROR) << "Setter for property " << key.c_str() << " threw an "
+                 << "exception.";
+    }
   }
 
   return result;
 }
 
-Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) {
+Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) const {
+  CHECK(!val.IsEmpty());
+
   if (val->IsNull())
     return Value::CreateNullValue();
 
@@ -113,6 +135,9 @@ Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) {
     return Value::CreateStringValue(std::string(*utf8, utf8.length()));
   }
 
+  if (allow_undefined_ && val->IsUndefined())
+    return Value::CreateNullValue();
+
   if (allow_date_ && val->IsDate()) {
     v8::Date* date = v8::Date::Cast(*val);
     return Value::CreateDoubleValue(date->NumberValue() / 1000.0);
@@ -125,32 +150,65 @@ Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) {
 
   // v8::Value doesn't have a ToArray() method for some reason.
   if (val->IsArray())
-    return FromV8Array(v8::Handle<v8::Array>::Cast(val));
+    return FromV8Array(val.As<v8::Array>());
 
   if (val->IsObject())
     return FromV8Object(val->ToObject());
 
-  NOTREACHED() << "Unexpected v8::Value type.";
+  LOG(ERROR) << "Unexpected v8 value type encountered.";
   return Value::CreateNullValue();
 }
 
-ListValue* V8ValueConverter::FromV8Array(v8::Handle<v8::Array> val) {
+ListValue* V8ValueConverter::FromV8Array(v8::Handle<v8::Array> val) const {
   ListValue* result = new ListValue();
   for (uint32 i = 0; i < val->Length(); ++i) {
-    result->Append(FromV8ValueImpl(val->Get(i)));
+    v8::TryCatch try_catch;
+    v8::Handle<v8::Value> child_v8 = val->Get(i);
+    if (try_catch.HasCaught()) {
+      LOG(ERROR) << "Getter for index " << i << " threw an exception.";
+      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);
+    CHECK(child);
+
+    result->Append(child);
   }
   return result;
 }
 
-DictionaryValue* V8ValueConverter::FromV8Object(v8::Handle<v8::Object> val) {
+DictionaryValue* V8ValueConverter::FromV8Object(
+    v8::Handle<v8::Object> val) const {
   DictionaryValue* result = new DictionaryValue();
   v8::Handle<v8::Array> property_names(val->GetPropertyNames());
   for (uint32 i = 0; i < property_names->Length(); ++i) {
-    v8::Handle<v8::String> name(
-        v8::Handle<v8::String>::Cast(property_names->Get(i)));
+    v8::Handle<v8::String> name(property_names->Get(i).As<v8::String>());
+
+    // 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(name->ToString());
+
+    v8::TryCatch try_catch;
+    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();
+    }
+
+    Value* child = FromV8ValueImpl(child_v8);
+    CHECK(child);
+
     result->SetWithoutPathExpansion(std::string(*name_utf8, name_utf8.length()),
-                                    FromV8ValueImpl(val->Get(name)));
+                                    child);
   }
   return result;
 }
diff --git a/content/renderer/v8_value_converter.h b/content/renderer/v8_value_converter.h
index 4bfce75..acb2d0a 100644
--- a/content/renderer/v8_value_converter.h
+++ b/content/renderer/v8_value_converter.h
@@ -14,37 +14,46 @@ class DictionaryValue;
 // Converts between v8::Value (JavaScript values in the v8 heap) and Chrome's
 // values (from base/values.h). Lists and dictionaries are converted
 // recursively.
+//
+// Only the JSON types (null, boolean, string, number, array, and object) are
+// supported by default. Additional types can be allowed with e.g.
+// set_allow_date(), set_allow_regexp().
 class V8ValueConverter {
  public:
   V8ValueConverter();
 
+  bool allow_undefined() const { return allow_undefined_; }
+  void set_allow_undefined(bool val) { allow_undefined_ = val; }
+
   bool allow_date() const { return allow_date_; }
   void set_allow_date(bool val) { allow_date_ = val; }
 
   bool allow_regexp() const { return allow_regexp_; }
   void set_allow_regexp(bool val) { allow_regexp_ = val; }
 
-  // Converts Value to v8::Value. Only the JSON types (null, boolean, string,
-  // number, array, and object) are supported. Other types DCHECK and are
-  // replaced with null.
+  // Converts Value to v8::Value. Unsupported types are replaced with null.
+  // If an array or object throws while setting a value, that property or item
+  // is skipped, leaving a hole in the case of arrays.
   v8::Handle<v8::Value> ToV8Value(Value* value,
-                                  v8::Handle<v8::Context> context);
+                                  v8::Handle<v8::Context> context) const;
 
-  // Converts v8::Value to Value. Only the JSON types (null, boolean, string,
-  // number, array, and object) are supported by default, but additional types
-  // can be supported with setters. If a number fits in int32, we will convert
-  // to Value::TYPE_INTEGER instead of Value::TYPE_DOUBLE.
+  // Converts v8::Value to Value. Unsupported types are replaced with null.
+  // If an array or object throws while getting a value, that property or item
+  // is replaced with null.
   Value* FromV8Value(v8::Handle<v8::Value> value,
-                     v8::Handle<v8::Context> context);
+                     v8::Handle<v8::Context> context) const;
 
  private:
-  v8::Handle<v8::Value> ToV8ValueImpl(Value* value);
-  v8::Handle<v8::Value> ToV8Array(ListValue* list);
-  v8::Handle<v8::Value> ToV8Object(DictionaryValue* dictionary);
+  v8::Handle<v8::Value> ToV8ValueImpl(Value* value) const;
+  v8::Handle<v8::Value> ToV8Array(ListValue* list) const;
+  v8::Handle<v8::Value> ToV8Object(DictionaryValue* dictionary) const;
+
+  Value* FromV8ValueImpl(v8::Handle<v8::Value> value) const;
+  ListValue* FromV8Array(v8::Handle<v8::Array> array) const;
+  DictionaryValue* FromV8Object(v8::Handle<v8::Object> object) const;
 
-  Value* FromV8ValueImpl(v8::Handle<v8::Value> value);
-  ListValue* FromV8Array(v8::Handle<v8::Array> array);
-  DictionaryValue* FromV8Object(v8::Handle<v8::Object> object);
+  // If true, we will convert undefined JavaScript values to null.
+  bool allow_undefined_;
 
   // If true, we will convert Date JavaScript objects to doubles.
   bool allow_date_;
diff --git a/content/renderer/v8_value_converter_browsertest.cc b/content/renderer/v8_value_converter_browsertest.cc
index 769b281..650927f 100644
--- a/content/renderer/v8_value_converter_browsertest.cc
+++ b/content/renderer/v8_value_converter_browsertest.cc
@@ -22,6 +22,115 @@ class V8ValueConverterTest : public testing::Test {
     context_.Dispose();
   }
 
+  std::string GetString(DictionaryValue* value, const std::string& key) {
+    std::string temp;
+    if (!value->GetString(key, &temp)) {
+      ADD_FAILURE();
+      return "";
+    }
+    return temp;
+  }
+
+  std::string GetString(v8::Handle<v8::Object> value, const std::string& key) {
+    v8::Handle<v8::String> temp =
+        value->Get(v8::String::New(key.c_str())).As<v8::String>();
+    if (temp.IsEmpty()) {
+      ADD_FAILURE();
+      return "";
+    }
+    v8::String::Utf8Value utf8(temp);
+    return std::string(*utf8, utf8.length());
+  }
+
+  std::string GetString(ListValue* value, uint32 index) {
+    std::string temp;
+    if (!value->GetString(static_cast<size_t>(index), &temp)) {
+      ADD_FAILURE();
+      return "";
+    }
+    return temp;
+  }
+
+  std::string GetString(v8::Handle<v8::Array> value, uint32 index) {
+    v8::Handle<v8::String> temp = value->Get(index).As<v8::String>();
+    if (temp.IsEmpty()) {
+      ADD_FAILURE();
+      return "";
+    }
+    v8::String::Utf8Value utf8(temp);
+    return std::string(*utf8, utf8.length());
+  }
+
+  bool IsNull(DictionaryValue* value, const std::string& key) {
+    Value* child = NULL;
+    if (!value->Get(key, &child)) {
+      ADD_FAILURE();
+      return false;
+    }
+    return child->GetType() == Value::TYPE_NULL;
+  }
+
+  bool IsNull(v8::Handle<v8::Object> value, const std::string& key) {
+    v8::Handle<v8::Value> child = value->Get(v8::String::New(key.c_str()));
+    if (child.IsEmpty()) {
+      ADD_FAILURE();
+      return false;
+    }
+    return child->IsNull();
+  }
+
+  bool IsNull(ListValue* value, uint32 index) {
+    Value* child = NULL;
+    if (!value->Get(static_cast<size_t>(index), &child)) {
+      ADD_FAILURE();
+      return false;
+    }
+    return child->GetType() == Value::TYPE_NULL;
+  }
+
+  bool IsNull(v8::Handle<v8::Array> value, uint32 index) {
+    v8::Handle<v8::Value> child = value->Get(index);
+    if (child.IsEmpty()) {
+      ADD_FAILURE();
+      return false;
+    }
+    return child->IsNull();
+  }
+
+  void TestWeirdType(const V8ValueConverter& converter,
+                     v8::Handle<v8::Value> val,
+                     Value::ValueType expected_type,
+                     Value* expected_value) {
+    scoped_ptr<Value> raw(converter.FromV8Value(val, context_));
+    ASSERT_TRUE(raw.get());
+    EXPECT_EQ(expected_type, raw->GetType());
+    if (expected_value)
+      EXPECT_TRUE(expected_value->Equals(raw.get()));
+
+    v8::Handle<v8::Object> object(v8::Object::New());
+    object->Set(v8::String::New("test"), val);
+    scoped_ptr<DictionaryValue> dictionary(
+        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());
+    if (expected_value)
+      EXPECT_TRUE(expected_value->Equals(temp));
+
+    v8::Handle<v8::Array> array(v8::Array::New());
+    array->Set(0, val);
+    scoped_ptr<ListValue> list(
+        static_cast<ListValue*>(
+            converter.FromV8Value(array, context_)));
+    ASSERT_TRUE(list.get());
+    ASSERT_TRUE(list->Get(0, &temp));
+    EXPECT_EQ(expected_type, temp->GetType());
+    if (expected_value)
+      EXPECT_TRUE(expected_value->Equals(temp));
+  }
+
   // Context for the JavaScript in the test.
   v8::Persistent<v8::Context> context_;
 };
@@ -56,9 +165,8 @@ TEST_F(V8ValueConverterTest, BasicRoundTrip) {
   v8::HandleScope handle_scope;
 
   V8ValueConverter converter;
-  v8::Handle<v8::Object> v8_object(
-      v8::Handle<v8::Object>::Cast(
-          converter.ToV8Value(&original_root, context_)));
+  v8::Handle<v8::Object> v8_object =
+      converter.ToV8Value(&original_root, context_).As<v8::Object>();
   ASSERT_FALSE(v8_object.IsEmpty());
 
   EXPECT_EQ(original_root.size(), v8_object->GetPropertyNames()->Length());
@@ -97,3 +205,122 @@ TEST_F(V8ValueConverterTest, KeysWithDots) {
 
   EXPECT_TRUE(original.Equals(copy.get()));
 }
+
+TEST_F(V8ValueConverterTest, ObjectExceptions) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  // Set up objects to throw when reading or writing 'foo'.
+  const char* source =
+      "Object.prototype.__defineSetter__('foo', "
+      "    function() { throw new Error('muah!'); });"
+      "Object.prototype.__defineGetter__('foo', "
+      "    function() { throw new Error('muah!'); });";
+
+  v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
+  script->Run();
+
+  v8::Handle<v8::Object> object(v8::Object::New());
+  object->Set(v8::String::New("bar"), v8::String::New("bar"));
+
+  // Converting from v8 value should replace the foo property with null.
+  V8ValueConverter converter;
+  scoped_ptr<DictionaryValue> converted(static_cast<DictionaryValue*>(
+      converter.FromV8Value(object, context_)));
+  EXPECT_TRUE(converted.get());
+  // http://code.google.com/p/v8/issues/detail?id=1342
+  // EXPECT_EQ(2u, converted->size());
+  // EXPECT_TRUE(IsNull(converted.get(), "foo"));
+  EXPECT_EQ(1u, converted->size());
+  EXPECT_EQ("bar", GetString(converted.get(), "bar"));
+
+  // Converting to v8 value should drop the foo property.
+  converted->SetString("foo", "foo");
+  v8::Handle<v8::Object> copy =
+      converter.ToV8Value(converted.get(), context_).As<v8::Object>();
+  EXPECT_FALSE(copy.IsEmpty());
+  EXPECT_EQ(2u, copy->GetPropertyNames()->Length());
+  EXPECT_EQ("bar", GetString(copy, "bar"));
+}
+
+TEST_F(V8ValueConverterTest, ArrayExceptions) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  const char* source = "(function() {"
+      "var arr = [];"
+      "arr.__defineSetter__(0, "
+      "    function() { throw new Error('muah!'); });"
+      "arr.__defineGetter__(0, "
+      "    function() { throw new Error('muah!'); });"
+      "arr[1] = 'bar';"
+      "return arr;"
+      "})();";
+
+  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());
+
+  // Converting from v8 value should replace the first item with null.
+  V8ValueConverter converter;
+  scoped_ptr<ListValue> converted(static_cast<ListValue*>(
+      converter.FromV8Value(array, context_)));
+  ASSERT_TRUE(converted.get());
+  // http://code.google.com/p/v8/issues/detail?id=1342
+  EXPECT_EQ(2u, converted->GetSize());
+  EXPECT_TRUE(IsNull(converted.get(), 0));
+
+  // Converting to v8 value should drop the first item and leave a hole.
+  converted.reset(new ListValue());
+  converted->Append(Value::CreateStringValue("foo"));
+  converted->Append(Value::CreateStringValue("bar"));
+  v8::Handle<v8::Array> copy =
+      converter.ToV8Value(converted.get(), context_).As<v8::Array>();
+  ASSERT_FALSE(copy.IsEmpty());
+  EXPECT_EQ(2u, copy->Length());
+  EXPECT_EQ("bar", GetString(copy, 1));
+}
+
+TEST_F(V8ValueConverterTest, WeirdTypes) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  v8::Handle<v8::RegExp> regex(
+      v8::RegExp::New(v8::String::New("."), v8::RegExp::kNone));
+
+  V8ValueConverter converter;
+  TestWeirdType(converter, v8::Undefined(), Value::TYPE_NULL, NULL);
+  TestWeirdType(converter, v8::Date::New(1000), Value::TYPE_DICTIONARY, NULL);
+  TestWeirdType(converter, regex, Value::TYPE_DICTIONARY, NULL);
+
+  converter.set_allow_undefined(true);
+  TestWeirdType(converter, v8::Undefined(), Value::TYPE_NULL, NULL);
+
+  converter.set_allow_date(true);
+  TestWeirdType(converter, v8::Date::New(1000), Value::TYPE_DOUBLE,
+                Value::CreateDoubleValue(1));
+
+  converter.set_allow_regexp(true);
+  TestWeirdType(converter, regex, Value::TYPE_STRING,
+                Value::CreateStringValue("/./"));
+}
+
+TEST_F(V8ValueConverterTest, Prototype) {
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  const char* source = "(function() {"
+      "Object.prototype.foo = 'foo';"
+      "return {};"
+      "})();";
+
+  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());
+
+  V8ValueConverter converter;
+  scoped_ptr<DictionaryValue> result(
+      static_cast<DictionaryValue*>(converter.FromV8Value(object, context_)));
+  ASSERT_TRUE(result.get());
+  EXPECT_EQ(0u, result->size());
+}