[GTK] GObject DOM bindings object are cached forever

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

DOMObjectCache::clearByFrame() is never called in WebKit2, so all objects are cached until the global cache is deleted (end of web process). In WebKit1 we call clearByFrame() from FrameLoaderClient::setMainFrameDocumentReady() when ready == False, and WebKitWebFrame when the core Frame object is destroyed.

[Carlos Garcia Campos]

Created attachment 246665 [details]
Patch

I've reworked the DOMObjectCache so that we no longer need to manually clear objects when a frame is destroyed. I've also cleaned up the code that was quite difficult to follow, added a bunch of assertions if DOM objects are leaked, and a new test case specific for the cache.

[Sergio Villar Senin]

Comment on attachment 246665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246665&action=review

Awesome patch. The code is highly simplified and the new test coverage is simply superb.

I have a few comments.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:73
> +static DOMObjectCacheFrameObserver& getOrCreateDOMObjectCacheFrameObserver(WebCore::Frame* frame)

As frame is never null this should be a reference.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:87
> +    DOMObjectCacheFrameObserver(WebCore::Frame* frame)

This should also be ideally a reference but the interface is a pointer :(

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:97
> +    void addObjectCacheData(DOMObjectCacheData* data)

This can be a reference.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:101
> +        m_objects.append(data);

See bellow my comment about m_objects.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:109
> +        observer->m_objects.removeFirstMatching([finalizedObject](DOMObjectCacheData* data) {

See bellow my comment about m_objects.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:120
> +        for (auto data : objects) {

I think it's preferred to be explicit about the type being a pointer, so "auto*" it's slightly better in this case.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:139
> +    Vector<DOMObjectCacheData*> m_objects;

Since respecting the insertion order is not needed, I think we should use a HashSet here. Being a set means that we can be sure that the objects are only once in the data structure and will also help with searches (contains() and the like).

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:164
>      if (domObjects().get(objectHandle))

!domObjects().contains(objectHandle) ?

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:171
> +    if (domObjects().get(objectHandle))

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMNodeTest.cpp:216
> +        // Get the same elment twice should return the same pointer.

Nit: element

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 246665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246665&action=review
> 
> Awesome patch. The code is highly simplified and the new test coverage is
> simply superb.

Thanks, hopefully it's also correct :-)

> I have a few comments.
> 
> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:73
> > +static DOMObjectCacheFrameObserver& getOrCreateDOMObjectCacheFrameObserver(WebCore::Frame* frame)
> 
> As frame is never null this should be a reference.

Ok.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:87
> > +    DOMObjectCacheFrameObserver(WebCore::Frame* frame)
> 
> This should also be ideally a reference but the interface is a pointer :(

We can still receive a Ref and pass the address to the parent constructor.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:97
> > +    void addObjectCacheData(DOMObjectCacheData* data)
> 
> This can be a reference.

Ok.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:101
> > +        m_objects.append(data);
> 
> See bellow my comment about m_objects.
> 
> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:109
> > +        observer->m_objects.removeFirstMatching([finalizedObject](DOMObjectCacheData* data) {
> 
> See bellow my comment about m_objects.
> 
> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:120
> > +        for (auto data : objects) {
> 
> I think it's preferred to be explicit about the type being a pointer, so
> "auto*" it's slightly better in this case.

Ok.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:139
> > +    Vector<DOMObjectCacheData*> m_objects;
> 
> Since respecting the insertion order is not needed, I think we should use a
> HashSet here. Being a set means that we can be sure that the objects are
> only once in the data structure and will also help with searches (contains()
> and the like).

It was a HashSet initially, I don't remember why I changed to a Vector.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:164
> >      if (domObjects().get(objectHandle))
> 
> !domObjects().contains(objectHandle) ?

Actually domObjects().contains(objectHandle), since we want to return early when the wrapped object is already cached, but yes, contains is better thnn get in this case.

> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:171
> > +    if (domObjects().get(objectHandle))
> 
> Ditto.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/DOMNodeTest.cpp:216
> > +        // Get the same elment twice should return the same pointer.
> 
> Nit: element

Sure.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:139
> > > +    Vector<DOMObjectCacheData*> m_objects;
> > 
> > Since respecting the insertion order is not needed, I think we should use a
> > HashSet here. Being a set means that we can be sure that the objects are
> > only once in the data structure and will also help with searches (contains()
> > and the like).
> 
> It was a HashSet initially, I don't remember why I changed to a Vector.

I remember now. Switched to a Vector in the end mainly because of removeFirstMatching() that makes very convenient to find the data containing the GObject that has finalized. Note that we don't actually need hashes here, adding the same object twice would be a bug, and there's an assert for that case indeed. We always want either iterate all the elements, or find a particular element, but not using the data pointer, but the GObject, because in objectFinalizedCallback we don't have the data pointer corresponding to the finalized GObject. Also, all the contains uses are inside asserts. We could improve a bit the Vector by stack allocating a few entries, 8 for example.

[Carlos Garcia Campos]

Committed r180214: <http://trac.webkit.org/changeset/180214>

[Zan Dobersek]

Comment on attachment 246665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246665&action=review

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:82
> +    auto observerPtr = domObjectCacheFrameObservers().get(frame);
> +    if (observerPtr)
> +        return *observerPtr;
> +
> +    std::unique_ptr<DOMObjectCacheFrameObserver> observer = std::make_unique<DOMObjectCacheFrameObserver>(frame);
> +    observerPtr = observer.get();
> +    domObjectCacheFrameObservers().set(frame, WTF::move(observer));
> +    return *observerPtr;

This could just call HashMap<>::add(), and then use the AddResult object that's returned to set the observer object if necessary.
Document::addAutoSizingNode() is one example of that approach.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:166
> +    domObjects().set(objectHandle, std::make_unique<DOMObjectCacheData>(G_OBJECT(wrapper)));

Same here.

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:176
> +    domObjects().set(objectHandle, WTF::move(data));

Same here.

[Carlos Garcia Campos]

Committed r180216: <http://trac.webkit.org/changeset/180216>