Bug 118505

Summary: Leak: TextCodecICU::registerCodecs is leaking
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: TextAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, commit-queue, darin, ews-watchlist, glenn, joepeck, menard, mrobinson, psolanki, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=154737
https://bugs.webkit.org/show_bug.cgi?id=160931
Bug Depends on:    
Bug Blocks: 116317, 106716    
Attachments:
Description Flags
Patch
ap: review-
stop silencing the leak none

Brian Holt
Reported 2013-07-09 07:03:52 PDT
In Source/WebCore/platform/text/TextCodecICU.cpp:215 Leaks found using the "--leak" option in the Gtk port: Command: /home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Debug/Programs/DumpRenderTree - Leak_DefinitelyLost 70 bytes in 7 blocks are definitely lost in loss record 10,235 of 17,061 malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) WTF::fastMalloc(unsigned long) (/WebKitBuild/Debug/../../Source/WTF/wtf/FastMalloc.cpp:286) WTF::fastStrDup(char const*) (/WebKitBuild/Debug/../../Source/WTF/wtf/FastMalloc.cpp:219) WebCore::TextCodecICU::registerCodecs(void (*)(char const*, WTF::PassOwnPtr<WebCore::TextCodec> (*)(WebCore::TextEncoding const&, void const*), void const*)) (/WebKitBuild/Debug/../../Source/WebCore/platform/text/TextCodecICU.cpp:215) WebCore::extendTextCodecMaps() (/WebKitBuild/Debug/../../Source/WebCore/platform/text/TextEncodingRegistry.cpp:293) WebCore::atomicCanonicalTextEncodingName(char const*) (/WebKitBuild/Debug/../../Source/WebCore/platform/text/TextEncodingRegistry.cpp:338) char const* WebCore::atomicCanonicalTextEncodingName<unsigned char>(unsigned char const*, unsigned long) (/WebKitBuild/Debug/../../Source/WebCore/platform/text/TextEncodingRegistry.cpp:355) WebCore::atomicCanonicalTextEncodingName(WTF::String const&) (/WebKitBuild/Debug/../../Source/WebCore/platform/text/TextEncodingRegistry.cpp:364) WebCore::TextEncoding::TextEncoding(WTF::String const&) (/WebKitBuild/Debug/../../Source/WebCore/platform/text/TextEncoding.cpp:57) WebCore::XMLHttpRequest::didReceiveData(char const*, int) (/WebKitBuild/Debug/../../Source/WebCore/xml/XMLHttpRequest.cpp:1198) WebCore::DocumentThreadableLoader::didReceiveData(unsigned long, char const*, int) (/WebKitBuild/Debug/../../Source/WebCore/loader/DocumentThreadableLoader.cpp:299) WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest const&, WebCore::SecurityCheckPolicy) (/WebKitBuild/Debug/../../Source/WebCore/loader/DocumentThreadableLoader.cpp:426) WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&) (/WebKitBuild/Debug/../../Source/WebCore/loader/DocumentThreadableLoader.cpp:87) WebCore::DocumentThreadableLoader::loadResourceSynchronously(WebCore::Document*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderClient&, WebCore::ThreadableLoaderOptions const&) (/WebKitBuild/Debug/../../Source/WebCore/loader/DocumentThreadableLoader.cpp:61) WebCore::ThreadableLoader::loadResourceSynchronously(WebCore::ScriptExecutionContext*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderClient&, WebCore::ThreadableLoaderOptions const&) (/WebKitBuild/Debug/../../Source/WebCore/loader/ThreadableLoader.cpp:78) WebCore::XMLHttpRequest::createRequest(int&) (/WebKitBuild/Debug/../../Source/WebCore/xml/XMLHttpRequest.cpp:818) WebCore::XMLHttpRequest::send(WTF::String const&, int&) (/WebKitBuild/Debug/../../Source/WebCore/xml/XMLHttpRequest.cpp:644) WebCore::JSXMLHttpRequest::send(JSC::ExecState*) (/WebKitBuild/Debug/../../Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:133) WebCore::jsXMLHttpRequestPrototypeFunctionSend(JSC::ExecState*) (/WebKitBuild/Debug/DerivedSources/WebCore/JSXMLHttpRequest.cpp:643) 0x3959E0E4 () JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*) (/WebKitBuild/Debug/../../Source/JavaScriptCore/jit/JITCode.h:135) JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) (/WebKitBuild/Debug/../../Source/JavaScriptCore/interpreter/Interpreter.cpp:1286) JSC::eval(JSC::ExecState*) (/WebKitBuild/Debug/../../Source/JavaScriptCore/interpreter/Interpreter.cpp:160) llint_slow_path_call_eval (/WebKitBuild/Debug/../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1489) 0x59927D4 (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Debug/.libs/libjavascriptcoregtk-3.0.so.0.14.1) JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::VM*) (/WebKitBuild/Debug/../../Source/JavaScriptCore/jit/JITCode.h:135) JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) (/WebKitBuild/Debug/../../Source/JavaScriptCore/interpreter/Interpreter.cpp:937) JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) (/WebKitBuild/Debug/../../Source/JavaScriptCore/runtime/Completion.cpp:83) WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) (/WebKitBuild/Debug/../../Source/WebCore/bindings/js/JSMainThreadExecState.h:77) WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) (/WebKitBuild/Debug/../../Source/WebCore/bindings/js/ScriptController.cpp:142) WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) (/WebKitBuild/Debug/../../Source/WebCore/bindings/js/ScriptController.cpp:158) WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) (/WebKitBuild/Debug/../../Source/WebCore/dom/ScriptElement.cpp:316) WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) (/WebKitBuild/Debug/../../Source/WebCore/dom/ScriptElement.cpp:245) WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:312) WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:181) WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:271) WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:290) WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:535) WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:235) WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() (/WebKitBuild/Debug/../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:896) Suppression (error hash=#81F7BC2F68C3BF33#): For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/memory-sheriff#TOC-Suppressing-memory-reports { <insert_a_suppression_name_here> Memcheck:Leak fun:malloc fun:_ZN3WTF10fastMallocEm fun:_ZN3WTF10fastStrDupEPKc fun:_ZN7WebCore12TextCodecICU14registerCodecsEPFvPKcPFN3WTF10PassOwnPtrINS_9TextCodecEEERKNS_12TextEncodingEPKvESB_E fun:_ZN7WebCoreL19extendTextCodecMapsEv fun:_ZN7WebCore31atomicCanonicalTextEncodingNameEPKc fun:_ZN7WebCore31atomicCanonicalTextEncodingNameIhEEPKcPKT_m fun:_ZN7WebCore31atomicCanonicalTextEncodingNameERKN3WTF6StringE fun:_ZN7WebCore12TextEncodingC1ERKN3WTF6StringE fun:_ZN7WebCore14XMLHttpRequest14didReceiveDataEPKci fun:_ZN7WebCore24DocumentThreadableLoader14didReceiveDataEmPKci fun:_ZN7WebCore24DocumentThreadableLoader11loadRequestERKNS_15ResourceRequestENS_19SecurityCheckPolicyE fun:_ZN7WebCore24DocumentThreadableLoaderC1EPNS_8DocumentEPNS_22ThreadableLoaderClientENS0_16BlockingBehaviorERKNS_15ResourceRequestERKNS_23ThreadableLoaderOptionsE fun:_ZN7WebCore24DocumentThreadableLoader25loadResourceSynchronouslyEPNS_8DocumentERKNS_15ResourceRequestERNS_22ThreadableLoaderClientERKNS_23ThreadableLoaderOptionsE fun:_ZN7WebCore16ThreadableLoader25loadResourceSynchronouslyEPNS_22ScriptExecutionContextERKNS_15ResourceRequestERNS_22ThreadableLoaderClientERKNS_23ThreadableLoaderOptionsE fun:_ZN7WebCore14XMLHttpRequest13createRequestERi fun:_ZN7WebCore14XMLHttpRequest4sendERKN3WTF6StringERi fun:_ZN7WebCore16JSXMLHttpRequest4sendEPN3JSC9ExecStateE fun:_ZN7WebCore37jsXMLHttpRequestPrototypeFunctionSendEPN3JSC9ExecStateE obj:* fun:_ZN3JSC7JITCode7executeEPNS_7JSStackEPNS_9ExecStateEPNS_2VME fun:_ZN3JSC11Interpreter7executeEPNS_14EvalExecutableEPNS_9ExecStateENS_7JSValueEPNS_7JSScopeE }
Attachments
Patch (1.46 KB, patch)
2013-07-09 07:18 PDT, Brian Holt 		ap: review-
DetailsFormatted DiffDiff
stop silencing the leak (2.09 KB, patch)
2017-11-28 13:30 PST, Alexey Proskuryakov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Brian Holt
Comment 1 2013-07-09 07:18:23 PDT
Created attachment 206319 [details] Patch
Alexey Proskuryakov
Comment 2 2013-07-09 09:15:40 PDT
Comment on attachment 206319 [details] Patch I think that the leak may be real, but the fix is not right. Generally, these strings are permanently stored in textCodecMap, and not leaked. One case where a leak may occur is when the same webStandardName is registered multiple times - when this happens, map value is thrown away. However, I don't think that it's correct to remove fastStrDup. The result of ucnv_getCanonicalName is not documented to remain useful indefinitely - for example, they may be reusing a static buffer. So, we need our own copy of the string.
Brian Holt
Comment 3 2013-07-09 09:40:22 PDT
(In reply to comment #2) > (From update of attachment 206319 [details]) > I think that the leak may be real, but the fix is not right. > > Generally, these strings are permanently stored in textCodecMap, and not leaked. One case where a leak may occur is when the same webStandardName is registered multiple times - when this happens, map value is thrown away. > > However, I don't think that it's correct to remove fastStrDup. The result of ucnv_getCanonicalName is not documented to remain useful indefinitely - for example, they may be reusing a static buffer. So, we need our own copy of the string. Thanks for your comments. It seems that there might be some inconsistency in the way that the memory is handled. If we need our own copy of the string, should we also make a copy of canonicalConverterName on line 193? And if we're making copies then should the responsibility fall to TextCodecMap to free the additional data? But the additionalData is const implying (at least to me) that the TextCodecMap doesn't own that data. What would you suggest would be the best way to address the leak?
Alexey Proskuryakov
Comment 4 2013-07-09 09:58:15 PDT
> If we need our own copy of the string, should we also make a copy of canonicalConverterName on line 193? Indeed! > And if we're making copies then should the responsibility fall to TextCodecMap to free the additional data? But the additionalData is const implying (at least to me) that the TextCodecMap doesn't own that data. Generally speaking, constness is separate from ownership, it's perfectly fine to free a const pointer. The tricky part here is that additional data is not necessarily a string pointer - TextCodecMap passes a pointer to a number. So the function to delete additional data needs to be provided by caller. > What would you suggest would be the best way to address the leak? I think that we shouldn't enumerate ICU aliases at all, and have our own encoding name table that's suitable for the web instead (similarly to what TextCodecMac does for legacy Mac encodings). That would be a valuable architectural improvement, and it would sidestep this issue entirely. Given the complexity of fixing only this leak, we should consider just implementing the above idea.
Alexey Proskuryakov
Comment 5 2013-11-17 22:19:45 PST
<rdar://problem/15486167>
Alexey Proskuryakov
Comment 6 2014-09-19 19:35:49 PDT
*** Bug 136973 has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 7 2015-04-20 20:06:47 PDT
*** Bug 143975 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 8 2015-04-21 08:45:21 PDT
It would be nice to fix this leak. But keep in mind that we leaked intentionally when originally creating this code, and it’s a little tricky to design this out. I like Alexey’s suggestion above.
Alexey Proskuryakov
Comment 9 2017-11-28 13:26:49 PST
This was fixed in r204605, just need to remove the special case from leaks parser.
Alexey Proskuryakov
Comment 10 2017-11-28 13:30:40 PST
Created attachment 327782 [details] stop silencing the leak
Joseph Pecoraro
Comment 11 2017-11-28 14:19:30 PST
Comment on attachment 327782 [details] stop silencing the leak r=me
WebKit Commit Bot
Comment 12 2017-11-28 14:39:08 PST
Comment on attachment 327782 [details] stop silencing the leak Clearing flags on attachment: 327782 Committed r225240: <https://trac.webkit.org/changeset/225240>
WebKit Commit Bot
Comment 13 2017-11-28 14:39:11 PST
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.