Bug 112913 – [ATK] [WebKit2] AccessibilityUIElement often leaks its AtkObject

• Component: WebKit2
• Status: RESOLVED FIXED

[Martin Robinson]

AccessibilityUIElements are created often by passing the result of atk_object_ref_accessible_child, which returns a new reference (transfer full). AccessibilityUIElement::~AccessibilityUIElement does not decrement the reference count of the element, so it will leak. Instead AccessibilityUIElement should have a RefPtr to the element and the constructor should be carefully not to leak when assigning the value to the RefPtr.

[Krzysztof Czech]

Created attachment 199409 [details]
Proposed patch

Proposed patch. 

It could be either added a WebKit's smart pointer to PlatformUIElement or some cleanup done in AccessibilityUIElement's desctuctor.
atk/AccessibilityUIElementAtk provides a way to add platform independent destructor, when reference can be decreased.

Waiting for opinions about this solution ?.

[Mario Sanchez Prada]

Comment on attachment 199409 [details]
Proposed patch

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

Looks good to me, I just made a couple of minor suggestions.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:423
> +    if (target)
> +        g_object_ref(target);
>      return target ? AccessibilityUIElement::create(target) : 0;

You can merge that g_object_ref call into the return line:

return target ? AccessibilityUIElement::create(g_object_ref(target)) : 0;

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:434
> +    if (parent)
> +        g_object_ref(parent);
>      return parent ? AccessibilityUIElement::create(parent) : 0;

Same here

[Krzysztof Czech]

Created attachment 199416 [details]
Proposed patch

[Krzysztof Czech]

Applied Mario's suggestions.

[Martin Robinson]

Comment on attachment 199416 [details]
Proposed patch

Thanks for working on this! I prefer using GRefPtr everywhere.

[Build Bot]

Comment on attachment 199416 [details]
Proposed patch

Attachment 199416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/126275

New failing tests:
fast/repaint/japanese-rl-selection-repaint-in-regions.html

[Build Bot]

Created attachment 199502 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Krzysztof Czech]

Created attachment 201937 [details]
Patch

[Krzysztof Czech]

(In reply to comment #5)
> (From update of attachment 199416 [details])
> Thanks for working on this! I prefer using GRefPtr everywhere.
Changed to GRefPtr.

[Mario Sanchez Prada]

Comment on attachment 201937 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:252
> +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> +    GRefPtr<AtkObject> m_element;
> +#else
>      PlatformUIElement m_element;
> +#endif

Would it be possible to update the typedef AtkObject* PlatformUIElement in this file to be typedef GRefPtr<AtkObject> PlatformUIElement, instead of changing this here?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:283
> +    return m_element.get() == otherElement->platformUIElement();

I think you don't need the .get() here

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:288
> +    if (!m_element.get() || !ATK_IS_OBJECT(m_element.get()))

I don't think you need to use get() for checking !m_element.

[Krzysztof Czech]

(In reply to comment #10)
> (From update of attachment 201937 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201937&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:252
> > +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> > +    GRefPtr<AtkObject> m_element;
> > +#else
> >      PlatformUIElement m_element;
> > +#endif
> 
> Would it be possible to update the typedef AtkObject* PlatformUIElement in this file to be typedef GRefPtr<AtkObject> PlatformUIElement, instead of changing this here?

Probably it would be feasible. It requires some additional casting code in AccessibilityController.cpp (rootElement and focusedElement methods)

I'm wondering, PlatformUIElement is used also as a constructor and function's parameter and a return value as well. Would it be too much refs and derefs of temporary objects ?. What to you think about it ?.

> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:283
> > +    return m_element.get() == otherElement->platformUIElement();
> 
> I think you don't need the .get() here
Right
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:288
> > +    if (!m_element.get() || !ATK_IS_OBJECT(m_element.get()))
> 
> I don't think you need to use get() for checking !m_element.
Right

[Krzysztof Czech]

Created attachment 202052 [details]
Patch

[Mario Sanchez Prada]

(In reply to comment #11)
> [...]
> Probably it would be feasible. It requires some additional casting code in 
> AccessibilityController.cpp (rootElement and focusedElement methods)

Yes, I was not expecting it to be a one liner change, that's why I asked. In any case, it's just a suggestion, but if it's too cumbersome and/or nobody else sees a benefit on this, I'd be fine with the current solution.

> I'm wondering, PlatformUIElement is used also as a constructor and function's 
> parameter and a return value as well. Would it be too much refs and derefs of 
> temporary objects ?. What to you think about it ?.

Hrm... I see. Maybe it's going to be too much then, you could just use PassRefPtr if they were not GObjects, but you have GRefPtr here, and that would mean I'm afraid too many changes for what it should be something quite simple in the end.

So, I think I'm leaning more towards keeping it as you have it right now unless, as I said before, anyone else opposes to it.

Thanks for considering my suggestions

[Martin Robinson]

Comment on attachment 202052 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:58
> +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> +    return m_element.get();
> +#else
> +    return m_element;
> +#endif
> +}

I don't believe you need this #ifdef here. Doesn't GRefPtr should be support implicit casts to boolean?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:66
> +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> +    return m_element.get();
> +#else
> +    return m_element;
> +#endif

This is a bit ugly...perhaps Mario is right and GRefPtr<...> should be typedef'd to PlatformUIElement.

[Krzysztof Czech]

Created attachment 202270 [details]
Patch

[Krzysztof Czech]

(In reply to comment #14)
> (From update of attachment 202052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202052&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:58
> > +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> > +    return m_element.get();
> > +#else
> > +    return m_element;
> > +#endif
> > +}
> 
> I don't believe you need this #ifdef here. Doesn't GRefPtr should be support implicit casts to boolean?
You are right.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:66
> > +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY))
> > +    return m_element.get();
> > +#else
> > +    return m_element;
> > +#endif
> 
> This is a bit ugly...perhaps Mario is right and GRefPtr<...> should be typedef'd to PlatformUIElement.

I changed.

[Mario Sanchez Prada]

Comment on attachment 202270 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:55
> +#include <wtf/gobject/GRefPtr.h>
> +typedef GRefPtr<AtkObject> PlatformUIElement;

Beautiful!

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:86
> +    if (!element.get() || !ATK_IS_OBJECT(element.get()))

Just if (!element || ...)

Sorry not to having caught this one before

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:122
> +    if (!element.get() || !ATK_IS_VALUE(element.get()))

Same here

[Krzysztof Czech]

Created attachment 202273 [details]
Patch

[Krzysztof Czech]

> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:86
> > +    if (!element.get() || !ATK_IS_OBJECT(element.get()))
> 
> Just if (!element || ...)
> 
> Sorry not to having caught this one before
Thanks, done.
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:122
> > +    if (!element.get() || !ATK_IS_VALUE(element.get()))
> 
> Same here
Done.

[WebKit Commit Bot]

Comment on attachment 202273 [details]
Patch

Clearing flags on attachment: 202273

Committed r150431: <http://trac.webkit.org/changeset/150431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.