Bug 106137

Summary:

REGRESSION(r138222): WebDocumentLoaderMac-related leaks seen on Leaks bot

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Page Loading

Assignee:

Nate Chapin <japhet>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

abarth, ap, barraclough, beidson, cgarcia, eric.carlson, ggaren, japhet, kling, koivisto, lforschler, mjs, mrowe, rniwa, thorton, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

105330, 106123

Bug Blocks:

119853

Attachments:

Description	Flags
Leak output sample	none
patch + test	none
Without the class variable	none
Patch for landing	none

Eric Seidel (no email)

Reported 2013-01-04 15:11:43 PST

WebDocumentLoaderMac-related leaks seen on Leaks bot The leaks bot is producing gigibytes of logs (literally) to document (primarily a set of leaks around WebDocumentLoaderMac Such as: Call stack: [thread 0x7fff734a4180]: | start | main DumpRenderTree.mm:931 | dumpRenderTree(int, char const**) DumpRenderTree.mm:893 | runTestingServerLoop() DumpRenderTree.mm:846 | runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) DumpRenderTree.mm:1417 | -[WebFrame loadHTMLString:baseURL:] WebFrame.mm:1443 | -[WebFrame _loadHTMLString:baseURL:unreachableURL:] WebFrame.mm:1436 | -[WebFrame _loadData:MIMEType:textEncodingName:baseURL:unreachableURL:] WebFrame.mm:1419 | WebCore::FrameLoader::load(WebCore::FrameLoadRequest const&) FrameLoader.cpp:1286 | WebFrameLoaderClient::createDocumentLoader(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) WebFrameLoaderClient.mm:1135 | WebDocumentLoaderMac::create(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) WebDocumentLoaderMac.h:44 | WebDocumentLoaderMac::WebDocumentLoaderMac(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) WebDocumentLoaderMac.mm:41 | WebDocumentLoaderMac::WebDocumentLoaderMac(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) WebDocumentLoaderMac.mm:40 | WebCore::DocumentLoader::DocumentLoader(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) DocumentLoader.cpp:91 | WebCore::ResourceResponse::ResourceResponse() ResourceResponse.h:45 | WebCore::ResourceResponse::ResourceResponse() ResourceResponse.h:44 | WebCore::ResourceResponseBase::ResourceResponseBase() ResourceResponseBase.cpp:70 | WebCore::HTTPHeaderMap::HTTPHeaderMap() HTTPHeaderMap.cpp:42 | WebCore::HTTPHeaderMap::HTTPHeaderMap() HTTPHeaderMap.cpp:42 | WTF::HashMap<WTF::AtomicString, WTF::String, WTF::CaseFoldingHash, WTF::HashTraits<WTF::AtomicString>, WTF::HashTraits<WTF::String> >::HashMap() HashMap.h:43 | WTF::HashTable<WTF::AtomicString, WTF::KeyValuePair<WTF::AtomicString, WTF::String>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::AtomicString, WTF::String> >, WTF::CaseFoldingHash, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>, WTF::HashTraits<WTF::String> >, WTF::HashTraits<WTF::AtomicString> >::HashTable() HashTable.h:560 | WTF::HashTable<WTF::AtomicString, WTF::KeyValuePair<WTF::AtomicString, WTF::String>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::AtomicString, WTF::String> >, WTF::CaseFoldingHash, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>, WTF::HashTraits<WTF::String> >, WTF::HashTraits<WTF::AtomicString> >::HashTable() HashTable.h:554 | WTF::Mutex::operator new(unsigned long) ThreadingPrimitives.h:76 | WTF::fastMalloc(unsigned long) FastMalloc.cpp:274 | malloc | malloc_zone_malloc And: Call stack: [thread 0x7fff734a4180]: | start | main DumpRenderTree.mm:931 | dumpRenderTree(int, char const**) DumpRenderTree.mm:893 | runTestingServerLoop() DumpRenderTree.mm:846 | runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) DumpRenderTree.mm:1417 | -[WebFrame loadHTMLString:baseURL:] WebFrame.mm:1443 | -[WebFrame _loadHTMLString:baseURL:unreachableURL:] WebFrame.mm:1436 | -[WebFrame _loadData:MIMEType:textEncodingName:baseURL:unreachableURL:] WebFrame.mm:1419 | WebCore::FrameLoader::load(WebCore::FrameLoadRequest const&) FrameLoader.cpp:1286 | WebFrameLoaderClient::createDocumentLoader(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) WebFrameLoaderClient.mm:1135 | WebDocumentLoaderMac::create(WebCore::ResourceRequest const&, WebCore::SubstituteData const&) WebDocumentLoaderMac.h:44 | WTF::RefCounted<WebCore::DocumentLoader>::operator new(unsigned long) RefCounted.h:197 | WTF::fastMalloc(unsigned long) FastMalloc.cpp:274 | malloc | malloc_zone_malloc Leak: 0x7fb71886b200 size=2560 zone: DefaultMallocZone_0x10806e000 WebDocumentLoaderMac C++ WebKit 0x08388b00 0x00000001 0x00000001 0x01000000 ..8............. 0x00000000 0x00000000 0x22011c70 0x00007fb7 ........p..".... 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x00000000 0x00000000 0x22011de0 0x00007fb7 ...........".... 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x00000000 0x00000000 0x00000000 0x00000000 ................ 0x22011e20 0x00007fb7 0x00000000 0x00000000 .."............ ... It's unclear what exactly is going on, but presumably some major leaking.

Attachments
Leak output sample (8.20 MB, text/plain) 2013-01-08 01:40 PST, Ryosuke Niwa	no flags	Details
patch + test (2.98 KB, patch) 2013-01-08 16:53 PST, Nate Chapin	no flags	Details Formatted Diff Diff
Without the class variable (2.21 KB, patch) 2013-01-09 10:24 PST, Nate Chapin	no flags	Details Formatted Diff Diff
Patch for landing (2.29 KB, patch) 2013-01-10 11:09 PST, Nate Chapin	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2013-01-04 15:16:51 PST

It's possible we're leaking the DocumentLoader on all ports, but that seems unlikely.

Eric Seidel (no email)

Comment 2 2013-01-04 15:23:25 PST

The log is here: http://build.webkit.org/builders/Apple%20MountainLion%20%28Leaks%29/builds/2277/steps/layout-test/logs/stdio I recommend using curl to grab the first 30mb or so, as its easier to view locally than in a browser. :)

Adam Barth

Comment 3 2013-01-04 16:58:35 PST

Which test? All of them?

Ryosuke Niwa

Comment 4 2013-01-07 21:06:38 PST

(In reply to comment #3) > Which test? All of them? It appears to be happening everywhere.

Ryosuke Niwa

Comment 5 2013-01-08 01:32:45 PST

Now, http://trac.webkit.org/changeset/138218: blackberry change http://trac.webkit.org/changeset/138219: rebaseline http://trac.webkit.org/changeset/138220: chromium change http://trac.webkit.org/changeset/138221: ditto http://trac.webkit.org/changeset/138222: loader change

Ryosuke Niwa

Comment 6 2013-01-08 01:40:24 PST

Created attachment 181670 [details] Leak output sample

Nate Chapin

Comment 7 2013-01-08 09:24:15 PST

(In reply to comment #6) > Created an attachment (id=181670) [details] > Leak output sample I'm fairly confident that this has the same route cause as https://bugs.webkit.org/show_bug.cgi?id=106123

Nate Chapin

Comment 8 2013-01-08 16:53:06 PST

Created attachment 181805 [details] patch + test

Eric Seidel (no email)

Comment 9 2013-01-08 16:59:05 PST

Comment on attachment 181805 [details] patch + test View in context: https://bugs.webkit.org/attachment.cgi?id=181805&action=review > Source/WebCore/ChangeLog:14 > + (WebCore::MainResourceLoader::receivedError): Call dispatchDidFailLoading() if > + a SubstituteData load fails or is cancelled. Without this call, load counts > + are not balanced on WebDocumentLoaderMac and it is retained forever. Is there a time in the code where we expect WebDocumentLoaderMac to go away that we can ASSERT it has 1 ref before we drop our last one?

Alexey Proskuryakov

Comment 10 2013-01-08 17:01:48 PST

Comment on attachment 181805 [details] patch + test Brady should probably review this. Additional state variables in loader make me unhappy though, especially ones as unclear as "m_finishing".

Nate Chapin

Comment 11 2013-01-09 10:24:57 PST

Created attachment 181942 [details] Without the class variable

Brady Eidson

Comment 12 2013-01-09 16:42:05 PST

Comment on attachment 181942 [details] Without the class variable View in context: https://bugs.webkit.org/attachment.cgi?id=181942&action=review > Source/WebCore/loader/MainResourceLoader.cpp:103 > + if (m_substituteDataLoadIdentifier) > + frameLoader()->client()->dispatchDidFailLoading(documentLoader(), m_substituteDataLoadIdentifier, error); Would like to see ASSERT(!loader()) in here.

Nate Chapin

Comment 13 2013-01-10 11:09:21 PST

Created attachment 182172 [details] Patch for landing

WebKit Review Bot

Comment 14 2013-01-10 11:23:34 PST

Comment on attachment 182172 [details] Patch for landing Clearing flags on attachment: 182172 Committed r139343: <http://trac.webkit.org/changeset/139343>

WebKit Review Bot

Comment 15 2013-01-10 11:23:39 PST

All reviewed patches have been landed. Closing bug.

Eric Seidel (no email)

Comment 16 2013-01-10 11:24:56 PST

Hot damn! It will be very interesting to see what the leaks bot shows now that this huge leak is gone.

Ryosuke Niwa

Comment 17 2013-01-10 11:29:44 PST

(In reply to comment #16) > Hot damn! It will be very interesting to see what the leaks bot shows now that this huge leak is gone. Unfortunately, it’s not much better :( We still have a lot of leaks.

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