Changeset 143441 in webkit

Timestamp:

Feb 20, 2013 1:58:37 AM (11 years ago)

Author:

commit-queue@webkit.org

Message:

[v8] ScriptValue has dangerous copy semantics
​https://bugs.webkit.org/show_bug.cgi?id=110206

Patch by Dan Carney <​dcarney@google.com> on 2013-02-20
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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/v8/ScriptValue.cpp (modified) (5 diffs)
• bindings/v8/ScriptValue.h (modified) (6 diffs)
• bindings/v8/SharedPersistent.h (modified) (1 diff)
• bindings/v8/custom/V8InjectedScriptHostCustom.cpp (modified) (1 diff)
• bindings/v8/custom/V8MessageEventCustom.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-20  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-02-20  Andrey Lushnikov  <lushnikov@chromium.org>
 

trunk/Source/WebCore/bindings/v8/ScriptValue.cpp

@@ -48 +48 @@
 {
     ScriptScope scope(scriptState);
-    return SerializedScriptValue::create(v8Value());
+    return SerializedScriptValue::create(v8ValueRaw());
 }
 
@@ -54 +54 @@
 {
     ScriptScope scope(scriptState);
-    return SerializedScriptValue::create(v8Value(), messagePorts, arrayBuffers, didThrow);
+    return SerializedScriptValue::create(v8ValueRaw(), messagePorts, arrayBuffers, didThrow);
 }
 
@@ -65 +65 @@
 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;
 }
@@ -78 +78 @@
 {
     v8::TryCatch block;
-    v8::Handle<v8::String> string = m_value->ToString();
+    v8::Handle<v8::String> string = v8ValueRaw()->ToString();
     if (block.HasCaught())
         return String();
@@ -143 +143 @@
     // 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

trunk/Source/WebCore/bindings/v8/ScriptValue.h

@@ -32 +32 @@
 #define ScriptValue_h
 
-#include "ScopedPersistent.h"
 #include "ScriptState.h"
+#include "SharedPersistent.h"
 #include <v8.h>
 #include <wtf/PassRefPtr.h>
@@ -61 +61 @@
     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;
     }
@@ -91 +80 @@
     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();
     }
 
@@ -111 +101 @@
     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();
     }
 
@@ -135 +128 @@
     void clear()
     {
-        m_value.clear();
+        m_value = 0;
     }
 
-    v8::Handle<v8::Value> v8Value() const { return m_value.get(); }
+    v8::Handle<v8::Value> v8Value() const
+    {
+        return v8::Local<v8::Value>::New(v8ValueRaw());
+    }
+
+    // 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); }
@@ -147 +149 @@
 
 private:
-    ScopedPersistent<v8::Value> m_value;
+    RefPtr<SharedPersistent<v8::Value> > m_value;
 };
 

trunk/Source/WebCore/bindings/v8/SharedPersistent.h

@@ -39 +39 @@
 namespace WebCore {
 
-    // FIXME: Remove this class.
     template <typename T>
     class SharedPersistent : public RefCounted<SharedPersistent<T> > {

trunk/Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp

@@ -66 +66 @@
     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()));
 }
 

trunk/Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp

@@ -54 +54 @@
             result = v8Null(info.GetIsolate());
         else
-            result = v8::Local<v8::Value>::New(scriptValue.v8Value());
+            result = scriptValue.v8Value();
         break;
     }