[v8] ScriptValue has dangerous copy semantics
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Mar 2013 16:24:42 +0000 (16:24 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Mar 2013 16:24:42 +0000 (16:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=110206

Patch by Dan Carney <dcarney@google.com> on 2013-03-01
Reviewed by Kentaro Hara.

Update ScriptValue to used a SharedPersistent,
making it impossible to return dead references.

No new tests. No change in functionality.

* bindings/v8/ScriptValue.cpp:
(WebCore::ScriptValue::serialize):
(WebCore::ScriptValue::getString):
(WebCore::ScriptValue::toString):
(WebCore::ScriptValue::toInspectorValue):
* bindings/v8/ScriptValue.h:
(WebCore::ScriptValue::ScriptValue):
(WebCore::ScriptValue::operator=):
(WebCore::ScriptValue::operator==):
(WebCore::ScriptValue::isEqual):
(WebCore::ScriptValue::isFunction):
(WebCore::ScriptValue::isNull):
(WebCore::ScriptValue::isUndefined):
(WebCore::ScriptValue::isObject):
(WebCore::ScriptValue::hasNoValue):
(WebCore::ScriptValue::clear):
(ScriptValue):
(WebCore::ScriptValue::v8Value):
(WebCore::ScriptValue::v8ValueRaw):
* bindings/v8/SharedPersistent.h:
* bindings/v8/custom/V8InjectedScriptHostCustom.cpp:
(WebCore::InjectedScriptHost::scriptValueAsNode):
* bindings/v8/custom/V8MessageEventCustom.cpp:
(WebCore::V8MessageEvent::dataAttrGetterCustom):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@144458 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/ScriptValue.cpp
Source/WebCore/bindings/v8/ScriptValue.h
Source/WebCore/bindings/v8/SharedPersistent.h
Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp

index b34da6f..4b1903a 100644 (file)
@@ -1,3 +1,40 @@
+2013-03-01  Dan Carney  <dcarney@google.com>
+
+        [v8] ScriptValue has dangerous copy semantics
+        https://bugs.webkit.org/show_bug.cgi?id=110206
+
+        Reviewed by Kentaro Hara.
+
+        Update ScriptValue to used a SharedPersistent,
+        making it impossible to return dead references.
+
+        No new tests. No change in functionality.
+
+        * bindings/v8/ScriptValue.cpp:
+        (WebCore::ScriptValue::serialize):
+        (WebCore::ScriptValue::getString):
+        (WebCore::ScriptValue::toString):
+        (WebCore::ScriptValue::toInspectorValue):
+        * bindings/v8/ScriptValue.h:
+        (WebCore::ScriptValue::ScriptValue):
+        (WebCore::ScriptValue::operator=):
+        (WebCore::ScriptValue::operator==):
+        (WebCore::ScriptValue::isEqual):
+        (WebCore::ScriptValue::isFunction):
+        (WebCore::ScriptValue::isNull):
+        (WebCore::ScriptValue::isUndefined):
+        (WebCore::ScriptValue::isObject):
+        (WebCore::ScriptValue::hasNoValue):
+        (WebCore::ScriptValue::clear):
+        (ScriptValue):
+        (WebCore::ScriptValue::v8Value):
+        (WebCore::ScriptValue::v8ValueRaw):
+        * bindings/v8/SharedPersistent.h:
+        * bindings/v8/custom/V8InjectedScriptHostCustom.cpp:
+        (WebCore::InjectedScriptHost::scriptValueAsNode):
+        * bindings/v8/custom/V8MessageEventCustom.cpp:
+        (WebCore::V8MessageEvent::dataAttrGetterCustom):
+
 2013-03-01  Julien Chaffraix  <jchaffraix@webkit.org>
 
         Add FeatureObserver for marquee and reflection
index 0799948..bd05157 100644 (file)
@@ -47,13 +47,13 @@ ScriptValue::~ScriptValue()
 PassRefPtr<SerializedScriptValue> ScriptValue::serialize(ScriptState* scriptState)
 {
     ScriptScope scope(scriptState);
-    return SerializedScriptValue::create(v8Value());
+    return SerializedScriptValue::create(v8ValueRaw());
 }
 
 PassRefPtr<SerializedScriptValue> ScriptValue::serialize(ScriptState* scriptState, MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, bool& didThrow)
 {
     ScriptScope scope(scriptState);
-    return SerializedScriptValue::create(v8Value(), messagePorts, arrayBuffers, didThrow);
+    return SerializedScriptValue::create(v8ValueRaw(), messagePorts, arrayBuffers, didThrow);
 }
 
 ScriptValue ScriptValue::deserialize(ScriptState* scriptState, SerializedScriptValue* value)
@@ -64,20 +64,20 @@ ScriptValue ScriptValue::deserialize(ScriptState* scriptState, SerializedScriptV
 
 bool ScriptValue::getString(String& result) const
 {
-    if (m_value.isEmpty())
+    if (hasNoValue())
         return false;
 
-    if (!m_value->IsString())
+    if (!v8ValueRaw()->IsString())
         return false;
 
-    result = toWebCoreString(m_value.get());
+    result = toWebCoreString(v8ValueRaw());
     return true;
 }
 
 String ScriptValue::toString(ScriptState*) const
 {
     v8::TryCatch block;
-    v8::Handle<v8::String> string = m_value->ToString();
+    v8::Handle<v8::String> string = v8ValueRaw()->ToString();
     if (block.HasCaught())
         return String();
     return v8StringToWebCoreString<String>(string, DoNotExternalize);
@@ -142,7 +142,7 @@ PassRefPtr<InspectorValue> ScriptValue::toInspectorValue(ScriptState* scriptStat
     v8::HandleScope handleScope;
     // v8::Object::GetPropertyNames() expects current context to be not null.
     v8::Context::Scope contextScope(scriptState->context());
-    return v8ToInspectorValue(m_value.get(), InspectorValue::maxDepth);
+    return v8ToInspectorValue(v8ValueRaw(), InspectorValue::maxDepth);
 }
 #endif
 
index aa8c998..2ba9d31 100644 (file)
@@ -31,8 +31,8 @@
 #ifndef ScriptValue_h
 #define ScriptValue_h
 
-#include "ScopedPersistent.h"
 #include "ScriptState.h"
+#include "SharedPersistent.h"
 #include <v8.h>
 #include <wtf/PassRefPtr.h>
 #include <wtf/RefPtr.h>
@@ -60,47 +60,37 @@ public:
     ScriptValue() { }
     virtual ~ScriptValue();
 
-    ScriptValue(v8::Handle<v8::Value> value) 
+    ScriptValue(v8::Handle<v8::Value> value)
+        : m_value(value.IsEmpty() ? 0 : SharedPersistent<v8::Value>::create(value))
     {
-        if (value.IsEmpty())
-            return;
-        m_value.set(value);
     }
 
-    ScriptValue(const ScriptValue& value) 
+    ScriptValue(const ScriptValue& value)
+        : m_value(value.m_value)
     {
-        if (value.hasNoValue())
-            return;
-        m_value.set(value.m_value.get());
     }
 
     ScriptValue& operator=(const ScriptValue& value) 
     {
-        if (this == &value) 
-            return *this;
-
-        m_value.clear();
-
-        if (value.hasNoValue())
-            return *this;
-
-        m_value.set(value.m_value.get());
+        if (this != &value)
+            m_value = value.m_value;
         return *this;
     }
 
     bool operator==(const ScriptValue& value) const
     {
-        return m_value.get() == value.m_value.get();
+        return v8ValueRaw() == value.v8ValueRaw();
     }
 
     bool isEqual(ScriptState*, const ScriptValue& value) const
     {
-        return m_value.get() == value.m_value.get();
+        return operator==(value);
     }
 
     bool isFunction() const
     {
-        return m_value->IsFunction();
+        ASSERT(!hasNoValue());
+        return v8ValueRaw()->IsFunction();
     }
 
     bool operator!=(const ScriptValue& value) const
@@ -110,22 +100,25 @@ public:
 
     bool isNull() const
     {
-        return m_value->IsNull();
+        ASSERT(!hasNoValue());
+        return v8ValueRaw()->IsNull();
     }
 
     bool isUndefined() const
     {
-        return m_value->IsUndefined();
+        ASSERT(!hasNoValue());
+        return v8ValueRaw()->IsUndefined();
     }
 
     bool isObject() const
     {
-        return m_value->IsObject();
+        ASSERT(!hasNoValue());
+        return v8ValueRaw()->IsObject();
     }
 
     bool hasNoValue() const
     {
-        return m_value.isEmpty();
+        return !m_value.get() || m_value->get().IsEmpty();
     }
 
     PassRefPtr<SerializedScriptValue> serialize(ScriptState*);
@@ -134,10 +127,19 @@ public:
 
     void clear()
     {
-        m_value.clear();
+        m_value = 0;
+    }
+
+    v8::Handle<v8::Value> v8Value() const
+    {
+        return v8::Local<v8::Value>::New(v8ValueRaw());
     }
 
-    v8::Handle<v8::Value> v8Value() const { return m_value.get(); }
+    // FIXME: This function should be private. 
+    v8::Handle<v8::Value> v8ValueRaw() const
+    {
+        return m_value.get() ? m_value->get() : v8::Handle<v8::Value>();
+    }
 
     bool getString(ScriptState*, String& result) const { return getString(result); }
     bool getString(String& result) const;
@@ -146,7 +148,7 @@ public:
     PassRefPtr<InspectorValue> toInspectorValue(ScriptState*) const;
 
 private:
-    ScopedPersistent<v8::Value> m_value;
+    RefPtr<SharedPersistent<v8::Value> > m_value;
 };
 
 } // namespace WebCore
index 2e39a06..52106c2 100644 (file)
@@ -38,7 +38,6 @@
 
 namespace WebCore {
 
-    // FIXME: Remove this class.
     template <typename T>
     class SharedPersistent : public RefCounted<SharedPersistent<T> > {
     public:
index 2320f36..e63698f 100644 (file)
@@ -65,7 +65,7 @@ Node* InjectedScriptHost::scriptValueAsNode(ScriptValue value)
 {
     if (!value.isObject() || value.isNull())
         return 0;
-    return V8Node::toNative(v8::Handle<v8::Object>::Cast(value.v8Value()));
+    return V8Node::toNative(v8::Handle<v8::Object>::Cast(value.v8ValueRaw()));
 }
 
 ScriptValue InjectedScriptHost::nodeAsScriptValue(ScriptState* state, Node* node)
index 384cba5..5bdeda9 100644 (file)
@@ -53,7 +53,7 @@ v8::Handle<v8::Value> V8MessageEvent::dataAttrGetterCustom(v8::Local<v8::String>
         if (scriptValue.hasNoValue())
             result = v8Null(info.GetIsolate());
         else
-            result = v8::Local<v8::Value>::New(scriptValue.v8Value());
+            result = scriptValue.v8Value();
         break;
     }