Bug 118307

Summary:

[ATK] Leak: AtkAttributeSet* should be freed

Product:

WebKit

Reporter:

Brian Holt <brian.holt>

Component:

Accessibility

Assignee:

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:

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

Brian Holt

Reported 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 }

Attachments
Patch (15.17 KB, patch) 2013-07-02 09:56 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Patch (9.15 KB, patch) 2013-07-03 07:41 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Patch (12.46 KB, patch) 2013-07-03 09:24 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Revised patch (12.51 KB, patch) 2013-07-03 10:23 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Patch (12.61 KB, patch) 2013-07-04 04:03 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Brian Holt

Comment 1 2013-07-02 09:56:14 PDT

Created attachment 205924 [details] Patch

EFL EWS Bot

Comment 2 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

Brian Holt

Comment 3 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.

Chris Dumez

Comment 4 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.

Mario Sanchez Prada

Comment 5 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.

Mario Sanchez Prada

Comment 6 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...

Brian Holt

Comment 7 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?

Chris Dumez

Comment 8 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.

Chris Dumez

Comment 9 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)?

Brian Holt

Comment 10 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.

Brian Holt

Comment 11 2013-07-03 07:41:25 PDT

Created attachment 206002 [details] Patch

Chris Dumez

Comment 12 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*

Brian Holt

Comment 13 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.

Chris Dumez

Comment 14 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.

Brian Holt

Comment 15 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.

Brian Holt

Comment 16 2013-07-03 09:24:59 PDT

Created attachment 206006 [details] Patch

Chris Dumez

Comment 17 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.

Brian Holt

Comment 18 2013-07-03 10:23:31 PDT

Created attachment 206009 [details] Revised patch As per Christophe's comments

Chris Dumez

Comment 19 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

Brian Holt

Comment 20 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);

Chris Dumez

Comment 21 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.

Brian Holt

Comment 22 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.

Brian Holt

Comment 23 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.

Brian Holt

Comment 24 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?

Chris Dumez

Comment 25 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.

Brian Holt

Comment 26 2013-07-04 04:03:11 PDT

Created attachment 206070 [details] Patch

Chris Dumez

Comment 27 2013-07-04 04:15:19 PDT

Comment on attachment 206070 [details] Patch Ok, r=me.

Brian Holt

Comment 28 2013-07-04 04:39:26 PDT

Thanks Christophe!

WebKit Commit Bot

Comment 29 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>

WebKit Commit Bot

Comment 30 2013-07-04 05:11:35 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.