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

TBR=aa@chromium.org Review URL: http://codereview.chromium.org/6900020 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@82677 0039d316-1c4b-4281-b951-d872f2087c98
author: aa@chromium.org <aa@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2011-04-22 18:33:41 +0000
committer: aa@chromium.org <aa@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> 2011-04-22 18:33:41 +0000
commit: 56fb1cf1244d2c7de266794426794dc8c3a2771d (patch)
tree: f10d857a9c614233d881c90716015f58a8d5b19e
parent: 3c8b60c5a5c3327e6ed049995ca502a938ecd091 (diff)
download: chromium_src-56fb1cf1244d2c7de266794426794dc8c3a2771d.zip
chromium_src-56fb1cf1244d2c7de266794426794dc8c3a2771d.tar.gz
chromium_src-56fb1cf1244d2c7de266794426794dc8c3a2771d.tar.bz2
3 files changed, 38 insertions, 332 deletions
diff --git a/content/renderer/v8_value_converter.cc b/content/renderer/v8_value_converter.cc
index cefbea8..dcbf8db 100644
--- a/content/renderer/v8_value_converter.cc
+++ b/content/renderer/v8_value_converter.cc
@@ -11,27 +11,22 @@
 #include "base/values.h"
 #include "v8/include/v8.h"
 
-V8ValueConverter::V8ValueConverter()
-    : allow_undefined_(false),
-      allow_date_(false),
-      allow_regexp_(false) {
+V8ValueConverter::V8ValueConverter() {
 }
 
 v8::Handle<v8::Value> V8ValueConverter::ToV8Value(
-    Value* value, v8::Handle<v8::Context> context) const {
+    Value* value, v8::Handle<v8::Context> context) {
   v8::Context::Scope context_scope(context);
-  v8::HandleScope handle_scope;
-  return handle_scope.Close(ToV8ValueImpl(value));
+  return ToV8ValueImpl(value);
 }
 
 Value* V8ValueConverter::FromV8Value(v8::Handle<v8::Value> val,
-                                     v8::Handle<v8::Context> context) const {
+                                     v8::Handle<v8::Context> context) {
   v8::Context::Scope context_scope(context);
-  v8::HandleScope handle_scope;
   return FromV8ValueImpl(val);
 }
 
-v8::Handle<v8::Value> V8ValueConverter::ToV8ValueImpl(Value* value) const {
+v8::Handle<v8::Value> V8ValueConverter::ToV8ValueImpl(Value* value) {
   CHECK(value);
   switch (value->GetType()) {
     case Value::TYPE_NULL:
@@ -68,56 +63,39 @@ v8::Handle<v8::Value> V8ValueConverter::ToV8ValueImpl(Value* value) const {
       return ToV8Object(static_cast<DictionaryValue*>(value));
 
     default:
-      LOG(ERROR) << "Unexpected value type: " << value->GetType();
+      NOTREACHED() << "Unexpected value type: " << value->GetType();
       return v8::Null();
   }
 }
 
-v8::Handle<v8::Value> V8ValueConverter::ToV8Array(ListValue* val) const {
+v8::Handle<v8::Value> V8ValueConverter::ToV8Array(ListValue* val) {
   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));
-
-    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.";
+    result->Set(static_cast<uint32>(i), ToV8ValueImpl(child));
   }
 
   return result;
 }
 
-v8::Handle<v8::Value> V8ValueConverter::ToV8Object(DictionaryValue* val) const {
+v8::Handle<v8::Value> V8ValueConverter::ToV8Object(DictionaryValue* val) {
   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;
-    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.";
-    }
+    result->Set(v8::String::New(key.c_str(), key.length()),
+                ToV8ValueImpl(child));
   }
 
   return result;
 }
 
-Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) const {
-  CHECK(!val.IsEmpty());
-
+Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) {
   if (val->IsNull())
     return Value::CreateNullValue();
 
@@ -135,9 +113,6 @@ Value* V8ValueConverter::FromV8ValueImpl(v8::Handle<v8::Value> val) const {
     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);
@@ -150,65 +125,32 @@ Value* V8ValueConverter::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(v8::Handle<v8::Array>::Cast(val));
 
   if (val->IsObject())
     return FromV8Object(val->ToObject());
 
-  LOG(ERROR) << "Unexpected v8 value type encountered.";
+  NOTREACHED() << "Unexpected v8::Value type.";
   return Value::CreateNullValue();
 }
 
-ListValue* V8ValueConverter::FromV8Array(v8::Handle<v8::Array> val) const {
+ListValue* V8ValueConverter::FromV8Array(v8::Handle<v8::Array> val) {
   ListValue* result = new ListValue();
   for (uint32 i = 0; i < val->Length(); ++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);
+    result->Append(FromV8ValueImpl(val->Get(i)));
   }
   return result;
 }
 
-DictionaryValue* V8ValueConverter::FromV8Object(
-    v8::Handle<v8::Object> val) const {
+DictionaryValue* V8ValueConverter::FromV8Object(v8::Handle<v8::Object> val) {
   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(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::Handle<v8::String> name(
+        v8::Handle<v8::String>::Cast(property_names->Get(i)));
     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()),
-                                    child);
+                                    FromV8ValueImpl(val->Get(name)));
   }
   return result;
 }
diff --git a/content/renderer/v8_value_converter.h b/content/renderer/v8_value_converter.h
index acb2d0a..4bfce75 100644
--- a/content/renderer/v8_value_converter.h
+++ b/content/renderer/v8_value_converter.h
@@ -14,46 +14,37 @@ 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. 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.
+  // 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.
   v8::Handle<v8::Value> ToV8Value(Value* value,
-                                  v8::Handle<v8::Context> context) const;
+                                  v8::Handle<v8::Context> context);
 
-  // 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.
+  // 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.
   Value* FromV8Value(v8::Handle<v8::Value> value,
-                     v8::Handle<v8::Context> context) const;
+                     v8::Handle<v8::Context> context);
 
  private:
-  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;
+  v8::Handle<v8::Value> ToV8ValueImpl(Value* value);
+  v8::Handle<v8::Value> ToV8Array(ListValue* list);
+  v8::Handle<v8::Value> ToV8Object(DictionaryValue* dictionary);
 
-  // If true, we will convert undefined JavaScript values to null.
-  bool allow_undefined_;
+  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 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 650927f..769b281 100644
--- a/content/renderer/v8_value_converter_browsertest.cc
+++ b/content/renderer/v8_value_converter_browsertest.cc
@@ -22,115 +22,6 @@ 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_;
 };
@@ -165,8 +56,9 @@ TEST_F(V8ValueConverterTest, BasicRoundTrip) {
   v8::HandleScope handle_scope;
 
   V8ValueConverter converter;
-  v8::Handle<v8::Object> v8_object =
-      converter.ToV8Value(&original_root, context_).As<v8::Object>();
+  v8::Handle<v8::Object> v8_object(
+      v8::Handle<v8::Object>::Cast(
+          converter.ToV8Value(&original_root, context_)));
   ASSERT_FALSE(v8_object.IsEmpty());
 
   EXPECT_EQ(original_root.size(), v8_object->GetPropertyNames()->Length());
@@ -205,122 +97,3 @@ 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());
-}
author	aa@chromium.org <aa@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2011-04-22 18:33:41 +0000
committer	aa@chromium.org <aa@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>	2011-04-22 18:33:41 +0000
commit	56fb1cf1244d2c7de266794426794dc8c3a2771d (patch)
tree	f10d857a9c614233d881c90716015f58a8d5b19e
parent	3c8b60c5a5c3327e6ed049995ca502a938ecd091 (diff)
download	chromium_src-56fb1cf1244d2c7de266794426794dc8c3a2771d.zip chromium_src-56fb1cf1244d2c7de266794426794dc8c3a2771d.tar.gz chromium_src-56fb1cf1244d2c7de266794426794dc8c3a2771d.tar.bz2