| 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: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 116317 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 205903 [details]
Patch
Fix for 2 of the 3 leaks
Comment on attachment 205903 [details] Patch Attachment 205903 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1019110 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. Created attachment 205904 [details]
Revised patch
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. (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. 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()." (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). 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? (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 :-) (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.* Created attachment 205916 [details]
Patch
Fix for 1 of the 3 leaks
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? (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. 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. Comment on attachment 205916 [details] Patch Clearing flags on attachment: 205916 Committed r152298: <http://trac.webkit.org/changeset/152298> All reviewed patches have been landed. Closing bug. |
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 }