Merge r180214 - [GTK] GObject DOM bindings object are cached forever

https://bugs.webkit.org/show_bug.cgi?id=141558

Reviewed by Sergio Villar Senin.

Source/WebCore:

Rework the DOMObjectCache to avoid having to manually clear the
objects when the frame is destroyed, using a FrameDestructionObserver
instead. When a new object associated to a Frame is added to the
cache, a FrameDestructionObserver is created for the frame if
needed, and the DOM object is tracked by the observer too. When
the frame is detached from the page all its objects are cleared,
and if the aren't additional references, the object is finalized
and removed from the cache normally.
This patch also simplifies and modernizes the code to make it
easier to read an maintain.

* bindings/gobject/DOMObjectCache.cpp:
(WebKit::DOMObjectCacheData::DOMObjectCacheData): Add constructor
to initialize its members and simplify the callers.
(WebKit::DOMObjectCacheData::clearObject): Remove the references
added by the cache, ensuring the GObject is not finalized until
the method returns.
(WebKit::DOMObjectCacheData::refObject): Adds a reference owned by
the cache.
(WebKit::domObjectCacheFrameObservers): Map a frame to a FrameDestructionObserver.
(WebKit::getOrCreateDOMObjectCacheFrameObserver): Create a new
FrameDestructionObserver for the given Frame or return the
existing one.
(WebKit::domObjects): Map wrapped object to wrapper object.
(WebKit::DOMObjectCache::forget): Remove the wrapped object from
the cache.
(WebKit::DOMObjectCache::get): Return the wrapped object if it is
in the cache, increasing the cache references.
(WebKit::DOMObjectCache::put): Add the wrapper object to the cache
for the given wrapped object. If it's a Node and has a frame add
the node to the FrameDestructionObserver corresponding to the frame.
(WebKit::getFrameFromHandle): Deleted.
(WebKit::weakRefNotify): Deleted.
(WebKit::DOMObjectCache::clearByFrame): Deleted.
(WebKit::DOMObjectCache::~DOMObjectCache): Deleted.
* bindings/gobject/DOMObjectCache.h:

Merge r181631 - [GTK] WebKitDOM objects leaking
https://bugs.webkit.org/show_bug.cgi?id=118788

Reviewed by Darin Adler and Sergio Villar Senin.

Source/WebCore:

Use a DOMwindowObserver class, derived from DOMWindowProperty to
be notified when the window object is detached from the frame to
clear the DOM objects associated to that frame in that case too.

* bindings/gobject/DOMObjectCache.cpp:

Source/WebCore/ChangeLog

@@ -1,3 +1,60 @@
+2015-03-17  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GTK] WebKitDOM objects leaking
+        https://bugs.webkit.org/show_bug.cgi?id=118788
+
+        Reviewed by Darin Adler and Sergio Villar Senin.
+
+        Use a DOMwindowObserver class, derived from DOMWindowProperty to
+        be notified when the window object is detached from the frame to
+        clear the DOM objects associated to that frame in that case too.
+
+        * bindings/gobject/DOMObjectCache.cpp:
+
+2015-02-17  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GTK] GObject DOM bindings object are cached forever
+        https://bugs.webkit.org/show_bug.cgi?id=141558
+
+        Reviewed by Sergio Villar Senin.
+
+        Rework the DOMObjectCache to avoid having to manually clear the
+        objects when the frame is destroyed, using a FrameDestructionObserver
+        instead. When a new object associated to a Frame is added to the
+        cache, a FrameDestructionObserver is created for the frame if
+        needed, and the DOM object is tracked by the observer too. When
+        the frame is detached from the page all its objects are cleared,
+        and if the aren't additional references, the object is finalized
+        and removed from the cache normally.
+        This patch also simplifies and modernizes the code to make it
+        easier to read an maintain.
+
+        * bindings/gobject/DOMObjectCache.cpp:
+        (WebKit::DOMObjectCacheData::DOMObjectCacheData): Add constructor
+        to initialize its members and simplify the callers.
+        (WebKit::DOMObjectCacheData::clearObject): Remove the references
+        added by the cache, ensuring the GObject is not finalized until
+        the method returns.
+        (WebKit::DOMObjectCacheData::refObject): Adds a reference owned by
+        the cache.
+        (WebKit::domObjectCacheFrameObservers): Map a frame to a FrameDestructionObserver.
+        (WebKit::getOrCreateDOMObjectCacheFrameObserver): Create a new
+        FrameDestructionObserver for the given Frame or return the
+        existing one.
+        (WebKit::domObjects): Map wrapped object to wrapper object.
+        (WebKit::DOMObjectCache::forget): Remove the wrapped object from
+        the cache.
+        (WebKit::DOMObjectCache::get): Return the wrapped object if it is
+        in the cache, increasing the cache references.
+        (WebKit::DOMObjectCache::put): Add the wrapper object to the cache
+        for the given wrapped object. If it's a Node and has a frame add
+        the node to the FrameDestructionObserver corresponding to the frame.
+        (WebKit::getFrameFromHandle): Deleted.
+        (WebKit::weakRefNotify): Deleted.
+        (WebKit::DOMObjectCache::clearByFrame): Deleted.
+        (WebKit::DOMObjectCache::~DOMObjectCache): Deleted.
+        * bindings/gobject/DOMObjectCache.h:
+
 2015-04-07  Milan Crha  <mcrha@redhat.com>
 
         [GTK] [WebKit1] Crash under WebCore::ScrollView::contentsToWindow()

Source/WebCore/bindings/gobject/DOMObjectCache.cpp

@@ -1,5 +1,5 @@
 /*
- *  Copyright (C) 2010 Igalia S.L.
+ *  Copyright (C) 2010, 2015 Igalia S.L.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -19,142 +19,203 @@
 #include "config.h"
 #include "DOMObjectCache.h"
 
+#include "DOMWindowProperty.h"
 #include "Document.h"
+#include "Frame.h"
+#include "FrameDestructionObserver.h"
 #include "Node.h"
 #include <glib-object.h>
 #include <wtf/HashMap.h>
+#include <wtf/NeverDestroyed.h>
+#include <wtf/Vector.h>
+#include <wtf/gobject/GRefPtr.h>
 
 namespace WebKit {
 
-typedef struct {
+struct DOMObjectCacheData {
+    DOMObjectCacheData(GObject* wrapper)
+        : object(wrapper)
+        , cacheReferences(1)
+    {
+    }
+
+    void clearObject()
+    {
+        ASSERT(object);
+        ASSERT(cacheReferences >= 1);
+
+        GRefPtr<GObject> protect(object);
+        do {
+            g_object_unref(object);
+        } while (--cacheReferences);
+        object = nullptr;
+    }
+
+    void* refObject()
+    {
+        ASSERT(object);
+
+        cacheReferences++;
+        return g_object_ref(object);
+    }
+
     GObject* object;
-    WebCore::Frame* frame;
-    guint timesReturned;
-} DOMObjectCacheData;
+    unsigned cacheReferences;
+};
 
-typedef HashMap<void*, DOMObjectCacheData*> DOMObjectMap;
+class DOMObjectCacheFrameObserver;
+typedef HashMap<WebCore::Frame*, std::unique_ptr<DOMObjectCacheFrameObserver>> DOMObjectCacheFrameObserverMap;
 
-static DOMObjectMap& domObjects()
+static DOMObjectCacheFrameObserverMap& domObjectCacheFrameObservers()
 {
-    static DOMObjectMap staticDOMObjects;
-    return staticDOMObjects;
+    static NeverDestroyed<DOMObjectCacheFrameObserverMap> map;
+    return map;
 }
 
-static WebCore::Frame* getFrameFromHandle(void* objectHandle)
+static DOMObjectCacheFrameObserver& getOrCreateDOMObjectCacheFrameObserver(WebCore::Frame& frame)
 {
-    WebCore::Node* node = static_cast<WebCore::Node*>(objectHandle);
-    if (!node->inDocument())
-        return 0;
-    return node->document().frame();
+    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, std::move(observer));
+    return *observerPtr;
 }
 
-void DOMObjectCache::forget(void* objectHandle)
-{
-    DOMObjectCacheData* cacheData = domObjects().get(objectHandle);
-    ASSERT(cacheData);
-    g_slice_free(DOMObjectCacheData, cacheData);
-    domObjects().take(objectHandle);
-}
+class DOMObjectCacheFrameObserver final: public WebCore::FrameDestructionObserver {
+public:
+    DOMObjectCacheFrameObserver(WebCore::Frame& frame)
+        : FrameDestructionObserver(&frame)
+    {
+    }
 
-static void weakRefNotify(gpointer data, GObject* zombie)
-{
-    gboolean* objectDead = static_cast<gboolean*>(data);
-    *objectDead = TRUE;
-}
+    ~DOMObjectCacheFrameObserver()
+    {
+        ASSERT(m_objects.isEmpty());
+    }
 
-void DOMObjectCache::clearByFrame(WebCore::Frame* frame)
-{
-    Vector<DOMObjectCacheData*> toUnref;
-
-    // Unreffing the objects removes them from the cache in their
-    // finalize method, so just save them to do that while we are not
-    // iterating the cache itself.
-    DOMObjectMap::iterator end = domObjects().end();
-    for (DOMObjectMap::iterator iter = domObjects().begin(); iter != end; ++iter) {
-        DOMObjectCacheData* data = iter->value;
-        ASSERT(data);
-        if ((!frame || data->frame == frame) && data->timesReturned)
-            toUnref.append(data);
+    void addObjectCacheData(DOMObjectCacheData& data)
+    {
+        ASSERT(!m_objects.contains(&data));
+
+        if (!m_domWindowObserver && m_frame->document()->domWindow())
+            m_domWindowObserver = std::make_unique<DOMWindowObserver>(*m_frame, *this);
+
+        m_objects.append(&data);
+        g_object_weak_ref(data.object, DOMObjectCacheFrameObserver::objectFinalizedCallback, this);
     }
 
-    Vector<DOMObjectCacheData*>::iterator last = toUnref.end();
-    for (Vector<DOMObjectCacheData*>::iterator it = toUnref.begin(); it != last; ++it) {
-        DOMObjectCacheData* data = *it;
-        // We can't really know what the user has done with the DOM
-        // objects, so in case any of the external references to them
-        // were unreffed (but not all, otherwise the object would be
-        // dead and out of the cache) we'll add a weak ref before we
-        // start to get rid of the cache's own references; if the
-        // object dies in the middle of the process, we'll just stop.
-        gboolean objectDead = FALSE;
-        g_object_weak_ref(data->object, weakRefNotify, &objectDead);
-        // We need to check objectDead first, otherwise the cache data
-        // might be garbage already.
-        while (!objectDead && data->timesReturned > 0) {
-            // If this is the last unref we are going to do,
-            // disconnect the weak ref. We cannot do it afterwards
-            // because the object might be dead at that point.
-            if (data->timesReturned == 1) {
-                g_object_weak_unref(data->object, weakRefNotify, &objectDead);
-                // At this point, the next time the DOMObject is
-                // unref'ed it will be finalized,
-                // DOMObject::finalize() will call
-                // DOMObjectCache::forget(), which will free 'data'.
-                // Toggling 'objectDead' here will ensure we don't
-                // dereference an invalid pointer in the next
-                // iteration.
-                objectDead = TRUE;
+private:
+    class DOMWindowObserver final: public WebCore::DOMWindowProperty {
+        WTF_MAKE_FAST_ALLOCATED;
+    public:
+        DOMWindowObserver(WebCore::Frame& frame, DOMObjectCacheFrameObserver& frameObserver)
+            : DOMWindowProperty(&frame)
+            , m_frameObserver(frameObserver)
+        {
+        }
+
+        virtual ~DOMWindowObserver()
+        {
+        }
+
+    private:
+        virtual void willDetachGlobalObjectFromFrame() override
+        {
+            // Clear the DOMWindowProperty first, and then notify the Frame observer.
+            DOMWindowProperty::willDetachGlobalObjectFromFrame();
+            m_frameObserver.willDetachGlobalObjectFromFrame();
+        }
+
+        DOMObjectCacheFrameObserver& m_frameObserver;
+    };
+
+    static void objectFinalizedCallback(gpointer userData, GObject* finalizedObject)
+    {
+        DOMObjectCacheFrameObserver* observer = static_cast<DOMObjectCacheFrameObserver*>(userData);
+        for (size_t i = 0; i < observer->m_objects.size(); ++i) {
+            if (observer->m_objects[i]->object == finalizedObject) {
+                observer->m_objects.remove(i);
+                break;
             }
-            data->timesReturned--;
-            g_object_unref(data->object);
         }
     }
+
+    void clear()
+    {
+        if (m_objects.isEmpty())
+            return;
+
+        auto objects = std::move(m_objects);
+        for (auto* data : objects) {
+            g_object_weak_unref(data->object, DOMObjectCacheFrameObserver::objectFinalizedCallback, this);
+            data->clearObject();
+        }
+    }
+
+    virtual void willDetachPage() override
+    {
+        clear();
+    }
+
+    virtual void frameDestroyed() override
+    {
+        clear();
+        WebCore::Frame* frame = m_frame;
+        FrameDestructionObserver::frameDestroyed();
+        domObjectCacheFrameObservers().remove(frame);
+    }
+
+    void willDetachGlobalObjectFromFrame()
+    {
+        clear();
+        m_domWindowObserver = nullptr;
+    }
+
+    Vector<DOMObjectCacheData*, 8> m_objects;
+    std::unique_ptr<DOMWindowObserver> m_domWindowObserver;
+};
+
+typedef HashMap<void*, std::unique_ptr<DOMObjectCacheData>> DOMObjectMap;
+
+static DOMObjectMap& domObjects()
+{
+    static NeverDestroyed<DOMObjectMap> staticDOMObjects;
+    return staticDOMObjects;
 }
 
-DOMObjectCache::~DOMObjectCache()
+void DOMObjectCache::forget(void* objectHandle)
 {
-    clearByFrame();
+    ASSERT(domObjects().contains(objectHandle));
+    domObjects().remove(objectHandle);
 }
 
 void* DOMObjectCache::get(void* objectHandle)
 {
     DOMObjectCacheData* data = domObjects().get(objectHandle);
-    if (!data)
-        return 0;
-
-    // We want to add one ref each time a wrapper is returned, so that
-    // the user can manually unref them if he chooses to.
-    ASSERT(data->object);
-    data->timesReturned++;
-    return g_object_ref(data->object);
+    return data ? data->refObject() : nullptr;
 }
 
-void* DOMObjectCache::put(void* objectHandle, void* wrapper)
+void DOMObjectCache::put(void* objectHandle, void* wrapper)
 {
-    if (domObjects().get(objectHandle))
-        return wrapper;
-
-    DOMObjectCacheData* data = g_slice_new(DOMObjectCacheData);
-    data->object = static_cast<GObject*>(wrapper);
-    data->frame = 0;
-    data->timesReturned = 1;
-
-    domObjects().set(objectHandle, data);
-    return wrapper;
+    if (domObjects().contains(objectHandle))
+        return;
+    domObjects().set(objectHandle, std::make_unique<DOMObjectCacheData>(G_OBJECT(wrapper)));
 }
 
-void* DOMObjectCache::put(WebCore::Node* objectHandle, void* wrapper)
+void DOMObjectCache::put(WebCore::Node* objectHandle, void* wrapper)
 {
-    // call the ::put version that takes void* to do the basic cache
-    // insertion work
-    put(static_cast<void*>(objectHandle), wrapper);
-
-    DOMObjectCacheData* data = domObjects().get(objectHandle);
-    ASSERT(data);
+    if (domObjects().contains(objectHandle))
+        return;
 
-    data->frame = getFrameFromHandle(objectHandle);
+    std::unique_ptr<DOMObjectCacheData> data = std::make_unique<DOMObjectCacheData>(G_OBJECT(wrapper));
+    auto dataPtr = data.get();
+    domObjects().set(objectHandle, std::move(data));
 
-    return wrapper;
+    if (WebCore::Frame* frame = objectHandle->document().frame())
+        getOrCreateDOMObjectCacheFrameObserver(*frame).addObjectCacheData(*dataPtr);
 }
 
 }

Source/WebCore/bindings/gobject/DOMObjectCache.h

@@ -21,18 +21,15 @@
 
 namespace WebCore {
 class Node;
-class Frame;
 };
 
 namespace WebKit {
 class DOMObjectCache {
 public:
     static void* get(void* objectHandle);
-    static void* put(void* objectHandle, void* wrapper);
-    static void* put(WebCore::Node* objectHandle, void* wrapper);
-    static void clearByFrame(WebCore::Frame* frame = 0);
+    static void put(void* objectHandle, void* wrapper);
+    static void put(WebCore::Node* objectHandle, void* wrapper);
     static void forget(void* objectHandle);
-    ~DOMObjectCache();
 };
 } // namespace WebKit