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: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 116317 | ||||||||||||||
Attachments: |
|
Description
Brian Holt
2013-07-02 08:32:29 PDT
Created attachment 205924 [details]
Patch
Comment on attachment 205924 [details] Patch Attachment 205924 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1017335 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. 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. (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. (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... (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? (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 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)? 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. Created attachment 206002 [details]
Patch
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 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 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 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. Created attachment 206006 [details]
Patch
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. Created attachment 206009 [details]
Revised patch
As per Christophe's comments
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 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 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 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. (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 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? (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. Created attachment 206070 [details]
Patch
Comment on attachment 206070 [details]
Patch
Ok, r=me.
Thanks Christophe! Comment on attachment 206070 [details] Patch Clearing flags on attachment: 206070 Committed r152397: <http://trac.webkit.org/changeset/152397> All reviewed patches have been landed. Closing bug. |