Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls
authoradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Apr 2012 01:38:28 +0000 (01:38 +0000)
committeradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 5 Apr 2012 01:38:28 +0000 (01:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=82238

Reviewed by Adam Barth.

Relanding r112163 without modification, as it still seems valid.
Will watch Chrome Canaries closely for any memory issues.

The setJSWrapper* methods previously featured a comment that asked
callers to ref the objects before passing them in. This change makes
that contract explicit (and allows the removal of the comment).

In addition, for ConstructorCallbacks, this change slightly reduces
refcount churn by passing on the initial ref via RefPtr::release().

No new tests, no change in behavior.

* bindings/scripts/CodeGeneratorV8.pm:
(GenerateConstructorCallback): Use RefPtr::release() to avoid refcount churn and remove explicit ref() call.
(GenerateNamedConstructorCallback): ditto.
* bindings/v8/V8DOMWindowShell.cpp:
(WebCore::V8DOMWindowShell::installDOMWindow): Cast to a PassRefPtr and remove explicit ref call.
* bindings/v8/V8DOMWrapper.cpp:
(WebCore::V8DOMWrapper::setJSWrapperForDOMNode): Pass leaked refs into the DOMNodeMaps.
* bindings/v8/V8DOMWrapper.h:
(V8DOMWrapper): Make the setJSWrapperFor* methods take PassRefPtr<T>.
(WebCore::V8DOMWrapper::setJSWrapperForDOMObject): Pass leaked ref into the DOMObjectMap.
(WebCore::V8DOMWrapper::setJSWrapperForActiveDOMObject): Pass leaked ref into the ActiveDOMObjectMap.
* bindings/v8/V8Proxy.h:
(WebCore::toV8): Remove explicit ref.
* bindings/v8/WorkerContextExecutionProxy.cpp:
(WebCore::WorkerContextExecutionProxy::initContextIfNeeded): Cast to a PassRefPTr and remove explicit ref call.
* bindings/v8/custom/V8HTMLImageElementConstructor.cpp:
(WebCore::v8HTMLImageElementConstructorCallback): Use RefPtr::release() to avoid refcount churn and remove explicit ref.
* bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:
(WebCore::V8WebKitMutationObserver::constructorCallback): ditto.
* bindings/v8/custom/V8WebSocketCustom.cpp:
(WebCore::V8WebSocket::constructorCallback): ditto.
* bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:
(WebCore::V8XMLHttpRequest::constructorCallback): ditto.

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

Source/WebCore/ChangeLog
Source/WebCore/bindings/scripts/CodeGeneratorV8.pm
Source/WebCore/bindings/v8/V8DOMWindowShell.cpp
Source/WebCore/bindings/v8/V8DOMWrapper.cpp
Source/WebCore/bindings/v8/V8DOMWrapper.h
Source/WebCore/bindings/v8/V8Proxy.h
Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.cpp
Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp
Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp
Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp

index 1fb2090..474be8b 100644 (file)
@@ -1,3 +1,46 @@
+2012-04-04  Adam Klein  <adamk@chromium.org>
+
+        Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls
+        https://bugs.webkit.org/show_bug.cgi?id=82238
+
+        Reviewed by Adam Barth.
+
+        Relanding r112163 without modification, as it still seems valid.
+        Will watch Chrome Canaries closely for any memory issues.
+
+        The setJSWrapper* methods previously featured a comment that asked
+        callers to ref the objects before passing them in. This change makes
+        that contract explicit (and allows the removal of the comment).
+
+        In addition, for ConstructorCallbacks, this change slightly reduces
+        refcount churn by passing on the initial ref via RefPtr::release().
+
+        No new tests, no change in behavior.
+
+        * bindings/scripts/CodeGeneratorV8.pm:
+        (GenerateConstructorCallback): Use RefPtr::release() to avoid refcount churn and remove explicit ref() call.
+        (GenerateNamedConstructorCallback): ditto.
+        * bindings/v8/V8DOMWindowShell.cpp:
+        (WebCore::V8DOMWindowShell::installDOMWindow): Cast to a PassRefPtr and remove explicit ref call.
+        * bindings/v8/V8DOMWrapper.cpp:
+        (WebCore::V8DOMWrapper::setJSWrapperForDOMNode): Pass leaked refs into the DOMNodeMaps.
+        * bindings/v8/V8DOMWrapper.h:
+        (V8DOMWrapper): Make the setJSWrapperFor* methods take PassRefPtr<T>.
+        (WebCore::V8DOMWrapper::setJSWrapperForDOMObject): Pass leaked ref into the DOMObjectMap.
+        (WebCore::V8DOMWrapper::setJSWrapperForActiveDOMObject): Pass leaked ref into the ActiveDOMObjectMap.
+        * bindings/v8/V8Proxy.h:
+        (WebCore::toV8): Remove explicit ref.
+        * bindings/v8/WorkerContextExecutionProxy.cpp:
+        (WebCore::WorkerContextExecutionProxy::initContextIfNeeded): Cast to a PassRefPTr and remove explicit ref call.
+        * bindings/v8/custom/V8HTMLImageElementConstructor.cpp:
+        (WebCore::v8HTMLImageElementConstructorCallback): Use RefPtr::release() to avoid refcount churn and remove explicit ref.
+        * bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:
+        (WebCore::V8WebKitMutationObserver::constructorCallback): ditto.
+        * bindings/v8/custom/V8WebSocketCustom.cpp:
+        (WebCore::V8WebSocket::constructorCallback): ditto.
+        * bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:
+        (WebCore::V8XMLHttpRequest::constructorCallback): ditto.
+
 2012-04-04  Chris Rogers  <crogers@google.com>
 
         WaveTable::waveDataForFundamentalFrequency() should properly interpret negative frequency
index d8e212d..971cfa6 100644 (file)
@@ -1762,8 +1762,7 @@ END
     push(@implContent, <<END);
 
     V8DOMWrapper::setDOMWrapper(wrapper, &info, impl.get());
-    impl->ref();
-    V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.get(), v8::Persistent<v8::Object>::New(wrapper));
+    V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.release(), v8::Persistent<v8::Object>::New(wrapper));
     return args.Holder();
 END
 
@@ -1944,8 +1943,7 @@ END
     push(@implContent, <<END);
 
     V8DOMWrapper::setDOMWrapper(wrapper, &V8${implClassName}Constructor::info, impl.get());
-    impl->ref();
-    V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.get(), v8::Persistent<v8::Object>::New(wrapper));
+    V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.release(), v8::Persistent<v8::Object>::New(wrapper));
     return args.Holder();
 END
 
index 3e16904..c094450 100644 (file)
@@ -411,8 +411,7 @@ bool V8DOMWindowShell::installDOMWindow(v8::Handle<v8::Context> context, DOMWind
     V8DOMWrapper::setDOMWrapper(jsWindow, &V8DOMWindow::info, window);
     V8DOMWrapper::setDOMWrapper(v8::Handle<v8::Object>::Cast(jsWindow->GetPrototype()), &V8DOMWindow::info, window);
 
-    window->ref();
-    V8DOMWrapper::setJSWrapperForDOMObject(window, v8::Persistent<v8::Object>::New(jsWindow));
+    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), v8::Persistent<v8::Object>::New(jsWindow));
 
     // Insert the window instance as the prototype of the shadow object.
     v8::Handle<v8::Object> v8RealGlobal = v8::Handle<v8::Object>::Cast(context->Global()->GetPrototype());
index a03cbb8..84fd172 100644 (file)
 
 namespace WebCore {
 
-// The caller must have increased obj's ref count.
-void V8DOMWrapper::setJSWrapperForDOMObject(void* object, v8::Persistent<v8::Object> wrapper)
+void V8DOMWrapper::setJSWrapperForDOMNode(PassRefPtr<Node> node, v8::Persistent<v8::Object> wrapper)
 {
-    ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
-    ASSERT(!domWrapperType(wrapper)->toActiveDOMObjectFunction);
-    getDOMObjectMap().set(object, wrapper);
-}
-
-// The caller must have increased obj's ref count.
-void V8DOMWrapper::setJSWrapperForActiveDOMObject(void* object, v8::Persistent<v8::Object> wrapper)
-{
-    ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
-    ASSERT(domWrapperType(wrapper)->toActiveDOMObjectFunction);
-    getActiveDOMObjectMap().set(object, wrapper);
-}
-
-// The caller must have increased node's ref count.
-void V8DOMWrapper::setJSWrapperForDOMNode(Node* node, v8::Persistent<v8::Object> wrapper)
-{
-    ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
+    ASSERT(maybeDOMWrapper(wrapper));
     if (node->isActiveNode())
-        getActiveDOMNodeMap().set(node, wrapper);
+        getActiveDOMNodeMap().set(node.leakRef(), wrapper);
     else
-        getDOMNodeMap().set(node, wrapper);
+        getDOMNodeMap().set(node.leakRef(), wrapper);
 }
 
 v8::Local<v8::Function> V8DOMWrapper::constructorForType(WrapperTypeInfo* type, DOMWindow* window)
index 66e7269..d766307 100644 (file)
@@ -38,6 +38,7 @@
 #include "NodeFilter.h"
 #include "PlatformString.h"
 #include "V8CustomXPathNSResolver.h"
+#include "V8DOMMap.h"
 #include "V8Event.h"
 #include "V8IsolatedContext.h"
 #include "V8Utilities.h"
@@ -46,6 +47,7 @@
 #include "XPathNSResolver.h"
 #include <v8.h>
 #include <wtf/MainThread.h>
+#include <wtf/PassRefPtr.h>
 
 namespace WebCore {
 
@@ -104,10 +106,9 @@ namespace WebCore {
         static v8::Local<v8::Function> constructorForType(WrapperTypeInfo*, WorkerContext*);
 #endif
 
-        // Set JS wrapper of a DOM object, the caller in charge of increase ref.
-        static void setJSWrapperForDOMObject(void*, v8::Persistent<v8::Object>);
-        static void setJSWrapperForActiveDOMObject(void*, v8::Persistent<v8::Object>);
-        static void setJSWrapperForDOMNode(Node*, v8::Persistent<v8::Object>);
+        template<typename T> static void setJSWrapperForDOMObject(PassRefPtr<T>, v8::Persistent<v8::Object>);
+        template<typename T> static void setJSWrapperForActiveDOMObject(PassRefPtr<T>, v8::Persistent<v8::Object>);
+        static void setJSWrapperForDOMNode(PassRefPtr<Node>, v8::Persistent<v8::Object>);
 
         static bool isValidDOMObject(v8::Handle<v8::Value>);
 
@@ -151,6 +152,22 @@ namespace WebCore {
         static V8BindingPerContextData* perContextData(WorkerContext*);
 #endif
     };
+
+    template<typename T>
+    void V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<T> object, v8::Persistent<v8::Object> wrapper)
+    {
+        ASSERT(maybeDOMWrapper(wrapper));
+        ASSERT(!domWrapperType(wrapper)->toActiveDOMObjectFunction);
+        getDOMObjectMap().set(object.leakRef(), wrapper);
+    }
+
+    template<typename T>
+    void V8DOMWrapper::setJSWrapperForActiveDOMObject(PassRefPtr<T> object, v8::Persistent<v8::Object> wrapper)
+    {
+        ASSERT(maybeDOMWrapper(wrapper));
+        ASSERT(domWrapperType(wrapper)->toActiveDOMObjectFunction);
+        getActiveDOMObjectMap().set(object.leakRef(), wrapper);
+    }
 }
 
 #endif // V8DOMWrapper_h
index 494ca3c..fdd8bb7 100644 (file)
@@ -354,11 +354,10 @@ namespace WebCore {
 
     template <class T> inline v8::Handle<v8::Object> toV8(PassRefPtr<T> object, v8::Local<v8::Object> holder, IndependentMode independent = DoNotMarkIndependent)
     {
-        object->ref();
         v8::Persistent<v8::Object> handle = v8::Persistent<v8::Object>::New(holder);
         if (independent == MarkIndependent)
             handle.MarkIndependent();
-        V8DOMWrapper::setJSWrapperForDOMObject(object.get(), handle);
+        V8DOMWrapper::setJSWrapperForDOMObject(object, handle);
         return holder;
     }
 
index cc5c1f8..1f4848b 100644 (file)
@@ -179,8 +179,7 @@ bool WorkerContextExecutionProxy::initContextIfNeeded()
     // Wrap the object.
     V8DOMWrapper::setDOMWrapper(jsWorkerContext, contextType, m_workerContext);
 
-    V8DOMWrapper::setJSWrapperForDOMObject(m_workerContext, v8::Persistent<v8::Object>::New(jsWorkerContext));
-    m_workerContext->ref();
+    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<WorkerContext>(m_workerContext), v8::Persistent<v8::Object>::New(jsWorkerContext));
 
     // Insert the object instance as the prototype of the shadow object.
     v8::Handle<v8::Object> globalObject = v8::Handle<v8::Object>::Cast(m_context->Global()->GetPrototype());
index 67b9d1a..1c2fe66 100644 (file)
@@ -86,8 +86,7 @@ static v8::Handle<v8::Value> v8HTMLImageElementConstructorCallback(const v8::Arg
 
     RefPtr<HTMLImageElement> image = HTMLImageElement::createForJSConstructor(document, optionalWidth, optionalHeight);
     V8DOMWrapper::setDOMWrapper(args.Holder(), &V8HTMLImageElementConstructor::info, image.get());
-    image->ref();
-    V8DOMWrapper::setJSWrapperForDOMNode(image.get(), v8::Persistent<v8::Object>::New(args.Holder()));
+    V8DOMWrapper::setJSWrapperForDOMNode(image.release(), v8::Persistent<v8::Object>::New(args.Holder()));
     return args.Holder();
 }
 
index f7189b7..53f3442 100644 (file)
@@ -75,8 +75,7 @@ v8::Handle<v8::Value> V8WebKitMutationObserver::constructorCallback(const v8::Ar
     RefPtr<WebKitMutationObserver> observer = WebKitMutationObserver::create(callback.release());
 
     V8DOMWrapper::setDOMWrapper(args.Holder(), &info, observer.get());
-    observer->ref();
-    V8DOMWrapper::setJSWrapperForDOMObject(observer.get(), v8::Persistent<v8::Object>::New(args.Holder()));
+    V8DOMWrapper::setJSWrapperForDOMObject(observer.release(), v8::Persistent<v8::Object>::New(args.Holder()));
     return args.Holder();
 }
 
index 9b5e10b..5c1661e 100644 (file)
@@ -107,13 +107,8 @@ v8::Handle<v8::Value> V8WebSocket::constructorCallback(const v8::Arguments& args
     if (ec)
         return throwError(ec);
 
-    // Setup the standard wrapper object internal fields.
     V8DOMWrapper::setDOMWrapper(args.Holder(), &info, webSocket.get());
-
-    // Add object to the wrapper map.
-    webSocket->ref();
-    V8DOMWrapper::setJSWrapperForActiveDOMObject(webSocket.get(), v8::Persistent<v8::Object>::New(args.Holder()));
-
+    V8DOMWrapper::setJSWrapperForActiveDOMObject(webSocket.release(), v8::Persistent<v8::Object>::New(args.Holder()));
     return args.Holder();
 }
 
index 5a5b0c6..71d7dd2 100644 (file)
@@ -64,10 +64,7 @@ v8::Handle<v8::Value> V8XMLHttpRequest::constructorCallback(const v8::Arguments&
         securityOrigin = isolatedContext->securityOrigin();
     RefPtr<XMLHttpRequest> xmlHttpRequest = XMLHttpRequest::create(context, securityOrigin);
     V8DOMWrapper::setDOMWrapper(args.Holder(), &info, xmlHttpRequest.get());
-
-    // Add object to the wrapper map.
-    xmlHttpRequest->ref();
-    V8DOMWrapper::setJSWrapperForActiveDOMObject(xmlHttpRequest.get(), v8::Persistent<v8::Object>::New(args.Holder()));
+    V8DOMWrapper::setJSWrapperForActiveDOMObject(xmlHttpRequest.release(), v8::Persistent<v8::Object>::New(args.Holder()));
     return args.Holder();
 }