Bug 118297

Summary:

[ATK] Leak: leaks in AccessibilityUIElement

Product:

WebKit

Reporter:

Brian Holt <brian.holt>

Component:

Accessibility

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, eflews.bot, gyuyoung.kim

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

116317

Attachments:

Description	Flags
Patch	none
Revised patch	none
Patch	none

Brian Holt

Reported 2013-07-02 05:29:53 PDT

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 }

Attachments
Patch (2.25 KB, patch) 2013-07-02 06:31 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Revised patch (2.24 KB, patch) 2013-07-02 06:38 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Patch (1.70 KB, patch) 2013-07-02 07:53 PDT, Brian Holt	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Brian Holt

Comment 1 2013-07-02 06:31:18 PDT

Created attachment 205903 [details] Patch Fix for 2 of the 3 leaks

EFL EWS Bot

Comment 2 2013-07-02 06:35:30 PDT

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

Comment 3 2013-07-02 06:36:37 PDT

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

Comment 4 2013-07-02 06:38:31 PDT

Created attachment 205904 [details] Revised patch

Chris Dumez

Comment 5 2013-07-02 06:41:46 PDT

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

Comment 6 2013-07-02 06:46:14 PDT

(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 7 2013-07-02 07:31:29 PDT

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

Comment 8 2013-07-02 07:41:38 PDT

(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 9 2013-07-02 07:43:05 PDT

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

Comment 10 2013-07-02 07:45:26 PDT

(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

Comment 11 2013-07-02 07:46:41 PDT

(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

Comment 12 2013-07-02 07:53:11 PDT

Created attachment 205916 [details] Patch Fix for 1 of the 3 leaks

Chris Dumez

Comment 13 2013-07-02 08:01:47 PDT

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

Comment 14 2013-07-02 08:19:03 PDT

(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 15 2013-07-02 08:26:23 PDT

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 16 2013-07-02 09:51:32 PDT

Comment on attachment 205916 [details] Patch Clearing flags on attachment: 205916 Committed r152298: <http://trac.webkit.org/changeset/152298>

WebKit Commit Bot

Comment 17 2013-07-02 09:51: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.