Bug 107781 – Leak bots erroneously report JSC::WatchpointSet as leaking

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryosuke Niwa]

http://build.webkit.org/LeaksViewer/?url=%2Fresults%2FApple%20MountainLion%20%28Leaks%29%2Fr140529%20%282594%29%2F

Sample stack trace (backwards):

-[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] ResourceHandleMac.mm:785
WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) ResourceLoader.cpp:452
WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool) SubresourceLoader.cpp:228
WebCore::SubresourceLoader::sendDataToResource(char const*, int) SubresourceLoader.cpp:255
WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool) CachedRawResource.cpp:70
WebCore::MainResourceLoader::dataReceived(WebCore::CachedResource*, char const*, int) MainResourceLoader.cpp:515
WebCore::DocumentLoader::receivedData(char const*, int) DocumentLoader.cpp:398
WebCore::DocumentLoader::commitLoad(char const*, int) DocumentLoader.cpp:319
WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) WebFrameLoaderClient.mm:846
-[WebDataSource(WebInternal) _receivedData:] WebDataSource.mm:215
-[WebHTMLRepresentation receivedData:withDataSource:] WebHTMLRepresentation.mm:186
-[WebFrame(WebInternal) _commitData:] WebFrame.mm:826
WebCore::DocumentLoader::commitData(char const*, unsigned long) DocumentLoader.cpp:357
WebCore::DocumentWriter::addData(char const*, unsigned long) DocumentWriter.cpp:222
WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned long) DecodedDataDocumentParser.cpp:50
WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) HTMLDocumentParser.cpp:522
WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) HTMLDocumentParser.cpp:195
WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) HTMLDocumentParser.cpp:332
WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) HTMLDocumentParser.cpp:243
WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() HTMLDocumentParser.cpp:223
WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) HTMLScriptRunner.cpp:180
WebCore::HTMLScriptRunner::executeParsingBlockingScripts() HTMLScriptRunner.cpp:190
WebCore::HTMLScriptRunner::executeParsingBlockingScript() HTMLScriptRunner.cpp:118
WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent(WebCore::PendingScript&) HTMLScriptRunner.cpp:139
WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) ScriptElement.cpp:304
WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) ScriptController.cpp:158
WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) ScriptController.cpp:141
WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) JSMainThreadExecState.h:77
JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) Completion.cpp:76
JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) Interpreter.cpp:955
JSC::ProgramExecutable::initializeGlobalProperties(JSC::JSGlobalData&, JSC::ExecState*, JSC::JSScope*) Executable.cpp:405
JSC::ProgramExecutable::addGlobalVar(JSC::JSGlobalObject*, JSC::Identifier const&, JSC::ProgramExecutable::ConstantMode, JSC::ProgramExecutable::FunctionMode) Executable.cpp:368
JSC::SymbolTableEntry::attemptToWatch() SymbolTable.cpp:71
WTF::RefCounted<JSC::WatchpointSet>::operator new(unsigned long) RefCounted.h:197
WTF::fastMalloc(unsigned long) FastMalloc.cpp:274
malloc
malloc_zone_malloc 
Leak: 0x7f9cb1ae4c40  size=48  zone: DefaultMallocZone_0x10b4af000

[Geoffrey Garen]

<rdar://problem/12999306>

[Filip Pizlo]

This appears to be due to the fact that SymbolTable does low-bit tagging when pointing at the watchpoint set.

[Pratik Solanki]

> This appears to be due to the fact that SymbolTable does low-bit tagging when pointing at the watchpoint set.

So does that mean this isn't really a leak? That the memory is actually referenced and used but leaks doesn't know about it?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

We've found out that this is a false positive due to the fact FatEntry is a tagged pointer. I'm going to post a patch to work-around this problem in leaks by making it an "untagged" pointer.

[Ryosuke Niwa]

Created attachment 193423 [details]
Use 'Untagged pointer' to work around leaks' limitation

[Ryosuke Niwa]

Comment on attachment 193423 [details]
Use 'Untagged pointer' to work around leaks' limitation

View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review

> Source/JavaScriptCore/runtime/SymbolTable.h:132
> -                return m_bits & FatFlag;
> +                return m_bits && !(m_bits & SlimFlag);

I'm somewhat concerned about the performance implication of this change.

> Source/JavaScriptCore/runtime/SymbolTable.h:323
> -            bitsRef = (static_cast<intptr_t>(index) << FlagBits) | NotNullFlag;
> +            bitsRef = (static_cast<intptr_t>(index) << FlagBits) | NotNullFlag | (bitsRef & SlimFlag);

And also this dependency. It appears that the code assumed that FatFlag is never set so we could add ASSERT(!isFat()) and simply | SlimFlag here instead.

[Filip Pizlo]

Comment on attachment 193423 [details]
Use 'Untagged pointer' to work around leaks' limitation

View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review

Looks good, but I kind of want to know why you need to check that m_bits is non-zero in addition to checking that it doesn't have SlimFlag set.

>> Source/JavaScriptCore/runtime/SymbolTable.h:132
>> +                return m_bits && !(m_bits & SlimFlag);
> 
> I'm somewhat concerned about the performance implication of this change.

Why doesn't this just say !(m_bits & SlimFlag)?

>> Source/JavaScriptCore/runtime/SymbolTable.h:323
>> +            bitsRef = (static_cast<intptr_t>(index) << FlagBits) | NotNullFlag | (bitsRef & SlimFlag);
> 
> And also this dependency. It appears that the code assumed that FatFlag is never set so we could add ASSERT(!isFat()) and simply | SlimFlag here instead.

Looks right.

[Filip Pizlo]

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

[Ryosuke Niwa]

Comment on attachment 193423 [details]
Use 'Untagged pointer' to work around leaks' limitation

View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review

>>> Source/JavaScriptCore/runtime/SymbolTable.h:132
>>> +                return m_bits && !(m_bits & SlimFlag);
>> 
>> I'm somewhat concerned about the performance implication of this change.
> 
> Why doesn't this just say !(m_bits & SlimFlag)?

So the reason SymbolTableEntry::isFast checks m_bits && !(m_bits & SlimFlag) is because we have emptyValueIsZero set to true in SymbolTableIndexHashTraits
but we probably don’t need it here if we forced to set SlimFlag or NotNullFlag in Fast::Fast.

[Ryosuke Niwa]

Created attachment 193636 [details]
Simplified pack

[Filip Pizlo]

(In reply to comment #10)
> (From update of attachment 193423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review
> 
> >>> Source/JavaScriptCore/runtime/SymbolTable.h:132
> >>> +                return m_bits && !(m_bits & SlimFlag);
> >> 
> >> I'm somewhat concerned about the performance implication of this change.
> > 
> > Why doesn't this just say !(m_bits & SlimFlag)?
> 
> So the reason SymbolTableEntry::isFast checks m_bits && !(m_bits & SlimFlag) is because we have emptyValueIsZero set to true in SymbolTableIndexHashTraits
> but we probably don’t need it here if we forced to set SlimFlag or NotNullFlag in Fast::Fast.

Can we just change the traits?

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 193423 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review
> > 
> > >>> Source/JavaScriptCore/runtime/SymbolTable.h:132
> > >>> +                return m_bits && !(m_bits & SlimFlag);
> > >> 
> > >> I'm somewhat concerned about the performance implication of this change.
> > > 
> > > Why doesn't this just say !(m_bits & SlimFlag)?
> > 
> > So the reason SymbolTableEntry::isFast checks m_bits && !(m_bits & SlimFlag) is because we have emptyValueIsZero set to true in SymbolTableIndexHashTraits
> > but we probably don’t need it here if we forced to set SlimFlag or NotNullFlag in Fast::Fast.
> 
> Can we just change the traits?

I'm concerned that it may negatively affect the performance but we can try.

[Ryosuke Niwa]

Created attachment 193707 [details]
Use emptyValueIsZero = false

[Maciej Stachowiak]

<rdar://problem/12999306>

[Ryosuke Niwa]

Comment on attachment 193707 [details]
Use emptyValueIsZero = false

Clearing flags on attachment: 193707

Committed r146568: <http://trac.webkit.org/changeset/146568>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.