Bug 107781

Summary:

Leak bots erroneously report JSC::WatchpointSet as leaking

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

JavaScriptCore

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, fpizlo, ggaren, mjs, psolanki, simon.fraser

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

106716

Attachments:

Description	Flags
Use 'Untagged pointer' to work around leaks' limitation	none
Simplified pack	none
Use emptyValueIsZero = false	none

Ryosuke Niwa

Reported 2013-01-23 20:48:06 PST

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

Attachments
Use 'Untagged pointer' to work around leaks' limitation (5.90 KB, patch) 2013-03-15 21:54 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Simplified pack (5.92 KB, patch) 2013-03-18 13:10 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Use emptyValueIsZero = false (7.00 KB, patch) 2013-03-18 17:49 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Geoffrey Garen

Comment 1 2013-01-24 10:00:51 PST

<rdar://problem/12999306>

Filip Pizlo

Comment 2 2013-01-24 11:29:44 PST

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

Pratik Solanki

Comment 3 2013-02-04 18:27:39 PST

> 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

Comment 4 2013-03-15 17:25:22 PDT

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

Ryosuke Niwa

Comment 5 2013-03-15 17:26:41 PDT

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

Comment 6 2013-03-15 21:54:01 PDT

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

Ryosuke Niwa

Comment 7 2013-03-15 21:55:57 PDT

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 8 2013-03-16 18:46:13 PDT

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

Comment 9 2013-03-16 18:47:37 PDT

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

Ryosuke Niwa

Comment 10 2013-03-17 15:53:36 PDT

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

Comment 11 2013-03-18 13:10:57 PDT

Created attachment 193636 [details] Simplified pack

Filip Pizlo

Comment 12 2013-03-18 13:37:10 PDT

(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

Comment 13 2013-03-18 13:51:06 PDT

(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

Comment 14 2013-03-18 17:49:46 PDT

Created attachment 193707 [details] Use emptyValueIsZero = false

Maciej Stachowiak

Comment 15 2013-03-19 16:38:11 PDT

<rdar://problem/12999306>

Ryosuke Niwa

Comment 16 2013-03-22 00:39:12 PDT

Comment on attachment 193707 [details] Use emptyValueIsZero = false Clearing flags on attachment: 193707 Committed r146568: <http://trac.webkit.org/changeset/146568>

Ryosuke Niwa

Comment 17 2013-03-22 00:39:16 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.