Bug 106137 - REGRESSION(r138222): WebDocumentLoaderMac-related leaks seen on Leaks bot
Summary: REGRESSION(r138222): WebDocumentLoaderMac-related leaks seen on Leaks bot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Nate Chapin
URL:
Keywords:
Depends on: 105330 106123
Blocks: 119853
  Show dependency treegraph
 
Reported: 2013-01-04 15:11 PST by Eric Seidel (no email)
Modified: 2013-08-15 11:39 PDT (History)
16 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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&lt;char, std::__1::char_traits&lt;char&gt;, std::__1::allocator&lt;char&gt; &gt; const&amp;) 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&amp;) FrameLoader.cpp:1286 | WebFrameLoaderClient::createDocumentLoader(WebCore::ResourceRequest const&amp;, WebCore::SubstituteData const&amp;) WebFrameLoaderClient.mm:1135 | WebDocumentLoaderMac::create(WebCore::ResourceRequest const&amp;, WebCore::SubstituteData const&amp;) WebDocumentLoaderMac.h:44 | WTF::RefCounted&lt;WebCore::DocumentLoader&gt;::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..&#34;....
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        0x00000000 0x00000000 0x22011de0 0x00007fb7     ...........&#34;....
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        0x00000000 0x00000000 0x00000000 0x00000000     ................
        0x22011e20 0x00007fb7 0x00000000 0x00000000      ..&#34;............
        ...


It's unclear what exactly is going on, but presumably some major leaking.
Comment 1 Eric Seidel (no email) 2013-01-04 15:16:51 PST
It's possible we're leaking the DocumentLoader on all ports, but that seems unlikely.
Comment 2 Eric Seidel (no email) 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. :)
Comment 3 Adam Barth 2013-01-04 16:58:35 PST
Which test?  All of them?
Comment 4 Ryosuke Niwa 2013-01-07 21:06:38 PST
(In reply to comment #3)
> Which test?  All of them?

It appears to be happening everywhere.
Comment 6 Ryosuke Niwa 2013-01-08 01:40:24 PST
Created attachment 181670 [details]
Leak output sample
Comment 7 Nate Chapin 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
Comment 8 Nate Chapin 2013-01-08 16:53:06 PST
Created attachment 181805 [details]
patch + test
Comment 9 Eric Seidel (no email) 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?
Comment 10 Alexey Proskuryakov 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".
Comment 11 Nate Chapin 2013-01-09 10:24:57 PST
Created attachment 181942 [details]
Without the class variable
Comment 12 Brady Eidson 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.
Comment 13 Nate Chapin 2013-01-10 11:09:21 PST
Created attachment 182172 [details]
Patch for landing
Comment 14 WebKit Review Bot 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>
Comment 15 WebKit Review Bot 2013-01-10 11:23:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Eric Seidel (no email) 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.
Comment 17 Ryosuke Niwa 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.