WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
118297
[ATK] Leak: leaks in AccessibilityUIElement
https://bugs.webkit.org/show_bug.cgi?id=118297
Summary
[ATK] Leak: leaks in AccessibilityUIElement
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.
Top of Page
Format For Printing
XML
Clone This Bug