From 5319190a069ce6657577718a72e645dad45be0af Mon Sep 17 00:00:00 2001 From: xqian6 Date: Thu, 31 May 2012 16:54:04 +0800 Subject: Fix memory leak caused by CSSRuleList wrapper Accessing cssrulelist in javascript will create a new CSSRuleList object and wrapper JS object. The wrapper JS object will be added into hidden array in parent JS object which is alive during the whole execution. Thus memory leak happens (CSSRuleList in webkit, wrapper object and weak global handle in v8). Cherry pick 2 patches from upstream solves the problem: http://trac.webkit.org/changeset/90949 This patch changes hidden reference from array to named property. So new wrapper will replace old wrappper. But the memory leak still exists because the CSSRuleList wrapper will be added into an object group of current document. So they will still be alive during execution. http://trac.webkit.org/changeset/91256 This patch avoids to adding CSSRuleList wrapper into document object group. Combined with the first patch, it can resolve the memory leak problem. Change-Id: Icb523db52963726f27b6c02596822cfb6e8d5049 Author: Vitaly Repeshko Signed-off-by: Xi Qian Signed-off-by: Shuo Gao Signed-off-by: Bruce Beare Signed-off-by: Jack Ren Author-tracking-BZ: 32630 --- Source/WebCore/bindings/scripts/CodeGeneratorV8.pm | 4 ++-- .../WebCore/bindings/scripts/test/V8/V8TestObj.cpp | 2 +- Source/WebCore/bindings/v8/V8DOMWrapper.cpp | 20 ++++++++------------ Source/WebCore/bindings/v8/V8DOMWrapper.h | 12 ++++++++---- Source/WebCore/bindings/v8/V8GCController.cpp | 7 ------- Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp | 15 ++++++++++++++- Source/WebCore/bindings/v8/V8HiddenPropertyName.h | 2 ++ Source/WebCore/bindings/v8/WrapperTypeInfo.h | 3 +-- .../bindings/v8/custom/V8CSSStyleSheetCustom.cpp | 2 +- .../bindings/v8/custom/V8DOMStringMapCustom.cpp | 2 +- .../bindings/v8/custom/V8DOMTokenListCustom.cpp | 2 +- .../WebCore/bindings/v8/custom/V8LocationCustom.cpp | 2 +- .../v8/custom/V8MessageChannelConstructor.cpp | 4 ++-- .../bindings/v8/custom/V8NamedNodeMapCustom.cpp | 2 +- .../bindings/v8/custom/V8StyleSheetCustom.cpp | 2 +- .../v8/custom/V8WebGLRenderingContextCustom.cpp | 7 ++++++- 16 files changed, 50 insertions(+), 38 deletions(-) diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm b/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm index 22051bf..c9ed679 100644 --- a/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm +++ b/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm @@ -817,9 +817,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"); diff --git a/Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp b/Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp index 5884e89..8030559 100644 --- a/Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp +++ b/Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp @@ -75,7 +75,7 @@ static v8::Handle readOnlyTestObjAttrAttrGetter(v8::Local if (wrapper.IsEmpty()) { wrapper = toV8(result.get()); if (!wrapper.IsEmpty()) - V8DOMWrapper::setHiddenReference(info.Holder(), wrapper); + V8DOMWrapper::setNamedHiddenReference(info.Holder(), "readOnlyTestObjAttr", wrapper); } return wrapper; } diff --git a/Source/WebCore/bindings/v8/V8DOMWrapper.cpp b/Source/WebCore/bindings/v8/V8DOMWrapper.cpp index 249ca7c..7d0fe60 100644 --- a/Source/WebCore/bindings/v8/V8DOMWrapper.cpp +++ b/Source/WebCore/bindings/v8/V8DOMWrapper.cpp @@ -31,6 +31,7 @@ #include "config.h" #include "V8DOMWrapper.h" +#include "ArrayBufferView.h" #include "CSSMutableStyleDeclaration.h" #include "DOMDataStore.h" #include "DocumentLoader.h" @@ -40,10 +41,10 @@ #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" @@ -71,7 +73,6 @@ #include "V8WorkerContext.h" #include "V8WorkerContextEventListener.h" #include "V8XMLHttpRequest.h" -#include "ArrayBufferView.h" #include "WebGLContextAttributes.h" #include "WebGLUniformLocation.h" #include "WorkerContextExecutionProxy.h" @@ -185,18 +186,13 @@ v8::Local V8DOMWrapper::getConstructor(WrapperTypeInfo* type, Work } #endif -void V8DOMWrapper::setHiddenReference(v8::Handle parent, v8::Handle child) + +void V8DOMWrapper::setNamedHiddenReference(v8::Handle parent, const char* name, v8::Handle child) { - v8::Local hiddenReferenceObject = parent->GetInternalField(v8DOMHiddenReferenceArrayIndex); - if (hiddenReferenceObject->IsNull() || hiddenReferenceObject->IsUndefined()) { - hiddenReferenceObject = v8::Array::New(); - parent->SetInternalField(v8DOMHiddenReferenceArrayIndex, hiddenReferenceObject); - } - v8::Local hiddenReferenceArray = v8::Local::Cast(hiddenReferenceObject); - hiddenReferenceArray->Set(v8::Integer::New(hiddenReferenceArray->Length()), child); + parent->SetHiddenValue(V8HiddenPropertyName::hiddenReferenceName(name), child); } -void V8DOMWrapper::setHiddenWindowReference(Frame* frame, v8::Handle jsObject) +void V8DOMWrapper::setNamedHiddenWindowReference(Frame* frame, const char* name, v8::Handle jsObject) { // Get DOMWindow if (!frame) @@ -210,7 +206,7 @@ void V8DOMWrapper::setHiddenWindowReference(Frame* frame, v8::Handle global = V8DOMWrapper::lookupDOMWrapper(V8DOMWindow::GetTemplate(), global); ASSERT(!global.IsEmpty()); - setHiddenReference(global, jsObject); + setNamedHiddenReference(global, name, jsObject); } WrapperTypeInfo* V8DOMWrapper::domWrapperType(v8::Handle object) diff --git a/Source/WebCore/bindings/v8/V8DOMWrapper.h b/Source/WebCore/bindings/v8/V8DOMWrapper.h index 7f4490a..0cae6db 100644 --- a/Source/WebCore/bindings/v8/V8DOMWrapper.h +++ b/Source/WebCore/bindings/v8/V8DOMWrapper.h @@ -114,10 +114,14 @@ namespace WebCore { // Check whether a V8 value is a wrapper of type |classType|. static bool isWrapperOfType(v8::Handle, WrapperTypeInfo*); - static void setHiddenReference(v8::Handle parent, v8::Handle child); - - // Set hidden references in a DOMWindow object of a frame. - static void setHiddenWindowReference(Frame*, v8::Handle); + // 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 parent, const char* name, v8::Handle child); + static void setNamedHiddenWindowReference(Frame*, const char*, v8::Handle); static v8::Local instantiateV8Object(V8Proxy* proxy, WrapperTypeInfo*, void* impl); diff --git a/Source/WebCore/bindings/v8/V8GCController.cpp b/Source/WebCore/bindings/v8/V8GCController.cpp index c9e9dfd..ea017e8 100644 --- a/Source/WebCore/bindings/v8/V8GCController.cpp +++ b/Source/WebCore/bindings/v8/V8GCController.cpp @@ -377,13 +377,6 @@ public: v8::V8::AddImplicitReferences(wrapper, values.data(), values.size()); } - } else if (typeInfo->isSubclass(&V8CSSRuleList::info)) { - CSSRuleList* cssRuleList = static_cast(object); - GroupId groupId(cssRuleList); - StyleList* styleList = cssRuleList->styleList(); - if (styleList) - groupId = calculateGroupId(styleList); - m_grouper.append(GrouperItem(groupId, wrapper)); } } diff --git a/Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp b/Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp index d83573f..2909269 100644 --- a/Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp +++ b/Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp @@ -31,6 +31,9 @@ #include "config.h" #include "V8HiddenPropertyName.h" +#include +#include + 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 V8HiddenPropertyName::name() \ { \ - static v8::Persistent* string = createString("WebCore::V8HiddenPropertyName::" V8_AS_STRING(name)); \ + static v8::Persistent* string = createString("WebCore::HiddenProperty::" V8_AS_STRING(name)); \ return *string; \ } V8_HIDDEN_PROPERTIES(V8_DEFINE_PROPERTY); +static const char hiddenReferenceNamePrefix[] = "WebCore::HiddenReference::"; + +v8::Handle V8HiddenPropertyName::hiddenReferenceName(const char* name) +{ + Vector prefixedName; + prefixedName.append(hiddenReferenceNamePrefix, sizeof(hiddenReferenceNamePrefix) - 1); + prefixedName.append(name, strlen(name)); + return v8::String::NewSymbol(prefixedName.data(), static_cast(prefixedName.size())); +} + v8::Persistent* V8HiddenPropertyName::createString(const char* key) { v8::HandleScope scope; diff --git a/Source/WebCore/bindings/v8/V8HiddenPropertyName.h b/Source/WebCore/bindings/v8/V8HiddenPropertyName.h index 7867b36..7c4aae0 100644 --- a/Source/WebCore/bindings/v8/V8HiddenPropertyName.h +++ b/Source/WebCore/bindings/v8/V8HiddenPropertyName.h @@ -52,6 +52,8 @@ namespace WebCore { V8_HIDDEN_PROPERTIES(V8_DECLARE_PROPERTY); #undef V8_DECLARE_PROPERTY + static v8::Handle hiddenReferenceName(const char* name); + private: static v8::Persistent* createString(const char* key); }; diff --git a/Source/WebCore/bindings/v8/WrapperTypeInfo.h b/Source/WebCore/bindings/v8/WrapperTypeInfo.h index 166d642..c2684d6 100644 --- a/Source/WebCore/bindings/v8/WrapperTypeInfo.h +++ b/Source/WebCore/bindings/v8/WrapperTypeInfo.h @@ -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; diff --git a/Source/WebCore/bindings/v8/custom/V8CSSStyleSheetCustom.cpp b/Source/WebCore/bindings/v8/custom/V8CSSStyleSheetCustom.cpp index 9effca3..9b2988b 100644 --- a/Source/WebCore/bindings/v8/custom/V8CSSStyleSheetCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8CSSStyleSheetCustom.cpp @@ -44,7 +44,7 @@ v8::Handle 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; } diff --git a/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp b/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp index 71ff357..1d6398e 100644 --- a/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp @@ -105,7 +105,7 @@ v8::Handle toV8(DOMStringMap* impl) if (!wrapper.IsEmpty() && element) { v8::Handle elementValue = toV8(element); if (!elementValue.IsEmpty() && elementValue->IsObject()) - V8DOMWrapper::setHiddenReference(elementValue.As(), wrapper); + V8DOMWrapper::setNamedHiddenReference(elementValue.As(), "domStringMap", wrapper); } return wrapper; } diff --git a/Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp b/Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp index 171ff5c..08051ab 100644 --- a/Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp @@ -48,7 +48,7 @@ v8::Handle toV8(DOMTokenList* impl) if (!wrapper.IsEmpty() && element) { v8::Handle elementValue = toV8(element); if (!elementValue.IsEmpty() && elementValue->IsObject()) - V8DOMWrapper::setHiddenReference(elementValue.As(), wrapper); + V8DOMWrapper::setNamedHiddenReference(elementValue.As(), "domTokenList", wrapper); } return wrapper; } diff --git a/Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp b/Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp index e166323..e143a96 100644 --- a/Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp @@ -281,7 +281,7 @@ v8::Handle 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; } diff --git a/Source/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp b/Source/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp index b966e42..775e745 100644 --- a/Source/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp +++ b/Source/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp @@ -67,8 +67,8 @@ v8::Handle 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()); diff --git a/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp b/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp index d9e1de0..c9014c0 100644 --- a/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp @@ -83,7 +83,7 @@ v8::Handle 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; } diff --git a/Source/WebCore/bindings/v8/custom/V8StyleSheetCustom.cpp b/Source/WebCore/bindings/v8/custom/V8StyleSheetCustom.cpp index b3f6ff7..0fe6af8 100644 --- a/Source/WebCore/bindings/v8/custom/V8StyleSheetCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8StyleSheetCustom.cpp @@ -47,7 +47,7 @@ v8::Handle 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; } diff --git a/Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp b/Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp index af81a41..fa915b6 100644 --- a/Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp @@ -161,22 +161,27 @@ static v8::Handle toV8Object(WebGLExtension* extension, v8::Handle extensionObject; + const char* referenceName; switch (extension->getName()) { case WebGLExtension::WebKitLoseContextName: extensionObject = toV8(static_cast(extension)); + referenceName = "webKitLoseContextName"; break; case WebGLExtension::OESStandardDerivativesName: extensionObject = toV8(static_cast(extension)); + referenceName = "oesStandardDerivativesName"; break; case WebGLExtension::OESTextureFloatName: extensionObject = toV8(static_cast(extension)); + referenceName = "oesTextureFloatName"; break; case WebGLExtension::OESVertexArrayObjectName: extensionObject = toV8(static_cast(extension)); + referenceName = "oesVertexArrayObjectName"; break; } ASSERT(!extensionObject.IsEmpty()); - V8DOMWrapper::setHiddenReference(contextObject, extensionObject); + V8DOMWrapper::setNamedHiddenReference(contextObject, referenceName, extensionObject); return extensionObject; } -- cgit v1.1