Bug 157563

Summary: Resource load cancelled if detaching the first of multiple iframes all loading from the same URL
Product: WebKit Reporter: Hyongyoub Kim <hyongyoub.kim>
Component: FramesAssignee: Alex Christensen <achristensen>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: achristensen, ap, beidson, buildbot, cdumez, dbates, koivisto, rniwa
Priority: P2    
Version: WebKit Local Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test html pages and cgi perl scripts
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews101 for mac-yosemite
none
sub.html missing from test.zip none

Hyongyoub Kim
Reported 2016-05-11 02:57:44 PDT
Created attachment 278609 [details] Test html pages and cgi perl scripts Multiple iframes are loading from the same URL. Before the main resource load completes, the first iframe that started the load gets detached from the page via javascript. The other iframes all see cancelled main resource. test.zip contains two simple test cases. Need cgi/perl enabled webserver. Case 1: html/main.html Case 2: html/main2.html Here is case 1 so you do not have to unzip the file. <html> <body> <div id="shadow"> <iframe src="/cgi-bin/slow-iframe"></iframe> </div> <div id="empty"></div> </body> <script> function set2() { document.getElementById("shadow").innerHTML = ""; } function set1() { document.getElementById("empty").innerHTML = document.getElementById("shadow").innerHTML; setTimeout(set2, 1000); } setTimeout(set1, 1000); </script> </html> The first iframe ('shadow') starts loading /cgi-bin/slow-iframe. 1 second later: set1 adds the second iframe, which also loads from /cgi-bin/slow-iframe. 2 seconds later: set2 detaches the first iframe from the page. 3 seconds later: slow-iframe finally returns the response. IE/Firefox/Chrome all show slow-iframe's response in the second iframe. But safari and other WebKit ports show a blank because the load gets cancelled when we detach the first iframe from the page. When detached from the page, FrameLoader calls stopAllLoaders, which ends up cancelling the main resource load. The second iframe waiting for the load to complete sees a cancelled load. Case 2 is similar but shows the problem with subresource not the main resource. In this case, the main resouce loads quickly so both iframes see the completed resource. But its subresource loads slowly. When the first iframe gets detached, the subresource load gets cancelled. For case 2, IE/Firefox show the subresource in the second iframe. Safari/Chrome/WebKit ports show blank. Chrome is okay for case 1 because it is not caching main resources in MemoryCache... This behavior feels like a bug because, without MemoryCache, we would not see cancelled loads. In case people feel like this sort of iframe usage is a far fetched scenario, many sites in China actually do employ some versions of it...
Attachments
Test html pages and cgi perl scripts (1.31 KB, application/x-zip-compressed)
2016-05-11 02:57 PDT, Hyongyoub Kim 		no flags
Details
Patch (2.28 KB, patch)
2016-05-12 13:33 PDT, Alex Christensen 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (800.86 KB, application/zip)
2016-05-12 14:24 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (644.00 KB, application/zip)
2016-05-12 14:28 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews117 for mac-yosemite (1.41 MB, application/zip)
2016-05-12 14:35 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews101 for mac-yosemite (798.49 KB, application/zip)
2016-05-12 15:38 PDT, Build Bot 		no flags
Details
sub.html missing from test.zip (96 bytes, text/html)
2016-05-12 17:08 PDT, Hyongyoub Kim 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2016-05-12 13:05:41 PDT
This can easily be made into a loading test that shows something that loads in other browsers but not in WebKit. We should probably fix this. Could you please upload sub.html?
Alex Christensen
Comment 2 2016-05-12 13:33:17 PDT
Created attachment 278756 [details] Patch
Alex Christensen
Comment 3 2016-05-12 13:34:17 PDT
I uploaded a patch with just a test. It passes in Chrome, Firefox, and WebKit with the memory cache disabled, but not in WebKit with the memory cache.
Build Bot
Comment 4 2016-05-12 14:24:05 PDT
Comment on attachment 278756 [details] Patch Attachment 278756 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1311180 New failing tests: http/tests/cache/iframe-detach.html
Build Bot
Comment 5 2016-05-12 14:24:09 PDT
Created attachment 278761 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 6 2016-05-12 14:28:29 PDT
Comment on attachment 278756 [details] Patch Attachment 278756 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1311176 New failing tests: http/tests/cache/iframe-detach.html
Build Bot
Comment 7 2016-05-12 14:28:33 PDT
Created attachment 278762 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Build Bot
Comment 8 2016-05-12 14:34:55 PDT
Comment on attachment 278756 [details] Patch Attachment 278756 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1311182 New failing tests: http/tests/cache/iframe-detach.html
Build Bot
Comment 9 2016-05-12 14:35:00 PDT
Created attachment 278765 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 10 2016-05-12 15:38:06 PDT
Comment on attachment 278756 [details] Patch Attachment 278756 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1311508 New failing tests: http/tests/cache/iframe-detach.html
Build Bot
Comment 11 2016-05-12 15:38:11 PDT
Created attachment 278771 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Hyongyoub Kim
Comment 12 2016-05-12 17:08:15 PDT
Created attachment 278784 [details] sub.html missing from test.zip Attaching sub.html. Forgot to include this in the zip file.
Antti Koivisto
Comment 13 2016-05-13 07:03:56 PDT
I suppose main resource loader cancels the load without taking into account that the cached resource may have other clients.
Alex Christensen
Comment 14 2016-05-16 15:05:09 PDT
Yep, here's the call stack: 1 0x1134d3033 WebCore::MemoryCache::remove(WebCore::CachedResource&) 2 0x113de5999 WebCore::SubresourceLoader::willCancel(WebCore::ResourceError const&) 3 0x111cd5294 WebCore::ResourceLoader::cancel(WebCore::ResourceError const&, WebCore::CachedResourceClient*) 4 0x1122e6cd4 WebCore::DocumentLoader::cancelMainResourceLoad(WebCore::ResourceError const&) 5 0x1122e67f9 WebCore::DocumentLoader::stopLoading() 6 0x11264108a WebCore::FrameLoader::stopAllLoaders(WebCore::ClearProvisionalItemPolicy) 7 0x1126455db WebCore::FrameLoader::frameDetached() The SubresourceLoader's CachedResourceLoader has multiple clients. We need to add a check to say "If there are multiple clients, just disconnect this one instead of cancelling", but I'm not sure where.
Hyongyoub Kim
Comment 15 2016-05-16 21:04:56 PDT
I've tried this simple hack in the past. --- a/Source/WebCore/loader/DocumentLoader.cpp +++ b/Source/WebCore/loader/DocumentLoader.cpp @@ -1562,7 +1562,12 @@ void DocumentLoader::cancelMainResourceLoad(const ResourceError& resourceError) cancelPolicyCheckIfNeeded(); - if (mainResourceLoader()) + // Remove this loader from the client list first. And cancel loading only + // if there are no other clients. Another iframe may be loading from the + // same CachedResource and we should not disturb that. + if (m_mainResource && m_mainResource->hasClient(this)) + m_mainResource->removeClient(this); + if (mainResourceLoader() && !m_mainResource->hasClients()) mainResourceLoader()->cancel(error); clearMainResource(); One problem is that the frame loader is detached from the page but loading is still happening through it. So if you change slow-iframe to return 302 redirect after 1 second delay, instead of an html page, the redirect response ends up crashing the engine. WTF.dll!WTFCrash WebCore::FrameLoader::applyUserAgent WebCore::ResourceLoadNotifier::willSendRequest WebCore::ResourceLoader::willSendRequestInternal WebCore::SubresourceLoader::willSendRequestInternal ResourceLoader::willSendRequest WebKit::WebResourceLoader::willSendRequest [...] void FrameLoader::applyUserAgent(ResourceRequest& request) { String userAgent = this->userAgent(request.url()); ASSERT(!userAgent.isNull()); // This assertion trips request.setHTTPUserAgent(userAgent); }
Chris Dumez
Comment 16 2016-09-16 16:24:29 PDT
*** This bug has been marked as a duplicate of bug 162094 ***
Note You need to log in before you can comment on or make changes to this bug.