<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[206062] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/206062">206062</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2016-09-16 20:18:45 -0700 (Fri, 16 Sep 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Cancelling one frame's load cancels load in other frames that have the same URL as well
https://bugs.webkit.org/show_bug.cgi?id=162094
Reviewed by Antti Koivisto.
Source/WebCore:
Cancelling one frame's load cancels load in other frames that have the same URL as well.
So if you have several frames that are loading URL X and you navigate one of the frames
to Y, then the load of X will be cancelled and this frame will navigate to Y. All other
frames will not load URL X even though they should.
The issue is that all the DocumentLoaders share the same CachedResource because of the
memoryCache. When we call DocumentLoader::stopLoading(), it will cancel the
CachedResource's load even though there are several clients for this CachedResource
and other clients still want the load.
The approach chosen in this patch is to not reuse CachedResources that are still
loading when trying to load a main resource. This is not the most efficient approach.
I still chose this approach because:
- It is very unlikely to introduce new bugs.
- The change is very simple.
- This is a corner case (several iframes having the same URL and cancelling the load in
one of them).
Test: http/tests/navigation/frames-same-url-cancel-load.html
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::determineRevalidationPolicy):
LayoutTests:
Add layout test coverage.
* http/tests/cache/iframe-detach-expected.txt: Added.
* http/tests/cache/iframe-detach.html: Added.
* http/tests/cache/resources/slow-iframe.php: Added.
Import Alex Christensen's test from Bug 157563.
* http/tests/navigation/frames-same-url-cancel-load-expected.txt: Added.
* http/tests/navigation/frames-same-url-cancel-load.html: Added.
* http/tests/navigation/resources/success.html: Added.
* http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestshttptestssecurityXFrameOptionsxframeoptionsdenymultipleclientsexpectedtxt">trunk/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreloadercacheCachedResourceLoadercpp">trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestshttptestscacheiframedetachexpectedtxt">trunk/LayoutTests/http/tests/cache/iframe-detach-expected.txt</a></li>
<li><a href="#trunkLayoutTestshttptestscacheiframedetachhtml">trunk/LayoutTests/http/tests/cache/iframe-detach.html</a></li>
<li><a href="#trunkLayoutTestshttptestscacheresourcesslowiframephp">trunk/LayoutTests/http/tests/cache/resources/slow-iframe.php</a></li>
<li><a href="#trunkLayoutTestshttptestsnavigationframessameurlcancelloadexpectedtxt">trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load-expected.txt</a></li>
<li><a href="#trunkLayoutTestshttptestsnavigationframessameurlcancelloadhtml">trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load.html</a></li>
<li><a href="#trunkLayoutTestshttptestsnavigationresourcessuccesshtml">trunk/LayoutTests/http/tests/navigation/resources/success.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (206061 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2016-09-17 02:53:09 UTC (rev 206061)
+++ trunk/LayoutTests/ChangeLog 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -1,3 +1,22 @@
</span><ins>+2016-09-16 Chris Dumez <cdumez@apple.com>
+
+ Cancelling one frame's load cancels load in other frames that have the same URL as well
+ https://bugs.webkit.org/show_bug.cgi?id=162094
+
+ Reviewed by Antti Koivisto.
+
+ Add layout test coverage.
+
+ * http/tests/cache/iframe-detach-expected.txt: Added.
+ * http/tests/cache/iframe-detach.html: Added.
+ * http/tests/cache/resources/slow-iframe.php: Added.
+ Import Alex Christensen's test from Bug 157563.
+
+ * http/tests/navigation/frames-same-url-cancel-load-expected.txt: Added.
+ * http/tests/navigation/frames-same-url-cancel-load.html: Added.
+ * http/tests/navigation/resources/success.html: Added.
+ * http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt:
+
</ins><span class="cx"> 2016-09-16 Joseph Pecoraro <pecoraro@apple.com>
</span><span class="cx">
</span><span class="cx"> Web Inspector: Implement Copy CSS Selector and Copy Xpath Selector context menus
</span></span></pre></div>
<a id="trunkLayoutTestshttptestscacheiframedetachexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/cache/iframe-detach-expected.txt (0 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/cache/iframe-detach-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/cache/iframe-detach-expected.txt 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -0,0 +1,2 @@
</span><ins>+ALERT: PASS - iframe loaded
+
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestscacheiframedetachhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/cache/iframe-detach.html (0 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/cache/iframe-detach.html (rev 0)
+++ trunk/LayoutTests/http/tests/cache/iframe-detach.html 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -0,0 +1,27 @@
</span><ins>+<html>
+<body>
+<div id="shadow">
+<iframe src="resources/slow-iframe.php"></iframe>
+</div>
+
+<div id="empty"></div>
+</body>
+
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function set2() {
+ document.getElementById("shadow").innerHTML = "";
+}
+
+function set1() {
+ document.getElementById("empty").innerHTML = document.getElementById("shadow").innerHTML;
+ setTimeout(set2, 250);
+}
+
+setTimeout(set1, 250);
+</script>
+</html>
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestscacheresourcesslowiframephp"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/cache/resources/slow-iframe.php (0 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/cache/resources/slow-iframe.php (rev 0)
+++ trunk/LayoutTests/http/tests/cache/resources/slow-iframe.php 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -0,0 +1,5 @@
</span><ins>+<?php
+sleep(1);
+header("Content-Type: text/html");
+echo("<script>alert('PASS - iframe loaded'); if (window.testRunner) testRunner.notifyDone(); </script>");
+?>
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestsnavigationframessameurlcancelloadexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load-expected.txt (0 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load-expected.txt 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -0,0 +1,10 @@
</span><ins>+Test that when several frames are loading the same URL, and you navigate one, it does not cancel the load for the other frames.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS frame2.contentDocument.URL is "http://127.0.0.1:8000/navigation/resources/success.html"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestsnavigationframessameurlcancelloadhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load.html (0 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load.html (rev 0)
+++ trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load.html 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -0,0 +1,22 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<body>
+<script src="/js-test-resources/js-test-pre.js"></script>
+<iframe id="frame1" src="resources/success.html"></iframe>
+<iframe id="frame2" src="resources/success.html"></iframe>
+<script>
+description("Test that when several frames are loading the same URL, and you navigate one, it does not cancel the load for the other frames.");
+jsTestIsAsync = true;
+
+document.getElementById("frame1").src = "about:blank";
+
+onload = function() {
+ frame2 = document.getElementById("frame2");
+ shouldBeEqualToString("frame2.contentDocument.URL", "http://127.0.0.1:8000/navigation/resources/success.html");
+
+ finishJSTest();
+}
+</script>
+<script src="/js-test-resources/js-test-post.js"></script>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestsnavigationresourcessuccesshtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/navigation/resources/success.html (0 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/navigation/resources/success.html (rev 0)
+++ trunk/LayoutTests/http/tests/navigation/resources/success.html 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -0,0 +1 @@
</span><ins>+SUCCESS
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestssecurityXFrameOptionsxframeoptionsdenymultipleclientsexpectedtxt"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt (206061 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt 2016-09-17 02:53:09 UTC (rev 206061)
+++ trunk/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -1,3 +1,5 @@
</span><span class="cx"> CONSOLE MESSAGE: Refused to display 'http://127.0.0.1:8000/security/XFrameOptions/resources/x-frame-options-deny.cgi' in a frame because it set 'X-Frame-Options' to 'deny'.
</span><span class="cx"> ALERT: PASS: onload fired.
</span><ins>+CONSOLE MESSAGE: Refused to display 'http://127.0.0.1:8000/security/XFrameOptions/resources/x-frame-options-deny.cgi' in a frame because it set 'X-Frame-Options' to 'deny'.
+ALERT: PASS: onload fired.
</ins><span class="cx"> Test that two main resources pointing to the same url that are canceled within didReceiveResponse() don't cause us to crash.
</span></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (206061 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2016-09-17 02:53:09 UTC (rev 206061)
+++ trunk/Source/WebCore/ChangeLog 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -1,3 +1,34 @@
</span><ins>+2016-09-16 Chris Dumez <cdumez@apple.com>
+
+ Cancelling one frame's load cancels load in other frames that have the same URL as well
+ https://bugs.webkit.org/show_bug.cgi?id=162094
+
+ Reviewed by Antti Koivisto.
+
+ Cancelling one frame's load cancels load in other frames that have the same URL as well.
+
+ So if you have several frames that are loading URL X and you navigate one of the frames
+ to Y, then the load of X will be cancelled and this frame will navigate to Y. All other
+ frames will not load URL X even though they should.
+
+ The issue is that all the DocumentLoaders share the same CachedResource because of the
+ memoryCache. When we call DocumentLoader::stopLoading(), it will cancel the
+ CachedResource's load even though there are several clients for this CachedResource
+ and other clients still want the load.
+
+ The approach chosen in this patch is to not reuse CachedResources that are still
+ loading when trying to load a main resource. This is not the most efficient approach.
+ I still chose this approach because:
+ - It is very unlikely to introduce new bugs.
+ - The change is very simple.
+ - This is a corner case (several iframes having the same URL and cancelling the load in
+ one of them).
+
+ Test: http/tests/navigation/frames-same-url-cancel-load.html
+
+ * loader/cache/CachedResourceLoader.cpp:
+ (WebCore::CachedResourceLoader::determineRevalidationPolicy):
+
</ins><span class="cx"> 2016-09-16 Michael Catanzaro <mcatanzaro@igalia.com>
</span><span class="cx">
</span><span class="cx"> ASSERTION FAILED: The string being removed is atomic in the string table of an other thread! iterator != atomicStringTable.end() at Source/WTF/wtf/text/AtomicStringImpl.cpp(453)
</span></span></pre></div>
<a id="trunkSourceWebCoreloadercacheCachedResourceLoadercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp (206061 => 206062)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp 2016-09-17 02:53:09 UTC (rev 206061)
+++ trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp 2016-09-17 03:18:45 UTC (rev 206062)
</span><span class="lines">@@ -928,10 +928,17 @@
</span><span class="cx"> logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonErrorKey());
</span><span class="cx"> return Reload;
</span><span class="cx"> }
</span><del>-
- // For resources that are not yet loaded we ignore the cache policy.
- if (existingResource->isLoading())
</del><ins>+
+ if (existingResource->isLoading()) {
+ // Do not use cached main resources that are still loading because sharing
+ // loading CachedResources in this case causes issues with regards to cancellation.
+ // If one of the DocumentLoader clients decides to cancel the load, then the load
+ // would be cancelled for all other DocumentLoaders as well.
+ if (type == CachedResource::Type::MainResource)
+ return Reload;
+ // For cached subresources that are still loading we ignore the cache policy.
</ins><span class="cx"> return Use;
</span><ins>+ }
</ins><span class="cx">
</span><span class="cx"> auto revalidationDecision = existingResource->makeRevalidationDecision(cachePolicy(type));
</span><span class="cx"> logResourceRevalidationDecision(revalidationDecision, frame());
</span></span></pre>
</div>
</div>
</body>
</html>