| Summary: | Leak of WTF::StringImpl under SymbolImpl::createNullSymbol() (48 bytes) in com.apple.WebKit.WebContent running layout tests | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||
| Severity: | Normal | CC: | achristensen, benjamin, cdumez, cmarcelo, commit-queue, darin, darkfloyd, dbates, ews-watchlist, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, rniwa, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Local Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=165247 | ||||||||||
| Bug Depends on: | 194202 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 358722 [details]
Patch v1
Comment on attachment 358722 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=358722&action=review > Source/WTF/wtf/text/SymbolImpl.cpp:59 > - return adoptRef(*new SymbolImpl); > + static NeverDestroyed<Ref<SymbolImpl>> nullSymbol(adoptRef(*new SymbolImpl)); > + return nullSymbol.get().get(); NOTE: I've confirmed that this DOES fix the leak, but I'm not sure exactly why (so it might be papering over the root cause). I need to switch back to another task, so I'm posting this in case it's actually the correct solution and the reviewer is confident of that. Comment on attachment 358722 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=358722&action=review >> Source/WTF/wtf/text/SymbolImpl.cpp:59 >> + return nullSymbol.get().get(); > > NOTE: I've confirmed that this DOES fix the leak, but I'm not sure exactly why (so it might be papering over the root cause). I need to switch back to another task, so I'm posting this in case it's actually the correct solution and the reviewer is confident of that. Oh, why is SymbolImpl::m_owner a raw pointer instead of a RefPtr<StringImpl>? // The pointer to the owner string should be immediately following after the StringImpl layout, // since we would like to align the layout of SymbolImpl to the one of BufferSubstring StringImpl. StringImpl* m_owner; And there is no SymbolImpl:~:SymbolImpl() destructor, so I'm not sure what the ownership model of SymbolImpl::m_owner is (for the null symbol or the non-null symbol case). That's probably the right question to answer to fix this bug. (In reply to David Kilzer (:ddkilzer) from comment #2) > Created attachment 358722 [details] > Patch v1 Ha! This looks like the wrong fix anyway if the jsc-armv7 tests start failing. :) Comment on attachment 358722 [details] Patch v1 Attachment 358722 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10687398 New failing tests: storage/domstorage/localstorage/named-items.html inspector/model/remote-object-get-properties.html Created attachment 358730 [details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358722 [details]
Patch v1
Speculatively marking this as r- as this doesn't seem to be the correct fix.
Comment on attachment 358722 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=358722&action=review >>> Source/WTF/wtf/text/SymbolImpl.cpp:59 >>> + return nullSymbol.get().get(); >> >> NOTE: I've confirmed that this DOES fix the leak, but I'm not sure exactly why (so it might be papering over the root cause). I need to switch back to another task, so I'm posting this in case it's actually the correct solution and the reviewer is confident of that. > > Oh, why is SymbolImpl::m_owner a raw pointer instead of a RefPtr<StringImpl>? > > // The pointer to the owner string should be immediately following after the StringImpl layout, > // since we would like to align the layout of SymbolImpl to the one of BufferSubstring StringImpl. > StringImpl* m_owner; > > And there is no SymbolImpl:~:SymbolImpl() destructor, so I'm not sure what the ownership model of SymbolImpl::m_owner is (for the null symbol or the non-null symbol case). > > That's probably the right question to answer to fix this bug. This is a bit tricky, but I think it works. All the SymbolImpl should have BufferSubstring buffer ownership. Then, the location `m_owner` should be the same to `substringBuffer()` (tailPointer<StringImpl*>(), it is tested by ASSERT). Let's see StringImpl::~StringImpl. It has `->deref()` code for BufferSubstring, and it should release it. Note that, createNullSymbol is expected to create a new null symbol, and it is typically used to create a new unique symbol which can be used as a hidden key in a IdentifierMap. (IIRC). https://bugs.webkit.org/show_bug.cgi?id=194082 I think this is the cause of this issue. (In reply to Yusuke Suzuki from comment #11) > https://bugs.webkit.org/show_bug.cgi?id=194082 I think this is the cause of > this issue. It's not the direct cause, but it did help me to find the root cause. SymbolImpl() needs a destructor to call StringImpl::deref() on `m_owner` since 2 of 3 SymbolImpl constructors leak a StringImpl ref to store the pointer value. Created attachment 360978 [details]
Patch v2
Comment on attachment 360978 [details]
Patch v2
r=me
Comment on attachment 360978 [details] Patch v2 Clearing flags on attachment: 360978 Committed r240896: <https://trac.webkit.org/changeset/240896> All reviewed patches have been landed. Closing bug. (In reply to David Kilzer (:ddkilzer) from comment #12) > (In reply to Yusuke Suzuki from comment #11) > > https://bugs.webkit.org/show_bug.cgi?id=194082 I think this is the cause of > > this issue. > > It's not the direct cause, but it did help me to find the root cause. > > SymbolImpl() needs a destructor to call StringImpl::deref() on `m_owner` > since 2 of 3 SymbolImpl constructors leak a StringImpl ref to store the > pointer value. I have two questions. 1. Why is deref() for m_owner necessary? StringImpl::~StringImpl's BufferSubstring code calls it. SymbolImpl always sets BufferSubString ownership so that this deref() breaks the balance. Is it right? 2. Is this destructor actually called? StringImpl and SymbolImpl do not have virtual destructor. Who calls this destructor? Re-opened since this is blocked by bug 194202 (In reply to WebKit Commit Bot from comment #18) > Re-opened since this is blocked by bug 194202 Rolled out because this isn't working as designed (based on Yusuke's comment): Committed r240903: <https://trac.webkit.org/changeset/240903> (In reply to Yusuke Suzuki from comment #17) > (In reply to David Kilzer (:ddkilzer) from comment #12) > > (In reply to Yusuke Suzuki from comment #11) > > > https://bugs.webkit.org/show_bug.cgi?id=194082 I think this is the cause of > > > this issue. > > > > It's not the direct cause, but it did help me to find the root cause. > > > > SymbolImpl() needs a destructor to call StringImpl::deref() on `m_owner` > > since 2 of 3 SymbolImpl constructors leak a StringImpl ref to store the > > pointer value. > > I have two questions. > > 1. Why is deref() for m_owner necessary? StringImpl::~StringImpl's > BufferSubstring code calls it. SymbolImpl always sets BufferSubString > ownership so that this deref() breaks the balance. Is it right? > 2. Is this destructor actually called? StringImpl and SymbolImpl do not have > virtual destructor. Who calls this destructor? I was unable to break on ~SymbolImpl with either Release or Debug builds using lldb, so there is clearly something wrong with my patch (which is also why I rolled it out yesterday). Also, if ~SymbolImpl isn't getting called, I can't explain why it fixes the leaks! (I had assumed that when a JSC::PrivateName object was destructed via JSC::Symbol::~Symbol(), it would deref JSC::PrivateName::m_uid, and then that WTF::SymbolImpl would be destructed when its ref count reached zero. FWIW, neither JSC::Symbol nor JSC::PrivateName have destructors declared, although I had assumed they'd get default destructors in that case. Note that JSC::Symbol::destroy() does call JSC::Symbol::~Symbol().) Maybe this is a false-positive leak of some sort? If so, it would be good to figure out if we can avoid the false-positive so it's easier to notice true leaks in the WebContent process. (I've run out of time to spend investigating this again, hence unassigning the bug.) This no longer reproduces with newer builds of Xcode, so moving to RESOLVED/CONFIGURATION CHANGED. |
Leak of WTF::StringImpl under SymbolImpl::createNullSymbol() (48 bytes) in com.apple.WebKit.WebContent running layout tests. $ ./Tools/Scripts/run-webkit-tests --no-build --debug --batch-size=1000 --child-processes=1 --verbose --leaks --no-retry --no-show-results crypto/subtle/aes-generate-key-malformed-parameters.html crypto/subtle/aes-import-jwk-key-export-jwk-key.html crypto/subtle/aes-import-jwk-key-export-raw-key.html crypto/subtle/aes-import-key-malformed-parameters.html NOTE: Requires changes to run-webkit-tests to support --leaks with WebKit2. STACK OF 1 INSTANCE OF 'ROOT LEAK: <0x7fbaa3e18b90>': [thread 0x115abd5c0]: 72 libdyld.dylib 0x7fff633ad08d start + 1 71 com.apple.WebKit.WebContent 0x1066d8352 main + 34 XPCServiceMain.mm:165 70 com.apple.WebKit.WebContent 0x1066d8065 WebKit::XPCServiceMain(int, char const**) + 1333 XPCServiceMain.mm:157 69 libxpc.dylib 0x7fff635e39e5 _xpc_copy_xpcservice_dictionary + 0 68 libxpc.dylib 0x7fff635e3ee6 _xpc_objc_main + 555 67 com.apple.Foundation 0x7fff384b828f -[NSRunLoop(NSRunLoop) run] + 76 66 com.apple.Foundation 0x7fff384b83ba -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280 65 com.apple.CoreFoundation 0x7fff36133be6 CFRunLoopRunSpecific + 467 64 com.apple.CoreFoundation 0x7fff36134303 __CFRunLoopRun + 1226 63 com.apple.CoreFoundation 0x7fff36134d5c __CFRunLoopDoSources0 + 195 62 com.apple.CoreFoundation 0x7fff36150eaf __CFRunLoopDoSource0 + 108 61 com.apple.CoreFoundation 0x7fff36150f09 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 60 com.apple.JavaScriptCore 0x3f0efa174 WTF::RunLoop::performWork(void*) + 36 RunLoopCF.cpp:38 59 com.apple.JavaScriptCore 0x3f0ef98cd WTF::RunLoop::performWork() + 445 RunLoop.cpp:124 58 com.apple.JavaScriptCore 0x3f0e93dad WTF::Function<void ()>::operator()() const + 173 Function.h:56 57 com.apple.WebKit 0x10674d939 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() + 25 Function.h:101 56 com.apple.WebKit 0x10674da28 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() + 104 Connection.cpp:957 55 com.apple.WebKit 0x10672ccb7 IPC::Connection::dispatchOneIncomingMessage() + 1607 Connection.cpp:1074 54 com.apple.WebKit 0x10671e2d1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 721 Connection.cpp:0 53 com.apple.WebKit 0x10672beec IPC::Connection::dispatchMessage(IPC::Decoder&) + 476 Connection.cpp:979 52 com.apple.WebKit 0x107549a46 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 166 NetworkProcessConnection.cpp:79 51 com.apple.WebKit 0x1079314dc WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&) + 636 WebResourceLoaderMessageReceiver.cpp:65 50 com.apple.WebKit 0x107931e28 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)) + 296 HandleMessage.h:134 49 com.apple.WebKit 0x107932b20 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebCore::NetworkLoadMetrics>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)) + 96 HandleMessage.h:48 48 com.apple.WebKit 0x107932c9a void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>) + 154 HandleMessage.h:42 47 com.apple.WebKit 0x107557b49 WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) + 457 WebResourceLoader.cpp:154 46 com.apple.WebCore 0x3e299150f WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 799 SubresourceLoader.cpp:636 45 com.apple.WebCore 0x3e2a33d8f WebCore::CachedScript::finishLoading(WebCore::SharedBuffer*) + 143 CachedScript.cpp:104 44 com.apple.WebCore 0x3e2a08501 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*) + 49 CachedResource.cpp:366 43 com.apple.WebCore 0x3e2a0d6af WebCore::CachedResource::checkNotify() + 127 CachedResource.cpp:348 42 com.apple.WebCore 0x3e2237f60 WebCore::LoadableClassicScript::notifyFinished(WebCore::CachedResource&) + 1360 LoadableClassicScript.cpp:118 41 com.apple.WebCore 0x3e2238179 WebCore::LoadableScript::notifyClientFinished() + 329 LoadableScript.cpp:59 40 com.apple.WebCore 0x3e228fa09 WebCore::PendingScript::notifyFinished(WebCore::LoadableScript&) + 25 PendingScript.cpp:75 39 com.apple.WebCore 0x3e228f9a7 WebCore::PendingScript::notifyClientFinished() + 71 PendingScript.cpp:0 38 com.apple.WebCore 0x3e265fec2 WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&) + 434 HTMLDocumentParser.cpp:566 37 com.apple.WebCore 0x3e265fa4d WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() + 445 HTMLDocumentParser.cpp:522 36 com.apple.WebCore 0x3e265c34d WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) + 205 HTMLDocumentParser.cpp:186 35 com.apple.WebCore 0x3e265ca3e WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 526 HTMLDocumentParser.cpp:302 34 com.apple.WebCore 0x3e265df13 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) + 211 HTMLDocumentParser.cpp:254 33 com.apple.WebCore 0x3e265d95d WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 1581 HTMLDocumentParser.cpp:233 32 com.apple.WebCore 0x3e267ef2f WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::DumbPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&) + 79 HTMLScriptRunner.cpp:142 31 com.apple.WebCore 0x3e267f104 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&) + 340 HTMLScriptRunner.cpp:252 30 com.apple.WebCore 0x3e22c1ecd WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 2589 ScriptElement.cpp:267 29 com.apple.WebCore 0x3e22c3c5b WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) + 891 ScriptElement.cpp:387 28 com.apple.WebCore 0x3e1c9e7bd WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*) + 61 ScriptController.cpp:147 27 com.apple.WebCore 0x3e1c9e436 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 310 ScriptController.cpp:131 26 com.apple.WebCore 0x3e1c9e6db WebCore::JSExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 75 JSExecState.h:80 25 com.apple.JavaScriptCore 0x3f229be71 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 97 Completion.cpp:122 24 com.apple.JavaScriptCore 0x3f229bcb5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 565 Completion.cpp:106 23 com.apple.JavaScriptCore 0x3f1fa381f JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 6255 Interpreter.cpp:832 22 com.apple.JavaScriptCore 0x3f1fa428e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 206 JITCodeInlines.h:38 21 com.apple.JavaScriptCore 0x3f134f0f2 vmEntryToJavaScript + 273 LowLevelInterpreter64.asm:295 20 com.apple.JavaScriptCore 0x3f13621eb llint_entry + 77442 LowLevelInterpreter.asm:899 19 com.apple.JavaScriptCore 0x3f1363607 llint_entry + 82590 LowLevelInterpreter.asm:995 18 com.apple.JavaScriptCore 0x3f20b8a91 llint_slow_path_call_eval + 65 LLIntSlowPaths.cpp:1750 17 com.apple.JavaScriptCore 0x3f20b8ce2 JSC::LLInt::commonCallEval(JSC::ExecState*, JSC::Instruction const*, JSC::MacroAssemblerCodePtr<(WTF::PtrTag)357>) + 562 LLIntSlowPaths.cpp:1744 16 com.apple.JavaScriptCore 0x3f1f9e1b2 JSC::eval(JSC::ExecState*) + 2226 Interpreter.cpp:171 15 com.apple.JavaScriptCore 0x3f1f9f8e3 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) + 5427 Interpreter.cpp:1139 14 com.apple.JavaScriptCore 0x3f1fa428e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 206 JITCodeInlines.h:38 13 com.apple.JavaScriptCore 0x3f134f0f2 vmEntryToJavaScript + 273 LowLevelInterpreter64.asm:295 12 com.apple.JavaScriptCore 0x3f13621eb llint_entry + 77442 LowLevelInterpreter.asm:899 11 0x2b61e2e0102d 0x2b61e2e01000 + 45 10 com.apple.JavaScriptCore 0x3f251cc24 JSC::callSymbol(JSC::ExecState*) + 68 SymbolConstructor.cpp:84 9 com.apple.JavaScriptCore 0x3f251c971 JSC::Symbol::create(JSC::VM&) + 65 Symbol.cpp:114 8 com.apple.JavaScriptCore 0x3f251c25d JSC::Symbol::Symbol(JSC::VM&) + 29 Symbol.cpp:42 7 com.apple.JavaScriptCore 0x3f251c23a JSC::Symbol::Symbol(JSC::VM&) + 90 Symbol.cpp:42 6 com.apple.JavaScriptCore 0x3f17a2ca5 JSC::PrivateName::PrivateName() + 21 PrivateName.h:38 5 com.apple.JavaScriptCore 0x3f17a38d5 JSC::PrivateName::PrivateName() + 21 PrivateName.h:38 4 com.apple.JavaScriptCore 0x3f0f21652 WTF::SymbolImpl::createNullSymbol() + 34 SymbolImpl.cpp:56 3 com.apple.JavaScriptCore 0x3f0ea7a55 WTF::StringImpl::operator new(unsigned long) + 21 StringImpl.h:163 2 com.apple.JavaScriptCore 0x3f0eaadcc WTF::fastMalloc(unsigned long) + 124 FastMalloc.cpp:187 1 libsystem_malloc.dylib 0x7fff6355a783 malloc + 24 0 libsystem_malloc.dylib 0x7fff6355a82b malloc_zone_malloc + 139 ==== 1 (48 bytes) ROOT LEAK: 0x7fbaa3e18b90 [48]