[Webkit-unassigned] [Bug 40302] [GTK] Memory managament for DOM GObject wrappers

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74295|review?                     |review-
               Flag|                            |




--- Comment #7 from Martin Robinson <mrobinson at webkit.org>  2010-11-18 16:46:12 PST ---
(From update of attachment 74295)
View in context: https://bugs.webkit.org/attachment.cgi?id=74295&action=review

Looks good, though I have a few nagging concerns (some of which I've outlined below). Another issue is what happens when a developer uses a DOM object to change the frame location. Isn't there the possibility that it will unref the DOM wrapper out from under them?

> WebCore/bindings/gobject/DOMObjectCache.cpp:72
> +    // Unrefing the objects removes them from the cache in their

Should be "Unreffing" here.

> WebCore/bindings/gobject/DOMObjectCache.cpp:77
> +        DOMObjectCacheData* data = static_cast<DOMObjectCacheData*>(iter->second);

I don't think this static_cast is necessary.

> WebCore/bindings/gobject/DOMObjectCache.cpp:78
> +        if (data && (!view || data->view == view))

Would ASSERT(data) be more appropriate, or are there cases where the data can, in fact, be NULL?

> WebCore/bindings/gobject/DOMObjectCache.cpp:101
> +    return g_object_ref(data->object);

Won't this lead to leaks, since the cache only does one unref when cleared and DOMObjectCache::get can be called multiple times before that?

> WebCore/bindings/gobject/DOMObjectCache.cpp:119
> +    put(static_cast<void*>(objectHandle), wrapper);

I don't think the cast is necessary to get void*.

> WebCore/bindings/gobject/DOMObjectCache.h:22
> +#include <glib-object.h>

Using void* instead of gpointer would prevent the need for this include, which would be a win for compile time.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:703
> -    // this is only interesting once we provide an external API for the DOM
> +    if (!ready)
> +        DOMObjectCache::clearByView(getViewFromFrame(m_frame));

Doesn't this mean that when an iframe reloads, all cached DOM objects will be removed for the entire view? I think it would be better to track per-frame.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.