[webkit-changes] [220340] branches/safari-604-branch

Mon, 07 Aug 2017 09:42:53 -0700

Title: [220340] branches/safari-604-branch

Diff

Modified: branches/safari-604-branch/LayoutTests/ChangeLog (220339 => 220340)


--- branches/safari-604-branch/LayoutTests/ChangeLog	2017-08-07 16:42:35 UTC (rev 220339)
+++ branches/safari-604-branch/LayoutTests/ChangeLog	2017-08-07 16:42:38 UTC (rev 220340)
@@ -1,5 +1,23 @@
 2017-08-07  Jason Marcell  <jmarc...@apple.com>
 
+        Cherry-pick r220185. rdar://problem/33713534
+
+    2017-08-02  Chris Dumez  <cdu...@apple.com>
+
+            NetworkResourceLoader::setDefersLoading() may cause start() to be called multiple times
+            https://bugs.webkit.org/show_bug.cgi?id=175109
+            <rdar://problem/33363169>
+
+            Reviewed by Brady Eidson.
+
+            Extend test coverage to cover cacheable redirects to a resource that needs
+            revalidation, similarly to the case in the radar.
+
+            * http/tests/cache/disk-cache/disk-cache-redirect-expected.txt:
+            * http/tests/cache/disk-cache/disk-cache-redirect.html:
+
+2017-08-07  Jason Marcell  <jmarc...@apple.com>
+
         Cherry-pick r220163. rdar://problem/33711032
 
     2017-08-02  Andy Estes  <aes...@apple.com>

Modified: branches/safari-604-branch/LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect-expected.txt (220339 => 220340)


--- branches/safari-604-branch/LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect-expected.txt	2017-08-07 16:42:35 UTC (rev 220339)
+++ branches/safari-604-branch/LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect-expected.txt	2017-08-07 16:42:38 UTC (rev 220340)
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-running 12 tests
+running 24 tests
 
 --------Testing loads from disk cache--------
 response headers: {"Status":"301","Location":"unique-cacheable"}
@@ -18,6 +18,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable"}
 response source: Network
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Disk cache after validation
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
 response headers: {"Status":"301","Location":"unique-cacheable","Cache-control":"max-age=0"}
 response source: Network
 
@@ -30,6 +42,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable","Cache-control":"max-age=0"}
 response source: Network
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
 response headers: {"Status":"301","Location":"unique-cacheable","Cache-control":"max-age=100"}
 response source: Disk cache
 
@@ -42,6 +66,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable","Cache-control":"max-age=100"}
 response source: Disk cache
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Disk cache after validation
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Disk cache after validation
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Disk cache after validation
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Disk cache after validation
+
 --------Testing loads through memory cache (XHR behavior)--------
 response headers: {"Status":"301","Location":"unique-cacheable"}
 response source: Memory cache
@@ -55,6 +91,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable"}
 response source: Network
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Memory cache after validation
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
 response headers: {"Status":"301","Location":"unique-cacheable","Cache-control":"max-age=0"}
 response source: Network
 
@@ -67,6 +115,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable","Cache-control":"max-age=0"}
 response source: Network
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
 response headers: {"Status":"301","Location":"unique-cacheable","Cache-control":"max-age=100"}
 response source: Memory cache
 
@@ -79,6 +139,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable","Cache-control":"max-age=100"}
 response source: Memory cache
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
 --------Testing loads through memory cache (subresource behavior)--------
 response headers: {"Status":"301","Location":"unique-cacheable"}
 response source: Memory cache
@@ -92,6 +164,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable"}
 response source: Network
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Memory cache after validation
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg"}
+response source: Network
+
 response headers: {"Status":"301","Location":"unique-cacheable","Cache-control":"max-age=0"}
 response source: Network
 
@@ -104,6 +188,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable","Cache-control":"max-age=0"}
 response source: Network
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=0"}
+response source: Network
+
 response headers: {"Status":"301","Location":"unique-cacheable","Cache-control":"max-age=100"}
 response source: Memory cache
 
@@ -116,6 +212,18 @@
 response headers: {"Status":"307","Location":"unique-cacheable","Cache-control":"max-age=100"}
 response source: Memory cache
 
+response headers: {"Status":"301","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
+response headers: {"Status":"302","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
+response headers: {"Status":"303","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
+response headers: {"Status":"307","Location":"/cache/resources/compass-no-cache.jpg","Cache-control":"max-age=100"}
+response source: Memory cache after validation
+
 PASS successfullyParsed is true
 
 TEST COMPLETE

Modified: branches/safari-604-branch/LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect.html (220339 => 220340)


--- branches/safari-604-branch/LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect.html	2017-08-07 16:42:35 UTC (rev 220339)
+++ branches/safari-604-branch/LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect.html	2017-08-07 16:42:38 UTC (rev 220340)
@@ -10,6 +10,10 @@
   { responseHeaders: {'Status': '302', 'Location': 'unique-cacheable' } },
   { responseHeaders: {'Status': '303', 'Location': 'unique-cacheable' } },
   { responseHeaders: {'Status': '307', 'Location': 'unique-cacheable' } },
+  { responseHeaders: {'Status': '301', 'Location': '/cache/resources/compass-no-cache.jpg' } },
+  { responseHeaders: {'Status': '302', 'Location': '/cache/resources/compass-no-cache.jpg' } },
+  { responseHeaders: {'Status': '303', 'Location': '/cache/resources/compass-no-cache.jpg' } },
+  { responseHeaders: {'Status': '307', 'Location': '/cache/resources/compass-no-cache.jpg' } },
   ],
  [
   { },

Modified: branches/safari-604-branch/Source/WebKit/ChangeLog (220339 => 220340)


--- branches/safari-604-branch/Source/WebKit/ChangeLog	2017-08-07 16:42:35 UTC (rev 220339)
+++ branches/safari-604-branch/Source/WebKit/ChangeLog	2017-08-07 16:42:38 UTC (rev 220340)
@@ -1,3 +1,44 @@
+2017-08-07  Jason Marcell  <jmarc...@apple.com>
+
+        Cherry-pick r220185. rdar://problem/33713534
+
+    2017-08-02  Chris Dumez  <cdu...@apple.com>
+
+            NetworkResourceLoader::setDefersLoading() may cause start() to be called multiple times
+            https://bugs.webkit.org/show_bug.cgi?id=175109
+            <rdar://problem/33363169>
+
+            Reviewed by Brady Eidson.
+
+            If NetworkResourceLoader::setDefersLoading(false) is called by the client while m_networkLoad is null
+            then we call NetworkResourceLoader::start() to start the load. This is needed in the case where
+            a NetworkResourceLoader is constructed in deferred mode so that the load can later be started via
+            setDefersLoading(). However, it is possible for setDefersLoading(false) to be called when start()
+            has already been called, which causes start() to be called multiple times and leads to an assertion
+            hit in debug.
+
+            Normally, setDefersLoading(false) returns without calling start() if m_networkLoad is not null. It
+            relies on m_networkLoad being non-null to determine if start() should be called. This is bad because
+            start() checks the disk cache asynchronously *before* calling startNetworkLoad() and initializing
+            m_networkLoad. Therefore, if setDefersLoading(false) is called *while* we are checking the cache,
+            then we will call incorrectly call start() again.
+
+            In the case of the radar, this happens when we:
+            1. Call start() and check the disk cache
+            2. Retrieve a cached redirect from the cache, which causes a WillSendRequest IPC to be sent to the
+               WebProcess
+            3. The WebProcess calls setDefersLoading(true), sends the ContinueWillSendRequest IPC back and
+               then calls setDefersLoading(false)
+
+            The call to continueWillSendRequest() causes us to retrieve the redirected entry from the cache,
+            asynchronously, which will return no entry and start a load.
+            The later call to setDefersLoading(false) causes us to call start() again and we end up back to step 1.
+
+            * NetworkProcess/NetworkResourceLoader.cpp:
+            (WebKit::NetworkResourceLoader::start):
+            (WebKit::NetworkResourceLoader::setDefersLoading):
+            * NetworkProcess/NetworkResourceLoader.h:
+
 2017-08-02  Jason Marcell  <jmarc...@apple.com>
 
         Cherry-pick r220138. rdar://problem/33692550

Modified: branches/safari-604-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (220339 => 220340)


--- branches/safari-604-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp	2017-08-07 16:42:35 UTC (rev 220339)
+++ branches/safari-604-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp	2017-08-07 16:42:38 UTC (rev 220340)
@@ -161,6 +161,9 @@
         return;
     }
 
+    ASSERT(!m_wasStarted);
+    m_wasStarted = true;
+
 #if ENABLE(NETWORK_CACHE)
     if (canUseCache(originalRequest())) {
         RELEASE_LOG_IF_ALLOWED("start: Checking cache for resource (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ", isMainResource = %d, isSynchronous = %d)", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier, isMainResource(), isSynchronous());
@@ -266,10 +269,10 @@
         return;
     }
 
-    if (!m_defersLoading)
+    if (!m_defersLoading && !m_wasStarted)
         start();
     else
-        RELEASE_LOG_IF_ALLOWED("setDefersLoading: defers = TRUE, but nothing to stop (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
+        RELEASE_LOG_IF_ALLOWED("setDefersLoading: defers = %d, but nothing to do (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", m_defersLoading, m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
 }
 
 void NetworkResourceLoader::cleanup()

Modified: branches/safari-604-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.h (220339 => 220340)


--- branches/safari-604-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.h	2017-08-07 16:42:35 UTC (rev 220339)
+++ branches/safari-604-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.h	2017-08-07 16:42:38 UTC (rev 220340)
@@ -151,6 +151,7 @@
     std::unique_ptr<SynchronousLoadData> m_synchronousLoadData;
     Vector<RefPtr<WebCore::BlobDataFileReference>> m_fileReferences;
 
+    bool m_wasStarted { false };
     bool m_didConsumeSandboxExtensions { false };
     bool m_defersLoading { false };
     size_t m_numBytesReceived { 0 };

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to