Bug 118297 – [ATK] Leak: leaks in AccessibilityUIElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Brian Holt]

In Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:

Leak found using the "--leak" option in the Gtk port:

{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:malloc
   fun:g_malloc
   fun:g_strdup
   fun:_ZN22AccessibilityUIElement11stringValueEv
   fun:_ZL22getStringValueCallbackPK15OpaqueJSContextP13OpaqueJSValueP14OpaqueJSStringPPKS2_
   fun:_ZN3JSC16JSCallbackObjectINS_20JSDestructibleObjectEE14getStaticValueEPNS_9ExecStateENS_12PropertyNameE
   fun:_ZN3JSC16JSCallbackObjectINS_20JSDestructibleObjectEE18getOwnPropertySlotEPNS_6JSCellEPNS_9ExecStateENS_12PropertyNameERNS_12PropertySlotE
   fun:llint_slow_path_get_by_id
   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
   fun:_ZN7WebCore16HTMLScriptRunner7executeEN3WTF10PassRefPtrINS_7ElementEEERKNS1_12TextPositionE
   fun:_ZN7WebCore18HTMLDocumentParser30runScriptsForPausedTreeBuilderEv
   fun:_ZN7WebCore18HTMLDocumentParser16canTakeNextTokenENS0_15SynchronousModeERNS_11PumpSessionE
   fun:_ZN7WebCore18HTMLDocumentParser13pumpTokenizerENS0_15SynchronousModeE
   fun:_ZN7WebCore18HTMLDocumentParser33resumeParsingAfterScriptExecutionEv
   fun:_ZN7WebCore18HTMLDocumentParser14notifyFinishedEPNS_14CachedResourceE
}

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

{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:malloc
   fun:g_malloc
   fun:g_strdup
   fun:_ZL27webkitAccessibleTextGetTextP8_AtkTextii
   fun:_ZL23webkitAccessibleGetNameP10_AtkObject
   fun:_ZN22AccessibilityUIElement5titleEv
   fun:_ZL16getTitleCallbackPK15OpaqueJSContextP13OpaqueJSValueP14OpaqueJSStringPPKS2_
   fun:_ZN3JSC16JSCallbackObjectINS_20JSDestructibleObjectEE14getStaticValueEPNS_9ExecStateENS_12PropertyNameE
   fun:_ZN3JSC16JSCallbackObjectINS_20JSDestructibleObjectEE18getOwnPropertySlotEPNS_6JSCellEPNS_9ExecStateENS_12PropertyNameERNS_12PropertySlotE
   fun:llint_slow_path_get_by_id
   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:_ZN7WebCore16ScriptController20executeScriptInWorldEPNS_15DOMWrapperWorldERKN3WTF6StringEb
   fun:_ZN7WebCore15ScheduledAction7executeEPNS_8DocumentE
   fun:_ZN7WebCore8DOMTimer5firedEv
   fun:_ZN7WebCore12ThreadTimers24sharedTimerFiredInternalEv
}

[Brian Holt]

Created attachment 205903 [details]
Patch

Fix for 2 of the 3 leaks

[EFL EWS Bot]

Comment on attachment 205903 [details]
Patch

Attachment 205903 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1019110

[Brian Holt]

The proposed patch addresses the first two leaks.

The final leak 

   Memcheck:Leak
   fun:malloc
   fun:g_malloc
   fun:g_strdup
   fun:_ZL27webkitAccessibleTextGetTextP8_AtkTextii
   fun:_ZL23webkitAccessibleGetNameP10_AtkObject
   fun:_ZN22AccessibilityUIElement5titleEv

will not be addressed in this bug because it requires a wider change to the Atk object caching mechanism that is used by the implementation of the Atk interface to return const pointers (e.g. const gchar*).  A new bug will be raised.

[Brian Holt]

Created attachment 205904 [details]
Revised patch

[Chris Dumez]

Comment on attachment 205904 [details]
Revised patch

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

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:316
> +    GOwnPtr<char> attributeData(attributeString.get());

This does not look right.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:424
> +    GOwnPtr<gchar> text(atk_text_get_text(ATK_TEXT(m_element), 0, -1));

This part looks OK though.

[Brian Holt]

(In reply to comment #5)
> (From update of attachment 205904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205904&action=review
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:316
> > +    GOwnPtr<char> attributeData(attributeString.get());
> 
> This does not look right.

Absolutely correct, don't know how that slipped past.

> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:424
> > +    GOwnPtr<gchar> text(atk_text_get_text(ATK_TEXT(m_element), 0, -1));
> 
> This part looks OK though.

[Chris Dumez]

Comment on attachment 205904 [details]
Revised patch

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

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:315
> +    GOwnPtr<char> attributeString(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));

There is indeed a leak here though. According to the doc:
https://developer.gnome.org/atk/unstable/AtkObject.html#atk-object-get-attributes

"This atkattributeset should be freed by a call to atk_attribute_set_free()."

[Brian Holt]

(In reply to comment #7)
> (From update of attachment 205904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205904&action=review
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:315
> > +    GOwnPtr<char> attributeString(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));
> 
> There is indeed a leak here though. According to the doc:
> https://developer.gnome.org/atk/unstable/AtkObject.html#atk-object-get-attributes
> 
> "This atkattributeset should be freed by a call to atk_attribute_set_free()."

Yes, I've just been discussing with Mario how to resolve this.  The best fix requires a specialisation of GOwnPtr so I'll propose that in a separate bug (coming shortly).

[Chris Dumez]

Comment on attachment 205904 [details]
Revised patch

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

>> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:315
>> +    GOwnPtr<char> attributeString(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));
> 
> There is indeed a leak here though. According to the doc:
> https://developer.gnome.org/atk/unstable/AtkObject.html#atk-object-get-attributes
> 
> "This atkattributeset should be freed by a call to atk_attribute_set_free()."

And BTW, same issue in Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp. Maybe you can fix the leaks in WKTR as well in the same patch?

[Brian Holt]

(In reply to comment #9)
> (From update of attachment 205904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205904&action=review
> 
> >> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:315
> >> +    GOwnPtr<char> attributeString(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));
> > 
> > There is indeed a leak here though. According to the doc:
> > https://developer.gnome.org/atk/unstable/AtkObject.html#atk-object-get-attributes
> > 
> > "This atkattributeset should be freed by a call to atk_attribute_set_free()."
> 
> And BTW, same issue in Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp. Maybe you can fix the leaks in WKTR as well in the same patch?

Absolutely :-)

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 205904 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=205904&action=review
> > 
> > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:315
> > > +    GOwnPtr<char> attributeString(attributeSetToString(atk_object_get_attributes(ATK_OBJECT(m_element))));
> > 
> > There is indeed a leak here though. According to the doc:
> > https://developer.gnome.org/atk/unstable/AtkObject.html#atk-object-get-attributes
> > 
> > "This atkattributeset should be freed by a call to atk_attribute_set_free()."
> 
> Yes, I've just been discussing with Mario how to resolve this.  The best fix requires a specialisation of GOwnPtr so I'll propose that in a separate bug (coming shortly).

I agree, probably in a new Source/WebCore/accessibility/atk/GOwnPtrAtk.h ?

See for example:
Source/WebCore/platform/network/soup/GOwnPtrSoup.*

[Brian Holt]

Created attachment 205916 [details]
Patch

Fix for 1 of the 3 leaks

[Chris Dumez]

Comment on attachment 205916 [details]
Patch

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

> Tools/ChangeLog:8
> +        Reviewed by Christophe Dumez.

The patch did not get r+. So It is a bit early to update this line :)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:425
> +    GOwnPtr<gchar> axValue(g_strdup_printf("AXValue: %s", textWithReplacedCharacters.get()));

Would you fix the same issue in Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp in the same patch please?

[Brian Holt]

(In reply to comment #13)
> (From update of attachment 205916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205916&action=review
> 
> > Tools/ChangeLog:8
> > +        Reviewed by Christophe Dumez.
> 
> The patch did not get r+. So It is a bit early to update this line :)

Sorry, I didn't realise that we only do that after r+. Because I'm not a committer I thought I could make the change to avoid having to upload another patch after r+ ...

> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:425
> > +    GOwnPtr<gchar> axValue(g_strdup_printf("AXValue: %s", textWithReplacedCharacters.get()));
> 
> Would you fix the same issue in Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp in the same patch please?

It turns out that this has already been fixed in WKTR, but it wasn't applied to the DRT.

[Chris Dumez]

Comment on attachment 205916 [details]
Patch

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

>>> Tools/ChangeLog:8
>>> +        Reviewed by Christophe Dumez.
>> 
>> The patch did not get r+. So It is a bit early to update this line :)
> 
> Sorry, I didn't realise that we only do that after r+. Because I'm not a committer I thought I could make the change to avoid having to upload another patch after r+ ...

You don't have to reupload a patch after r+ unless there are nits to fix. This line gets automatically updated when landing after a r+.

>>> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:425
>>> +    GOwnPtr<gchar> axValue(g_strdup_printf("AXValue: %s", textWithReplacedCharacters.get()));
>> 
>> Would you fix the same issue in Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp in the same patch please?
> 
> It turns out that this has already been fixed in WKTR, but it wasn't applied to the DRT.

Oh, my bad. I misread my grep output.

[WebKit Commit Bot]

Comment on attachment 205916 [details]
Patch

Clearing flags on attachment: 205916

Committed r152298: <http://trac.webkit.org/changeset/152298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.