Title: [206062] trunk
Revision
206062
Author
cdu...@apple.com
Date
2016-09-16 20:18:45 -0700 (Fri, 16 Sep 2016)

Log Message

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:

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (206061 => 206062)


--- trunk/LayoutTests/ChangeLog	2016-09-17 02:53:09 UTC (rev 206061)
+++ trunk/LayoutTests/ChangeLog	2016-09-17 03:18:45 UTC (rev 206062)
@@ -1,3 +1,22 @@
+2016-09-16  Chris Dumez  <cdu...@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:
+
 2016-09-16  Joseph Pecoraro  <pecor...@apple.com>
 
         Web Inspector: Implement Copy CSS Selector and Copy Xpath Selector context menus

Added: trunk/LayoutTests/http/tests/cache/iframe-detach-expected.txt (0 => 206062)


--- 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)
@@ -0,0 +1,2 @@
+ALERT: PASS - iframe loaded
+

Added: trunk/LayoutTests/http/tests/cache/iframe-detach.html (0 => 206062)


--- 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)
@@ -0,0 +1,27 @@
+<html>
+<body>
+<div id="shadow">
+<iframe src=""
+</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>

Added: trunk/LayoutTests/http/tests/cache/resources/slow-iframe.php (0 => 206062)


--- 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)
@@ -0,0 +1,5 @@
+<?php 
+sleep(1);
+header("Content-Type: text/html"); 
+echo("<script>alert('PASS - iframe loaded'); if (window.testRunner) testRunner.notifyDone(); </script>"); 
+?>

Added: trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load-expected.txt (0 => 206062)


--- 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)
@@ -0,0 +1,10 @@
+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
+ 

Added: trunk/LayoutTests/http/tests/navigation/frames-same-url-cancel-load.html (0 => 206062)


--- 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)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<iframe id="frame1" src=""
+<iframe id="frame2" src=""
+<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 = ""
+
+_onload_ = function() {
+    frame2 = document.getElementById("frame2");
+    shouldBeEqualToString("frame2.contentDocument.URL", "http://127.0.0.1:8000/navigation/resources/success.html");
+
+    finishJSTest();
+}
+</script>
+<script src=""
+</body>
+</html>

Added: trunk/LayoutTests/http/tests/navigation/resources/success.html (0 => 206062)


--- 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)
@@ -0,0 +1 @@
+SUCCESS

Modified: trunk/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients-expected.txt (206061 => 206062)


--- 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)
@@ -1,3 +1,5 @@
 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.
+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.
 Test that two main resources pointing to the same url that are canceled within didReceiveResponse() don't cause us to crash.   

Modified: trunk/Source/WebCore/ChangeLog (206061 => 206062)


--- 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)
@@ -1,3 +1,34 @@
+2016-09-16  Chris Dumez  <cdu...@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):
+
 2016-09-16  Michael Catanzaro  <mcatanz...@igalia.com>
 
         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)

Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp (206061 => 206062)


--- 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)
@@ -928,10 +928,17 @@
         logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonErrorKey());
         return Reload;
     }
-    
-    // For resources that are not yet loaded we ignore the cache policy.
-    if (existingResource->isLoading())
+
+    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.
         return Use;
+    }
 
     auto revalidationDecision = existingResource->makeRevalidationDecision(cachePolicy(type));
     logResourceRevalidationDecision(revalidationDecision, frame());
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to