Bug 193291 – Leak of WTF::StringImpl under SymbolImpl::createNullSymbol() (48 bytes) in com.apple.WebKit.WebContent running layout tests

• Component: JavaScriptCore
• Status: RESOLVED CONFIGURATION CHANGED

[David Kilzer (:ddkilzer)]

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]

[David Kilzer (:ddkilzer)]

<rdar://problem/46655953>

[David Kilzer (:ddkilzer)]

Created attachment 358722 [details]
Patch v1

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.  :)

[EWS Watchlist]

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

[EWS Watchlist]

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

[David Kilzer (:ddkilzer)]

Comment on attachment 358722 [details]
Patch v1

Speculatively marking this as r- as this doesn't seem to be the correct fix.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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).

[Yusuke Suzuki]

https://bugs.webkit.org/show_bug.cgi?id=194082 I think this is the cause of this issue.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 360978 [details]
Patch v2

[Keith Miller]

Comment on attachment 360978 [details]
Patch v2

r=me

[WebKit Commit Bot]

Comment on attachment 360978 [details]
Patch v2

Clearing flags on attachment: 360978

Committed r240896: <https://trac.webkit.org/changeset/240896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 194202

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(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.)

[David Kilzer (:ddkilzer)]

This no longer reproduces with newer builds of Xcode, so moving to RESOLVED/CONFIGURATION CHANGED.