NetworkResourceLoader::setDefersLoading() may cause start() to be called multiple...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Aug 2017 05:57:45 +0000 (05:57 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Aug 2017 05:57:45 +0000 (05:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=175109
<rdar://problem/33363169>

Reviewed by Brady Eidson.

Source/WebKit:

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:

LayoutTests:

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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@220185 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect-expected.txt
LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect.html
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
Source/WebKit/NetworkProcess/NetworkResourceLoader.h

index 6faf1e7900f238d1dff1932063d5ee061b997a5c..7660402d334ac61ae130ca2171a7222c22fd6f83 100644 (file)
@@ -1,3 +1,17 @@
+2017-08-02  Chris Dumez  <cdumez@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-02  Youenn Fablet  <youenn@apple.com>
 
         HTTP tests with 'https' suffix are only run over HTTPS for WK2, not WK1
index f1332872f065782b19c4de43f649617c1c48fd27..e7e173fbc93017b66f331ef734d67833fc3b76f5 100644 (file)
@@ -3,7 +3,7 @@ Test redirect caching
 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 source: Network
 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 source: Network
 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 source: Disk cache
 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 source: Network
 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 source: Network
 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 source: Memory cache
 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 source: Network
 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 source: Network
 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 source: Memory cache
 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
index f92d2e208b1a21d80b0f9c6f14eecf1cae95408a..58898fec0215369e02865ccbe694d973fd90ead8 100644 (file)
@@ -10,6 +10,10 @@ var testMatrix =
   { 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' } },
   ],
  [
   { },
index 10c6056f7d6d32301df905792b9029955b7f3dcd..3ab11062f1b30c64f777bddb73d9c51ac98a4b07 100644 (file)
@@ -1,3 +1,40 @@
+2017-08-02  Chris Dumez  <cdumez@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  Andreas Kling  <akling@apple.com>
 
         NetworkRTCProvider::createResolver() leaks CFHost objects
index a0df505aeb7cb3dd7b37c4ef897d6479f9ca78e0..c76a397c3df4b4e2e4db73b51185b7bf25366323 100644 (file)
@@ -161,6 +161,9 @@ void NetworkResourceLoader::start()
         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 @@ void NetworkResourceLoader::setDefersLoading(bool defers)
         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()
index fcc992236e403ab8590c3048d27ffef29bbc0b75..e57e7af310cebd65ba5e713d15f05d0981a0e732 100644 (file)
@@ -151,6 +151,7 @@ private:
     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 };