Changeset 113272 in webkit


Ignore:
Timestamp:
Apr 4, 2012 6:38:28 PM (12 years ago)
Author:
adamk@chromium.org
Message:

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.

Location:
trunk/Source/WebCore
Files:
11 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r113268 r113272  
     12012-04-04  Adam Klein  <adamk@chromium.org>
     2
     3        Use PassRefPtr in V8DOMWrapper interface to avoid explicit ref() calls
     4        https://bugs.webkit.org/show_bug.cgi?id=82238
     5
     6        Reviewed by Adam Barth.
     7
     8        Relanding r112163 without modification, as it still seems valid.
     9        Will watch Chrome Canaries closely for any memory issues.
     10
     11        The setJSWrapper* methods previously featured a comment that asked
     12        callers to ref the objects before passing them in. This change makes
     13        that contract explicit (and allows the removal of the comment).
     14
     15        In addition, for ConstructorCallbacks, this change slightly reduces
     16        refcount churn by passing on the initial ref via RefPtr::release().
     17
     18        No new tests, no change in behavior.
     19
     20        * bindings/scripts/CodeGeneratorV8.pm:
     21        (GenerateConstructorCallback): Use RefPtr::release() to avoid refcount churn and remove explicit ref() call.
     22        (GenerateNamedConstructorCallback): ditto.
     23        * bindings/v8/V8DOMWindowShell.cpp:
     24        (WebCore::V8DOMWindowShell::installDOMWindow): Cast to a PassRefPtr and remove explicit ref call.
     25        * bindings/v8/V8DOMWrapper.cpp:
     26        (WebCore::V8DOMWrapper::setJSWrapperForDOMNode): Pass leaked refs into the DOMNodeMaps.
     27        * bindings/v8/V8DOMWrapper.h:
     28        (V8DOMWrapper): Make the setJSWrapperFor* methods take PassRefPtr<T>.
     29        (WebCore::V8DOMWrapper::setJSWrapperForDOMObject): Pass leaked ref into the DOMObjectMap.
     30        (WebCore::V8DOMWrapper::setJSWrapperForActiveDOMObject): Pass leaked ref into the ActiveDOMObjectMap.
     31        * bindings/v8/V8Proxy.h:
     32        (WebCore::toV8): Remove explicit ref.
     33        * bindings/v8/WorkerContextExecutionProxy.cpp:
     34        (WebCore::WorkerContextExecutionProxy::initContextIfNeeded): Cast to a PassRefPTr and remove explicit ref call.
     35        * bindings/v8/custom/V8HTMLImageElementConstructor.cpp:
     36        (WebCore::v8HTMLImageElementConstructorCallback): Use RefPtr::release() to avoid refcount churn and remove explicit ref.
     37        * bindings/v8/custom/V8WebKitMutationObserverCustom.cpp:
     38        (WebCore::V8WebKitMutationObserver::constructorCallback): ditto.
     39        * bindings/v8/custom/V8WebSocketCustom.cpp:
     40        (WebCore::V8WebSocket::constructorCallback): ditto.
     41        * bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:
     42        (WebCore::V8XMLHttpRequest::constructorCallback): ditto.
     43
    1442012-04-04  Chris Rogers  <crogers@google.com>
    245
  • trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm

    r113250 r113272  
    17631763
    17641764    V8DOMWrapper::setDOMWrapper(wrapper, &info, impl.get());
    1765     impl->ref();
    1766     V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.get(), v8::Persistent<v8::Object>::New(wrapper));
     1765    V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.release(), v8::Persistent<v8::Object>::New(wrapper));
    17671766    return args.Holder();
    17681767END
     
    19451944
    19461945    V8DOMWrapper::setDOMWrapper(wrapper, &V8${implClassName}Constructor::info, impl.get());
    1947     impl->ref();
    1948     V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.get(), v8::Persistent<v8::Object>::New(wrapper));
     1946    V8DOMWrapper::setJSWrapperFor${DOMObject}(impl.release(), v8::Persistent<v8::Object>::New(wrapper));
    19491947    return args.Holder();
    19501948END
  • trunk/Source/WebCore/bindings/v8/V8DOMWindowShell.cpp

    r113250 r113272  
    412412    V8DOMWrapper::setDOMWrapper(v8::Handle<v8::Object>::Cast(jsWindow->GetPrototype()), &V8DOMWindow::info, window);
    413413
    414     window->ref();
    415     V8DOMWrapper::setJSWrapperForDOMObject(window, v8::Persistent<v8::Object>::New(jsWindow));
     414    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<DOMWindow>(window), v8::Persistent<v8::Object>::New(jsWindow));
    416415
    417416    // Insert the window instance as the prototype of the shadow object.
  • trunk/Source/WebCore/bindings/v8/V8DOMWrapper.cpp

    r113250 r113272  
    7070namespace WebCore {
    7171
    72 // The caller must have increased obj's ref count.
    73 void V8DOMWrapper::setJSWrapperForDOMObject(void* object, v8::Persistent<v8::Object> wrapper)
    74 {
    75     ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
    76     ASSERT(!domWrapperType(wrapper)->toActiveDOMObjectFunction);
    77     getDOMObjectMap().set(object, wrapper);
    78 }
    79 
    80 // The caller must have increased obj's ref count.
    81 void V8DOMWrapper::setJSWrapperForActiveDOMObject(void* object, v8::Persistent<v8::Object> wrapper)
    82 {
    83     ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
    84     ASSERT(domWrapperType(wrapper)->toActiveDOMObjectFunction);
    85     getActiveDOMObjectMap().set(object, wrapper);
    86 }
    87 
    88 // The caller must have increased node's ref count.
    89 void V8DOMWrapper::setJSWrapperForDOMNode(Node* node, v8::Persistent<v8::Object> wrapper)
    90 {
    91     ASSERT(V8DOMWrapper::maybeDOMWrapper(wrapper));
     72void V8DOMWrapper::setJSWrapperForDOMNode(PassRefPtr<Node> node, v8::Persistent<v8::Object> wrapper)
     73{
     74    ASSERT(maybeDOMWrapper(wrapper));
    9275    if (node->isActiveNode())
    93         getActiveDOMNodeMap().set(node, wrapper);
     76        getActiveDOMNodeMap().set(node.leakRef(), wrapper);
    9477    else
    95         getDOMNodeMap().set(node, wrapper);
     78        getDOMNodeMap().set(node.leakRef(), wrapper);
    9679}
    9780
  • trunk/Source/WebCore/bindings/v8/V8DOMWrapper.h

    r113250 r113272  
    3939#include "PlatformString.h"
    4040#include "V8CustomXPathNSResolver.h"
     41#include "V8DOMMap.h"
    4142#include "V8Event.h"
    4243#include "V8IsolatedContext.h"
     
    4748#include <v8.h>
    4849#include <wtf/MainThread.h>
     50#include <wtf/PassRefPtr.h>
    4951
    5052namespace WebCore {
     
    105107#endif
    106108
    107         // Set JS wrapper of a DOM object, the caller in charge of increase ref.
    108         static void setJSWrapperForDOMObject(void*, v8::Persistent<v8::Object>);
    109         static void setJSWrapperForActiveDOMObject(void*, v8::Persistent<v8::Object>);
    110         static void setJSWrapperForDOMNode(Node*, v8::Persistent<v8::Object>);
     109        template<typename T> static void setJSWrapperForDOMObject(PassRefPtr<T>, v8::Persistent<v8::Object>);
     110        template<typename T> static void setJSWrapperForActiveDOMObject(PassRefPtr<T>, v8::Persistent<v8::Object>);
     111        static void setJSWrapperForDOMNode(PassRefPtr<Node>, v8::Persistent<v8::Object>);
    111112
    112113        static bool isValidDOMObject(v8::Handle<v8::Value>);
     
    152153#endif
    153154    };
     155
     156    template<typename T>
     157    void V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<T> object, v8::Persistent<v8::Object> wrapper)
     158    {
     159        ASSERT(maybeDOMWrapper(wrapper));
     160        ASSERT(!domWrapperType(wrapper)->toActiveDOMObjectFunction);
     161        getDOMObjectMap().set(object.leakRef(), wrapper);
     162    }
     163
     164    template<typename T>
     165    void V8DOMWrapper::setJSWrapperForActiveDOMObject(PassRefPtr<T> object, v8::Persistent<v8::Object> wrapper)
     166    {
     167        ASSERT(maybeDOMWrapper(wrapper));
     168        ASSERT(domWrapperType(wrapper)->toActiveDOMObjectFunction);
     169        getActiveDOMObjectMap().set(object.leakRef(), wrapper);
     170    }
    154171}
    155172
  • trunk/Source/WebCore/bindings/v8/V8Proxy.h

    r113250 r113272  
    355355    template <class T> inline v8::Handle<v8::Object> toV8(PassRefPtr<T> object, v8::Local<v8::Object> holder, IndependentMode independent = DoNotMarkIndependent)
    356356    {
    357         object->ref();
    358357        v8::Persistent<v8::Object> handle = v8::Persistent<v8::Object>::New(holder);
    359358        if (independent == MarkIndependent)
    360359            handle.MarkIndependent();
    361         V8DOMWrapper::setJSWrapperForDOMObject(object.get(), handle);
     360        V8DOMWrapper::setJSWrapperForDOMObject(object, handle);
    362361        return holder;
    363362    }
  • trunk/Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp

    r113250 r113272  
    180180    V8DOMWrapper::setDOMWrapper(jsWorkerContext, contextType, m_workerContext);
    181181
    182     V8DOMWrapper::setJSWrapperForDOMObject(m_workerContext, v8::Persistent<v8::Object>::New(jsWorkerContext));
    183     m_workerContext->ref();
     182    V8DOMWrapper::setJSWrapperForDOMObject(PassRefPtr<WorkerContext>(m_workerContext), v8::Persistent<v8::Object>::New(jsWorkerContext));
    184183
    185184    // Insert the object instance as the prototype of the shadow object.
  • trunk/Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.cpp

    r112908 r113272  
    8787    RefPtr<HTMLImageElement> image = HTMLImageElement::createForJSConstructor(document, optionalWidth, optionalHeight);
    8888    V8DOMWrapper::setDOMWrapper(args.Holder(), &V8HTMLImageElementConstructor::info, image.get());
    89     image->ref();
    90     V8DOMWrapper::setJSWrapperForDOMNode(image.get(), v8::Persistent<v8::Object>::New(args.Holder()));
     89    V8DOMWrapper::setJSWrapperForDOMNode(image.release(), v8::Persistent<v8::Object>::New(args.Holder()));
    9190    return args.Holder();
    9291}
  • trunk/Source/WebCore/bindings/v8/custom/V8WebKitMutationObserverCustom.cpp

    r112908 r113272  
    7676
    7777    V8DOMWrapper::setDOMWrapper(args.Holder(), &info, observer.get());
    78     observer->ref();
    79     V8DOMWrapper::setJSWrapperForDOMObject(observer.get(), v8::Persistent<v8::Object>::New(args.Holder()));
     78    V8DOMWrapper::setJSWrapperForDOMObject(observer.release(), v8::Persistent<v8::Object>::New(args.Holder()));
    8079    return args.Holder();
    8180}
  • trunk/Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp

    r112908 r113272  
    108108        return throwError(ec);
    109109
    110     // Setup the standard wrapper object internal fields.
    111110    V8DOMWrapper::setDOMWrapper(args.Holder(), &info, webSocket.get());
    112 
    113     // Add object to the wrapper map.
    114     webSocket->ref();
    115     V8DOMWrapper::setJSWrapperForActiveDOMObject(webSocket.get(), v8::Persistent<v8::Object>::New(args.Holder()));
    116 
     111    V8DOMWrapper::setJSWrapperForActiveDOMObject(webSocket.release(), v8::Persistent<v8::Object>::New(args.Holder()));
    117112    return args.Holder();
    118113}
  • trunk/Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp

    r112908 r113272  
    6565    RefPtr<XMLHttpRequest> xmlHttpRequest = XMLHttpRequest::create(context, securityOrigin);
    6666    V8DOMWrapper::setDOMWrapper(args.Holder(), &info, xmlHttpRequest.get());
    67 
    68     // Add object to the wrapper map.
    69     xmlHttpRequest->ref();
    70     V8DOMWrapper::setJSWrapperForActiveDOMObject(xmlHttpRequest.get(), v8::Persistent<v8::Object>::New(args.Holder()));
     67    V8DOMWrapper::setJSWrapperForActiveDOMObject(xmlHttpRequest.release(), v8::Persistent<v8::Object>::New(args.Holder()));
    7168    return args.Holder();
    7269}
Note: See TracChangeset for help on using the changeset viewer.