Bug 118505 – Leak: TextCodecICU::registerCodecs is leaking

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Brian Holt]

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
}

[Brian Holt]

Created attachment 206319 [details]
Patch

[Alexey Proskuryakov]

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]

(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]

> 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]

<rdar://problem/15486167>

[Alexey Proskuryakov]

*** Bug 136973 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 143975 has been marked as a duplicate of this bug. ***

[Darin Adler]

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]

This was fixed in r204605, just need to remove the special case from leaks parser.

[Alexey Proskuryakov]

Created attachment 327782 [details]
stop silencing the leak

[Joseph Pecoraro]

Comment on attachment 327782 [details]
stop silencing the leak

r=me

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.