2011-07-13 Vitaly Repeshko <vitalyr@chromium.org>
authorvitalyr@chromium.org <vitalyr@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Jul 2011 21:44:29 +0000 (21:44 +0000)
committervitalyr@chromium.org <vitalyr@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Jul 2011 21:44:29 +0000 (21:44 +0000)
        [V8] Avoid memory leaks with hidden references.
        https://bugs.webkit.org/show_bug.cgi?id=64467

        Reviewed by Adam Barth.

        We used to have growing arrays of hidden references associated
        with objects. The entries in this array had no keys and were never
        removed. This patch changes the interface to require a reference
        name. This way it's harder to leak an unbounded number of
        objects. Also it makes our wrapper objects one machine word
        smaller.

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

16 files changed:
Source/WebCore/ChangeLog
Source/WebCore/bindings/scripts/CodeGeneratorV8.pm
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp
Source/WebCore/bindings/v8/V8DOMWrapper.cpp
Source/WebCore/bindings/v8/V8DOMWrapper.h
Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp
Source/WebCore/bindings/v8/V8HiddenPropertyName.h
Source/WebCore/bindings/v8/WrapperTypeInfo.h
Source/WebCore/bindings/v8/custom/V8CSSStyleSheetCustom.cpp
Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp
Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp
Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp
Source/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp
Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
Source/WebCore/bindings/v8/custom/V8StyleSheetCustom.cpp
Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp

index 5820807..c861e8b 100644 (file)
@@ -1,3 +1,55 @@
+2011-07-13  Vitaly Repeshko  <vitalyr@chromium.org>
+
+        [V8] Avoid memory leaks with hidden references.
+        https://bugs.webkit.org/show_bug.cgi?id=64467
+
+        Reviewed by Adam Barth.
+
+        We used to have growing arrays of hidden references associated
+        with objects. The entries in this array had no keys and were never
+        removed. This patch changes the interface to require a reference
+        name. This way it's harder to leak an unbounded number of
+        objects. Also it makes our wrapper objects one machine word
+        smaller.
+
+        * bindings/scripts/CodeGeneratorV8.pm:
+        (GenerateNormalAttrGetter): Started using new interface.
+
+        Interface changes:
+        * bindings/v8/V8DOMWrapper.cpp:
+        (WebCore::V8DOMWrapper::setNamedHiddenReference):
+        (WebCore::V8DOMWrapper::setNamedHiddenWindowReference):
+        * bindings/v8/V8DOMWrapper.h:
+
+        Added a helper to compute hidden reference names as V8 strings:
+        * bindings/v8/V8HiddenPropertyName.cpp:
+        (WebCore::V8HiddenPropertyName::hiddenReferenceName):
+        * bindings/v8/V8HiddenPropertyName.h:
+
+        * bindings/v8/WrapperTypeInfo.h: Removed the hidden reference
+        array internal field.
+
+        Update usages of hidden references:
+        * bindings/v8/custom/V8CSSStyleSheetCustom.cpp:
+        (WebCore::toV8):
+        * bindings/v8/custom/V8DOMStringMapCustom.cpp:
+        (WebCore::toV8):
+        * bindings/v8/custom/V8DOMTokenListCustom.cpp:
+        (WebCore::toV8):
+        * bindings/v8/custom/V8LocationCustom.cpp:
+        (WebCore::toV8):
+        * bindings/v8/custom/V8MessageChannelConstructor.cpp:
+        (WebCore::V8MessageChannel::constructorCallback):
+        * bindings/v8/custom/V8NamedNodeMapCustom.cpp:
+        (WebCore::toV8):
+        * bindings/v8/custom/V8StyleSheetCustom.cpp:
+        (WebCore::toV8):
+        * bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:
+        (WebCore::toV8Object):
+
+        * bindings/scripts/test/V8/V8TestObj.cpp:
+        (WebCore::TestObjInternal::readOnlyTestObjAttrAttrGetter): Updated.
+
 2011-07-13  Joseph Pecoraro  <joepeck@webkit.org>
 
         Improve names of some ApplicationCacheStorage accessor methods
index 66f970c..cf0edbe 100644 (file)
@@ -856,9 +856,9 @@ END
         push(@implContentDecls, "        wrapper = toV8(result.get());\n");
         push(@implContentDecls, "        if (!wrapper.IsEmpty())\n");
         if ($dataNode->name eq "DOMWindow") {
-            push(@implContentDecls, "            V8DOMWrapper::setHiddenWindowReference(imp->frame(), wrapper);\n");
+            push(@implContentDecls, "            V8DOMWrapper::setNamedHiddenWindowReference(imp->frame(), \"${attrName}\", wrapper);\n");
         } else {
-            push(@implContentDecls, "            V8DOMWrapper::setHiddenReference(info.Holder(), wrapper);\n");
+            push(@implContentDecls, "            V8DOMWrapper::setNamedHiddenReference(info.Holder(), \"${attrName}\", wrapper);\n");
         }
         push(@implContentDecls, "    }\n");
         push(@implContentDecls, "    return wrapper;\n");
index 61f8262..8b2eb6c 100644 (file)
@@ -76,7 +76,7 @@ static v8::Handle<v8::Value> readOnlyTestObjAttrAttrGetter(v8::Local<v8::String>
     if (wrapper.IsEmpty()) {
         wrapper = toV8(result.get());
         if (!wrapper.IsEmpty())
-            V8DOMWrapper::setHiddenReference(info.Holder(), wrapper);
+            V8DOMWrapper::setNamedHiddenReference(info.Holder(), "readOnlyTestObjAttr", wrapper);
     }
     return wrapper;
 }
index c6127bc..46cfbed 100644 (file)
@@ -31,6 +31,7 @@
 #include "config.h"
 #include "V8DOMWrapper.h"
 
+#include "ArrayBufferView.h"
 #include "CSSMutableStyleDeclaration.h"
 #include "DOMDataStore.h"
 #include "DocumentLoader.h"
 #include "V8AbstractEventListener.h"
 #include "V8Binding.h"
 #include "V8Collection.h"
-#include "V8DedicatedWorkerContext.h"
 #include "V8DOMApplicationCache.h"
 #include "V8DOMMap.h"
 #include "V8DOMWindow.h"
+#include "V8DedicatedWorkerContext.h"
 #include "V8EventListener.h"
 #include "V8EventListenerList.h"
 #include "V8EventSource.h"
@@ -51,6 +52,7 @@
 #include "V8FileWriter.h"
 #include "V8HTMLCollection.h"
 #include "V8HTMLDocument.h"
+#include "V8HiddenPropertyName.h"
 #include "V8IDBDatabase.h"
 #include "V8IDBRequest.h"
 #include "V8IDBTransaction.h"
@@ -75,7 +77,6 @@
 #include "V8WorkerContext.h"
 #include "V8WorkerContextEventListener.h"
 #include "V8XMLHttpRequest.h"
-#include "ArrayBufferView.h"
 #include "WebGLContextAttributes.h"
 #include "WebGLUniformLocation.h"
 #include "WorkerContextExecutionProxy.h"
@@ -189,18 +190,13 @@ v8::Local<v8::Function> V8DOMWrapper::getConstructor(WrapperTypeInfo* type, Work
 }
 #endif
 
-void V8DOMWrapper::setHiddenReference(v8::Handle<v8::Object> parent, v8::Handle<v8::Value> child)
+
+void V8DOMWrapper::setNamedHiddenReference(v8::Handle<v8::Object> parent, const char* name, v8::Handle<v8::Value> child)
 {
-    v8::Local<v8::Value> hiddenReferenceObject = parent->GetInternalField(v8DOMHiddenReferenceArrayIndex);
-    if (hiddenReferenceObject->IsNull() || hiddenReferenceObject->IsUndefined()) {
-        hiddenReferenceObject = v8::Array::New();
-        parent->SetInternalField(v8DOMHiddenReferenceArrayIndex, hiddenReferenceObject);
-    }
-    v8::Local<v8::Array> hiddenReferenceArray = v8::Local<v8::Array>::Cast(hiddenReferenceObject);
-    hiddenReferenceArray->Set(v8::Integer::New(hiddenReferenceArray->Length()), child);
+    parent->SetHiddenValue(V8HiddenPropertyName::hiddenReferenceName(name), child);
 }
 
-void V8DOMWrapper::setHiddenWindowReference(Frame* frame, v8::Handle<v8::Value> jsObject)
+void V8DOMWrapper::setNamedHiddenWindowReference(Frame* frame, const char* name, v8::Handle<v8::Value> jsObject)
 {
     // Get DOMWindow
     if (!frame)
@@ -214,7 +210,7 @@ void V8DOMWrapper::setHiddenWindowReference(Frame* frame, v8::Handle<v8::Value>
     global = V8DOMWrapper::lookupDOMWrapper(V8DOMWindow::GetTemplate(), global);
     ASSERT(!global.IsEmpty());
 
-    setHiddenReference(global, jsObject);
+    setNamedHiddenReference(global, name, jsObject);
 }
 
 WrapperTypeInfo* V8DOMWrapper::domWrapperType(v8::Handle<v8::Object> object)
index 7f4490a..0cae6db 100644 (file)
@@ -114,10 +114,14 @@ namespace WebCore {
         // Check whether a V8 value is a wrapper of type |classType|.
         static bool isWrapperOfType(v8::Handle<v8::Value>, WrapperTypeInfo*);
 
-        static void setHiddenReference(v8::Handle<v8::Object> parent, v8::Handle<v8::Value> child);
-
-        // Set hidden references in a DOMWindow object of a frame.
-        static void setHiddenWindowReference(Frame*, v8::Handle<v8::Value>);
+        // Proper object lifetime support.
+        //
+        // Helper functions to make sure the child object stays alive
+        // while the parent is alive. Using the name more than once
+        // overwrites previous references making it possible to free
+        // old children.
+        static void setNamedHiddenReference(v8::Handle<v8::Object> parent, const char* name, v8::Handle<v8::Value> child);
+        static void setNamedHiddenWindowReference(Frame*, const char*, v8::Handle<v8::Value>);
 
         static v8::Local<v8::Object> instantiateV8Object(V8Proxy* proxy, WrapperTypeInfo*, void* impl);
 
index d83573f..2909269 100644 (file)
@@ -31,6 +31,9 @@
 #include "config.h"
 #include "V8HiddenPropertyName.h"
 
+#include <string.h>
+#include <wtf/Vector.h>
+
 namespace WebCore {
 
 #define V8_AS_STRING(x) V8_AS_STRING_IMPL(x)
@@ -39,12 +42,22 @@ namespace WebCore {
 #define V8_DEFINE_PROPERTY(name) \
 v8::Handle<v8::String> V8HiddenPropertyName::name() \
 { \
-    static v8::Persistent<v8::String>* string = createString("WebCore::V8HiddenPropertyName::" V8_AS_STRING(name)); \
+    static v8::Persistent<v8::String>* string = createString("WebCore::HiddenProperty::" V8_AS_STRING(name)); \
     return *string; \
 }
 
 V8_HIDDEN_PROPERTIES(V8_DEFINE_PROPERTY);
 
+static const char hiddenReferenceNamePrefix[] = "WebCore::HiddenReference::";
+
+v8::Handle<v8::String> V8HiddenPropertyName::hiddenReferenceName(const char* name)
+{
+    Vector<char, 64> prefixedName;
+    prefixedName.append(hiddenReferenceNamePrefix, sizeof(hiddenReferenceNamePrefix) - 1);
+    prefixedName.append(name, strlen(name));
+    return v8::String::NewSymbol(prefixedName.data(), static_cast<int>(prefixedName.size()));
+}
+
 v8::Persistent<v8::String>* V8HiddenPropertyName::createString(const char* key)
 {
     v8::HandleScope scope;
index 7867b36..7c4aae0 100644 (file)
@@ -52,6 +52,8 @@ namespace WebCore {
         V8_HIDDEN_PROPERTIES(V8_DECLARE_PROPERTY);
 #undef V8_DECLARE_PROPERTY
 
+        static v8::Handle<v8::String> hiddenReferenceName(const char* name);
+
     private:
         static v8::Persistent<v8::String>* createString(const char* key);
     };
index 166d642..c2684d6 100644 (file)
@@ -39,8 +39,7 @@ namespace WebCore {
     
     static const int v8DOMWrapperTypeIndex = 0;
     static const int v8DOMWrapperObjectIndex = 1;
-    static const int v8DOMHiddenReferenceArrayIndex = 2;
-    static const int v8DefaultWrapperInternalFieldCount = 3;
+    static const int v8DefaultWrapperInternalFieldCount = 2;
 
     static const uint16_t v8DOMSubtreeClassId = 1;
 
index 9effca3..9b2988b 100644 (file)
@@ -44,7 +44,7 @@ v8::Handle<v8::Value> toV8(CSSStyleSheet* impl)
     // Add a hidden reference from stylesheet object to its owner node.
     Node* ownerNode = impl->ownerNode();
     if (ownerNode && !wrapper.IsEmpty())
-        V8DOMWrapper::setHiddenReference(wrapper, toV8(ownerNode));
+        V8DOMWrapper::setNamedHiddenReference(wrapper, "ownerNode", toV8(ownerNode));
     return wrapper;
 }
 
index 71ff357..1d6398e 100644 (file)
@@ -105,7 +105,7 @@ v8::Handle<v8::Value> toV8(DOMStringMap* impl)
     if (!wrapper.IsEmpty() && element) {
         v8::Handle<v8::Value> elementValue = toV8(element);
         if (!elementValue.IsEmpty() && elementValue->IsObject())
-            V8DOMWrapper::setHiddenReference(elementValue.As<v8::Object>(), wrapper);
+            V8DOMWrapper::setNamedHiddenReference(elementValue.As<v8::Object>(), "domStringMap", wrapper);
     }
     return wrapper;
 }
index 171ff5c..08051ab 100644 (file)
@@ -48,7 +48,7 @@ v8::Handle<v8::Value> toV8(DOMTokenList* impl)
     if (!wrapper.IsEmpty() && element) {
         v8::Handle<v8::Value> elementValue = toV8(element);
         if (!elementValue.IsEmpty() && elementValue->IsObject())
-            V8DOMWrapper::setHiddenReference(elementValue.As<v8::Object>(), wrapper);
+            V8DOMWrapper::setNamedHiddenReference(elementValue.As<v8::Object>(), "domTokenList", wrapper);
     }
     return wrapper;
 }
index b222a99..7db74e8 100644 (file)
@@ -280,7 +280,7 @@ v8::Handle<v8::Value> toV8(Location* impl)
     if (wrapper.IsEmpty()) {
         wrapper = V8Location::wrap(impl);
         if (!wrapper.IsEmpty())
-            V8DOMWrapper::setHiddenWindowReference(impl->frame(), wrapper);
+            V8DOMWrapper::setNamedHiddenWindowReference(impl->frame(), "location", wrapper);
     }
     return wrapper;
 }
index b966e42..775e745 100644 (file)
@@ -67,8 +67,8 @@ v8::Handle<v8::Value> V8MessageChannel::constructorCallback(const v8::Arguments&
     // Create references from the MessageChannel wrapper to the two
     // MessagePort wrappers to make sure that the MessagePort wrappers
     // stay alive as long as the MessageChannel wrapper is around.
-    V8DOMWrapper::setHiddenReference(messageChannel, toV8(obj->port1()));
-    V8DOMWrapper::setHiddenReference(messageChannel, toV8(obj->port2()));
+    V8DOMWrapper::setNamedHiddenReference(messageChannel, "port1", toV8(obj->port1()));
+    V8DOMWrapper::setNamedHiddenReference(messageChannel, "port2", toV8(obj->port2()));
 
     // Setup the standard wrapper object internal fields.
     V8DOMWrapper::setDOMWrapper(messageChannel, &info, obj.get());
index d9e1de0..c9014c0 100644 (file)
@@ -83,7 +83,7 @@ v8::Handle<v8::Value> toV8(NamedNodeMap* impl)
     // Add a hidden reference from named node map to its owner node.
     Element* element = impl->element();
     if (!wrapper.IsEmpty() && element)
-        V8DOMWrapper::setHiddenReference(wrapper, toV8(element));
+        V8DOMWrapper::setNamedHiddenReference(wrapper, "ownerNode", toV8(element));
     return wrapper;
 }
 
index b3f6ff7..0fe6af8 100644 (file)
@@ -47,7 +47,7 @@ v8::Handle<v8::Value> toV8(StyleSheet* impl)
     // Add a hidden reference from stylesheet object to its owner node.
     Node* ownerNode = impl->ownerNode();
     if (ownerNode && !wrapper.IsEmpty())
-        V8DOMWrapper::setHiddenReference(wrapper, toV8(ownerNode));
+        V8DOMWrapper::setNamedHiddenReference(wrapper, "ownerNode", toV8(ownerNode));
     return wrapper;
 }
 
index 474ff91..35d9c59 100644 (file)
@@ -161,22 +161,27 @@ static v8::Handle<v8::Value> toV8Object(WebGLExtension* extension, v8::Handle<v8
     if (!extension)
         return v8::Null();
     v8::Handle<v8::Value> extensionObject;
+    const char* referenceName;
     switch (extension->getName()) {
     case WebGLExtension::WebKitLoseContextName:
         extensionObject = toV8(static_cast<WebKitLoseContext*>(extension));
+        referenceName = "webKitLoseContextName";
         break;
     case WebGLExtension::OESStandardDerivativesName:
         extensionObject = toV8(static_cast<OESStandardDerivatives*>(extension));
+        referenceName = "oesStandardDerivativesName";
         break;
     case WebGLExtension::OESTextureFloatName:
         extensionObject = toV8(static_cast<OESTextureFloat*>(extension));
+        referenceName = "oesTextureFloatName";
         break;
     case WebGLExtension::OESVertexArrayObjectName:
         extensionObject = toV8(static_cast<OESVertexArrayObject*>(extension));
+        referenceName = "oesVertexArrayObjectName";
         break;
     }
     ASSERT(!extensionObject.IsEmpty());
-    V8DOMWrapper::setHiddenReference(contextObject, extensionObject);
+    V8DOMWrapper::setNamedHiddenReference(contextObject, referenceName, extensionObject);
     return extensionObject;
 }