Bug 118307

Summary: [ATK] Leak: AtkAttributeSet* should be freed
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cdumez, cfleizach, commit-queue, dmazzoni, eflews.bot, gyuyoung.kim, jdiggs, mario, mikhail.pozdnyakov, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 116317    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Revised patch
none
Patch none

Description Brian Holt 2013-07-02 08:32:29 PDT
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp and Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp 
both allocate (and don't free) memory from the call to atk_object_get_attributes(ATK_OBJECT(m_element)).

{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:malloc
   fun:g_malloc
   fun:g_slice_alloc
   fun:g_slist_prepend
   fun:_ZL29webkitAccessibleGetAttributesP10_AtkObject
   fun:_ZN22AccessibilityUIElement13allAttributesEv
   fun:_ZL21allAttributesCallbackPK15OpaqueJSContextP13OpaqueJSValueS3_mPKPKS2_PS5_
   fun:_ZN3JSC18JSCallbackFunction4callEPNS_9ExecStateE
   fun:_ZN3JSC5LLIntL14handleHostCallEPNS_9ExecStateEPNS_11InstructionENS_7JSValueENS_22CodeSpecializationKindE
   fun:llint_slow_path_call
   obj:/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libjavascriptcoregtk-3.0.so.0.14.1
   fun:_ZN3JSC11Interpreter7executeEPNS_14EvalExecutableEPNS_9ExecStateENS_7JSValueEPNS_7JSScopeE
   fun:_ZN3JSC4evalEPNS_9ExecStateE
   fun:llint_slow_path_call_eval
   obj:/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libjavascriptcoregtk-3.0.so.0.14.1
   fun:_ZN3JSC11Interpreter7executeEPNS_17ProgramExecutableEPNS_9ExecStateEPNS_8JSObjectE
   fun:_ZN3JSC8evaluateEPNS_9ExecStateERKNS_10SourceCodeENS_7JSValueEPS5_
   fun:_ZN7WebCore16ScriptController15evaluateInWorldERKNS_16ScriptSourceCodeEPNS_15DOMWrapperWorldE
   fun:_ZN7WebCore16ScriptController8evaluateERKNS_16ScriptSourceCodeE
   fun:_ZN7WebCore13ScriptElement13executeScriptERKNS_16ScriptSourceCodeE
   fun:_ZN7WebCore13ScriptElement13prepareScriptERKN3WTF12TextPositionENS0_17LegacyTypeSupportE
   fun:_ZN7WebCore16HTMLScriptRunner9runScriptEPNS_7ElementERKN3WTF12TextPositionE
}
Comment 1 Brian Holt 2013-07-02 09:56:14 PDT
Created attachment 205924 [details]
Patch
Comment 2 EFL EWS Bot 2013-07-02 10:02:09 PDT
Comment on attachment 205924 [details]
Patch

Attachment 205924 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1017335
Comment 3 Brian Holt 2013-07-02 10:08:19 PDT
The EFL failure is because AtkAttributeSet* is typedef'ed to_GSList*. Not sure why the EFL build fails but the GTK one is ok here.

Anyway, perhaps this patch doesn't actaully need a GOwnPtrAtk if it can just use the GSList* specialisation.
Comment 4 Chris Dumez 2013-07-02 10:18:23 PDT
I think the EFL bot is right. we cannot distinguish GSList and AtkAttributeSet if one is a typedef to the other :(

Using the existing GSList specialization would not be great because it would only free the list but not its items.

Maybe we should stick with atk_attribute_set_free() then? unless someone has a better proposal.
Comment 5 Mario Sanchez Prada 2013-07-03 01:40:42 PDT
(In reply to comment #4)
> I think the EFL bot is right. we cannot distinguish GSList and AtkAttributeSet
> if one is a typedef to the other :(

I wonder why this is a problem while building EFL and not while building GTK. Any idea?

Another "option" is to propose to ATK developers, making the most of the fact that they are defining/changing the API now for the incoming versions, to get rid of that typedef and just use a GSList. That would take more time of course and could even make sense in the long run, yet would not fix the issue at hand here (which might happen in the future in a similar scenario).

> Using the existing GSList specialization would not be great because it would 
> only free the list but not its items.

Indeed. Using the GSList specialization would leak not only every single data field in the GSList, but also two allocated gchar* for each of them too, since AtkAttribute is a struct.

> Maybe we should stick with atk_attribute_set_free() then?
> unless someone has a better proposal.

I would propose to investigate why this is a problem in EFL and not in GTK, and try to get it sorted out, because using a GOwnPtr specialization is, in my opinion, clearer and cleaner than the current approach we have now.
Comment 6 Mario Sanchez Prada 2013-07-03 02:18:27 PDT
(In reply to comment #5)
> [...]
> > Maybe we should stick with atk_attribute_set_free() then?
> > unless someone has a better proposal.
> 
> I would propose to investigate why this is a problem in EFL and not in GTK,
> and try to get it sorted out, because using a GOwnPtr specialization is, in
> my opinion, clearer and cleaner than the current approach we have now.

Brian just explained to me that he jsut updated PlatformGtk.cmake but not GNUmakefile.list.am, which I believe is what the EWS is using now.

I guess that explains why it was not failing in the GTK bot, which is probably using the GSList specialization after all...
Comment 7 Brian Holt 2013-07-03 02:48:31 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > [...]
> > > Maybe we should stick with atk_attribute_set_free() then?
> > > unless someone has a better proposal.
> > 
> > I would propose to investigate why this is a problem in EFL and not in GTK,
> > and try to get it sorted out, because using a GOwnPtr specialization is, in
> > my opinion, clearer and cleaner than the current approach we have now.
> 
> Brian just explained to me that he jsut updated PlatformGtk.cmake but not GNUmakefile.list.am, which I believe is what the EWS is using now.
> 
> I guess that explains why it was not failing in the GTK bot, which is probably using the GSList specialization after all...

Not quite - it doesn't fail when using the autotools make system on Gtk.  Perhaps the EFL cmake build system is trying to link the 2 object files into the same shared object and Gtk is putting them in their own .so files?
Comment 8 Chris Dumez 2013-07-03 02:52:59 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > [...]
> > > > Maybe we should stick with atk_attribute_set_free() then?
> > > > unless someone has a better proposal.
> > > 
> > > I would propose to investigate why this is a problem in EFL and not in GTK,
> > > and try to get it sorted out, because using a GOwnPtr specialization is, in
> > > my opinion, clearer and cleaner than the current approach we have now.
> > 
> > Brian just explained to me that he jsut updated PlatformGtk.cmake but not GNUmakefile.list.am, which I believe is what the EWS is using now.
> > 
> > I guess that explains why it was not failing in the GTK bot, which is probably using the GSList specialization after all...
> 
> Not quite - it doesn't fail when using the autotools make system on Gtk.  Perhaps the EFL cmake build system is trying to link the 2 object files into the same shared object and Gtk is putting them in their own .so files?

I don't think it matters. Since a AtkAttributeSet is the same thing as a GSList, how can you be sure the right GOwnPtr would be used? There is no way the differentiate the 2 types since they are the same type. You cannot do template specialization here. Even if it compiles on GTK, the behavior would be undetermined.
Comment 9 Chris Dumez 2013-07-03 02:53:18 PDT
Comment on attachment 205924 [details]
Patch

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

> Source/WebCore/accessibility/atk/GOwnPtrAtk.cpp:2
> + *  Copyright (C) 2013 Samsung Electronics Inc.

We usually use "Samsung Electronics. All rights reserved."

> Source/WebCore/accessibility/atk/GOwnPtrAtk.cpp:27
> +

extra line

> Source/WebCore/accessibility/atk/GOwnPtrAtk.h:2
> + *  Copyright (C) 2013 Samsung Electronics Inc.

copyright

> Source/WebCore/accessibility/atk/GOwnPtrAtk.h:22
> +

No #if HAVE(ACCESSIBILITY)?
Comment 10 Brian Holt 2013-07-03 06:45:15 PDT
Unfortunately there is no way around the typdef (http://stackoverflow.com/questions/4832900/how-can-i-specialize-a-typedef-and-its-implicit-type-differently).

I'm going to scrap the GOwnPtr idea and just simply call atk_attribute_set_free(ptr) to free the memory.
Comment 11 Brian Holt 2013-07-03 07:41:25 PDT
Created attachment 206002 [details]
Patch
Comment 12 Chris Dumez 2013-07-03 07:57:23 PDT
Comment on attachment 206002 [details]
Patch

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

> Tools/DumpRenderTree/atk/AccessibilityControllerAtk.cpp:97
> +    AtkAttributeSet* attributeSet(atk_object_get_attributes(parent));

I think the assignment looked better before.

> Tools/DumpRenderTree/atk/AccessibilityControllerAtk.cpp:98
>      for (GSList* attributes = attributeSet; attributes; attributes = attributes->next) {

Would be nice to use AtkAttributeSet here instead of GSList.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:808
> +    for (GSList* attributes = attributeSet; attributes; attributes = attributes->next) {

Would be nice to use AtkAttributeSet here instead of GSList.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:811
> +            attributeValue.set(g_strdup(atkAttribute->value));

If we call JSStringCreateWithUTF8CString() before freeing the attributes list, we would not need this strdup() and attributeValue could be a const gchar*.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:160
>      for (GSList* attributes = attributeSet; attributes; attributes = attributes->next) {

Would be nice to use AtkAttributeSet here instead of GSList.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:59
> +    for (GSList* attributes = attributeSet; attributes; attributes = attributes->next) {

Would be nice to use AtkAttributeSet here instead of GSList.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:-78
> -    attributesClear(atkAttributes.get());

Missing call to atk_attribute_set_free()?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:477
> +    for (GSList* attributes = attributeSet; attributes; attributes = attributes->next) {

AtkAttributeSet*
Comment 13 Brian Holt 2013-07-03 08:09:26 PDT
Comment on attachment 206002 [details]
Patch

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

Great comments, thanks very much!

>> Tools/DumpRenderTree/atk/AccessibilityControllerAtk.cpp:97
>> +    AtkAttributeSet* attributeSet(atk_object_get_attributes(parent));
> 
> I think the assignment looked better before.

My mistake, will change that.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:-78
>> -    attributesClear(atkAttributes.get());
> 
> Missing call to atk_attribute_set_free()?

No, the attributeSet will get freed in AccessibilityUIElement::allAttributes() where it is created.
Comment 14 Chris Dumez 2013-07-03 08:23:27 PDT
Comment on attachment 206002 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
>  static gchar* attributeSetToString(AtkAttributeSet* attributeSet)

Since this function is always called as:
"GOwnPtr<gchar> attributeData(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));"

How about changing the prototype to:
static gchar* getAtkObjectAttributesAsString(AtkObject *accessible); ?

This way, we handle freeing the list inside the function instead of the caller.
Comment 15 Brian Holt 2013-07-03 08:31:13 PDT
Comment on attachment 206002 [details]
Patch

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

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
>>  static gchar* attributeSetToString(AtkAttributeSet* attributeSet)
> 
> Since this function is always called as:
> "GOwnPtr<gchar> attributeData(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));"
> 
> How about changing the prototype to:
> static gchar* getAtkObjectAttributesAsString(AtkObject *accessible); ?
> 
> This way, we handle freeing the list inside the function instead of the caller.

That seems very reasonable to me, I'll make the change.
Comment 16 Brian Holt 2013-07-03 09:24:59 PDT
Created attachment 206006 [details]
Patch
Comment 17 Chris Dumez 2013-07-03 10:12:39 PDT
Comment on attachment 206006 [details]
Patch

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

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:806
> +    const char* attributeValue = "";

I would initialize to 0 instead

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:815
> +    JSStringRef jsStr = JSStringCreateWithUTF8CString(attributeValue);

no abbreviations as per coding style. Please use "jsString".
Also, mac implementation seems to return 0 if not found. So maybe something like:
JSStringRef jsString = attributeValue ? SStringCreateWithUTF8CString(attributeValue) : 0;

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:476
> +    const char* attributeValue = "";

Same comment as for DRT.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:485
> +    JSStringRef jsStr = JSStringCreateWithUTF8CString(attributeValue);

Ditto.
Comment 18 Brian Holt 2013-07-03 10:23:31 PDT
Created attachment 206009 [details]
Revised patch

As per Christophe's comments
Comment 19 Chris Dumez 2013-07-03 10:32:01 PDT
Comment on attachment 206009 [details]
Revised patch

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

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:295
> +static char* getAtkAttributeSetAsString(AtkObject* accessible)

gchar

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:317
> +    GOwnPtr<char> attributeData(getAtkAttributeSetAsString(ATK_OBJECT(m_element)));

gchar

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:804
>          return JSStringCreateWithCharacters(0, 0);

It looks a bit weird that we return 0 below but not here.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:806
> +    const char* attributeValue = 0;

gchar

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
> +static char* getAtkAttributeSetAsString(AtkObject* accessible)

gchar

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:463
> +    GOwnPtr<char> attributeData(getAtkAttributeSetAsString(ATK_OBJECT(m_element.get())));

gchar

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:474
>          return JSStringCreateWithCharacters(0, 0);

It looks a bit weird that we return 0 below but not here.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:476
> +    const char* attributeValue = 0;

gchar
Comment 20 Brian Holt 2013-07-03 10:48:13 PDT
Comment on attachment 206009 [details]
Revised patch

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

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
>> +static char* getAtkAttributeSetAsString(AtkObject* accessible)
> 
> gchar

Regarding the gchars, I was under the impression that for new code that is not exposed to the outside world we should be using the non-g-versions.  If that's not correct then I will happily change to gchars throughout.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:474
>>          return JSStringCreateWithCharacters(0, 0);
> 
> It looks a bit weird that we return 0 below but not here.

That's a good point.  Shall I do
JSStringRef jsString = attributeValue ? JSStringCreateWithUTF8CString(attributeValue)
                                      : JSStringCreateWithCharacters(0, 0);
Comment 21 Chris Dumez 2013-07-03 10:52:22 PDT
Comment on attachment 206009 [details]
Revised patch

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

>>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
>>> +static char* getAtkAttributeSetAsString(AtkObject* accessible)
>> 
>> gchar
> 
> Regarding the gchars, I was under the impression that for new code that is not exposed to the outside world we should be using the non-g-versions.  If that's not correct then I will happily change to gchars throughout.

Hmm, I'm not aware of that. Last I heard, gchar was preferred but the situation might have changed since then. Do you have a link to a review / comment supporting what you're saying?

>>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:474
>>>          return JSStringCreateWithCharacters(0, 0);
>> 
>> It looks a bit weird that we return 0 below but not here.
> 
> That's a good point.  Shall I do
> JSStringRef jsString = attributeValue ? JSStringCreateWithUTF8CString(attributeValue)
>                                       : JSStringCreateWithCharacters(0, 0);

Just "return 0;" here for consistency.
Comment 22 Brian Holt 2013-07-03 11:09:25 PDT
Comment on attachment 206009 [details]
Revised patch

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

>>>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
>>>> +static char* getAtkAttributeSetAsString(AtkObject* accessible)
>>> 
>>> gchar
>> 
>> Regarding the gchars, I was under the impression that for new code that is not exposed to the outside world we should be using the non-g-versions.  If that's not correct then I will happily change to gchars throughout.
> 
> Hmm, I'm not aware of that. Last I heard, gchar was preferred but the situation might have changed since then. Do you have a link to a review / comment supporting what you're saying?

Just hearsay at the moment, I'll see if I can fish out a link tomorrow.
Comment 23 Brian Holt 2013-07-04 02:52:18 PDT
(In reply to comment #22)
> (From update of attachment 206009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206009&action=review
> 
> >>>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
> >>>> +static char* getAtkAttributeSetAsString(AtkObject* accessible)
> >>> 
> >>> gchar
> >> 
> >> Regarding the gchars, I was under the impression that for new code that is not exposed to the outside world we should be using the non-g-versions.  If that's not correct then I will happily change to gchars throughout.
> > 
> > Hmm, I'm not aware of that. Last I heard, gchar was preferred but the situation might have changed since then. Do you have a link to a review / comment supporting what you're saying?
> 
> Just hearsay at the moment, I'll see if I can fish out a link tomorrow.

The fourth comment from Martin in this link mentions it https://bugs.webkit.org/show_bug.cgi?id=114871#c9, but the reason is not made clear.
Comment 24 Brian Holt 2013-07-04 03:07:10 PDT
Comment on attachment 206009 [details]
Revised patch

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

>>>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:474
>>>>          return JSStringCreateWithCharacters(0, 0);
>>> 
>>> It looks a bit weird that we return 0 below but not here.
>> 
>> That's a good point.  Shall I do
>> JSStringRef jsString = attributeValue ? JSStringCreateWithUTF8CString(attributeValue)
>>                                       : JSStringCreateWithCharacters(0, 0);
> 
> Just "return 0;" here for consistency.

I've just been looking through the file and JSStringCreateWithCharacters(0, 0) is definitely the common way to return an empty JSStringRef. Should I not do that instead for consistency?
Comment 25 Chris Dumez 2013-07-04 03:32:20 PDT
(In reply to comment #24)
> (From update of attachment 206009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206009&action=review
> 
> >>>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:474
> >>>>          return JSStringCreateWithCharacters(0, 0);
> >>> 
> >>> It looks a bit weird that we return 0 below but not here.
> >> 
> >> That's a good point.  Shall I do
> >> JSStringRef jsString = attributeValue ? JSStringCreateWithUTF8CString(attributeValue)
> >>                                       : JSStringCreateWithCharacters(0, 0);
> > 
> > Just "return 0;" here for consistency.
> 
> I've just been looking through the file and JSStringCreateWithCharacters(0, 0) is definitely the common way to return an empty JSStringRef. Should I not do that instead for consistency?

But the mac implementation seems to return 0 is those cases, right? Anyway, this is not strictly related to your fix so you can keep using JSStringCreateWithCharacters(0, 0) in this patch.
Comment 26 Brian Holt 2013-07-04 04:03:11 PDT
Created attachment 206070 [details]
Patch
Comment 27 Chris Dumez 2013-07-04 04:15:19 PDT
Comment on attachment 206070 [details]
Patch

Ok, r=me.
Comment 28 Brian Holt 2013-07-04 04:39:26 PDT
Thanks Christophe!
Comment 29 WebKit Commit Bot 2013-07-04 05:11:30 PDT
Comment on attachment 206070 [details]
Patch

Clearing flags on attachment: 206070

Committed r152397: <http://trac.webkit.org/changeset/152397>
Comment 30 WebKit Commit Bot 2013-07-04 05:11:35 PDT
All reviewed patches have been landed.  Closing bug.