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 }
Created attachment 206319 [details] Patch
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.
(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?
> 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.
<rdar://problem/15486167>
*** Bug 136973 has been marked as a duplicate of this bug. ***
*** Bug 143975 has been marked as a duplicate of this bug. ***
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.
This was fixed in r204605, just need to remove the special case from leaks parser.
Created attachment 327782 [details] stop silencing the leak
Comment on attachment 327782 [details] stop silencing the leak r=me
Comment on attachment 327782 [details] stop silencing the leak Clearing flags on attachment: 327782 Committed r225240: <https://trac.webkit.org/changeset/225240>
All reviewed patches have been landed. Closing bug.