We sometimes fail to remove outdated entry from the disk cache after revalidation...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Apr 2016 16:50:29 +0000 (16:50 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Apr 2016 16:50:29 +0000 (16:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156048
<rdar://problem/25514480>

Reviewed by Antti Koivisto.

Source/WebKit2:

We would sometimes fail to remove outdated entry from the disk cache
after revalidation and when the resource is no longer cacheable. This
was due to Storage::removeFromPendingWriteOperations() only removing
the first pending write operation with a given key instead of actually
removing all of the operations with this key.

* NetworkProcess/cache/NetworkCacheStorage.cpp:
(WebKit::NetworkCache::Storage::removeFromPendingWriteOperations):
* NetworkProcess/cache/NetworkCacheStorage.h:

LayoutTests:

Add test coverage for the bug.

* http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes-expected.txt: Added.
* http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes.html: Added.
* http/tests/cache/disk-cache/resources/json.php: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes.html [new file with mode: 0644]
LayoutTests/http/tests/cache/disk-cache/resources/json.php [new file with mode: 0644]
Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp
Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h

index 385da19..a14dc39 100644 (file)
@@ -1,3 +1,17 @@
+2016-04-05  Chris Dumez  <cdumez@apple.com>
+
+        We sometimes fail to remove outdated entry from the disk cache after revalidation and when the resource is no longer cacheable
+        https://bugs.webkit.org/show_bug.cgi?id=156048
+        <rdar://problem/25514480>
+
+        Reviewed by Antti Koivisto.
+
+        Add test coverage for the bug.
+
+        * http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes-expected.txt: Added.
+        * http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes.html: Added.
+        * http/tests/cache/disk-cache/resources/json.php: Added.
+
 2016-04-05  Antti Koivisto  <antti@apple.com>
 
         Shadow DOM: :host() From The First Shadow Context Should Not Style All Shadow Context
diff --git a/LayoutTests/http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes-expected.txt b/LayoutTests/http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes-expected.txt
new file mode 100644 (file)
index 0000000..1add695
--- /dev/null
@@ -0,0 +1,15 @@
+Make sure that we properly remove cached entry if the entry is no longer cacheable after revalidation
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+200: {"version":1}
+200: {"version":1}
+404: {"not":"found"}
+200: {"version":2}
+200: {"version":2}
+PASS bugReproduced is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes.html b/LayoutTests/http/tests/cache/disk-cache/disk-cache-remove-several-pending-writes.html
new file mode 100644 (file)
index 0000000..8a6c1ac
--- /dev/null
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<body>
+<script src="/js-test-resources/js-test-pre.js"></script>
+<script>
+description("Make sure that we properly remove cached entry if the entry is no longer cacheable after revalidation");
+jsTestIsAsync = true;
+
+var id = Math.floor((Math.random() * 1000000000000));
+var bugReproduced = true;
+
+function fetch(url) {
+  return new Promise(function (resolve, reject) {
+    var xhr = new XMLHttpRequest(url);
+    xhr.onerror = reject;
+    xhr.onload = function () {
+      resolve({status: xhr.status, body: JSON.parse(xhr.responseText)});
+    };
+    xhr.open('GET', url);
+    xhr.send();
+  })
+}
+
+function fetchResource() {
+  return fetch('resources/json.php?id=' + id).then(function (resp) {
+    if (resp.body.version === 2) {
+      bugReproduced = false;
+    }
+    debug(resp.status + ': ' + JSON.stringify(resp.body));
+  });
+}
+
+onload = function() {
+  fetchResource()
+    .then(fetchResource)
+    .then(fetchResource)
+    .then(fetchResource)
+    .then(fetchResource)
+    .then(function () {
+      shouldBeFalse("bugReproduced");
+      finishJSTest();
+    })
+    .catch(console.log.bind(console));
+}
+
+</script>
+<script src="/js-test-resources/js-test-post.js"></script>
+</body>
diff --git a/LayoutTests/http/tests/cache/disk-cache/resources/json.php b/LayoutTests/http/tests/cache/disk-cache/resources/json.php
new file mode 100644 (file)
index 0000000..f8a57c4
--- /dev/null
@@ -0,0 +1,42 @@
+<?php
+
+function send304()
+{
+    header("HTTP/1.1 304 Not Modified");
+    header("ETag: foo");
+}
+
+$id = $_GET["id"];
+$count = 1;
+if (isset($_COOKIE[$id])) {
+    $count = $_COOKIE[$id];
+}
+
+setcookie($id, $count + 1);
+
+if ($count == 1) {
+    header("Cache-Control: must-revalidate");
+    header("ETag: foo");
+    header("Content-Type: application/json");
+    echo '{"version": 1}';
+} else if ($count == 2) {
+    send304();
+} else if ($count == 3) {
+    header("HTTP/1.1 404 Not Found");
+    header("Cache-Control: must-revalidate");
+    header("Content-Type: application/json");
+    echo '{"not": "found"}';
+} else if ($count == 4) {
+    if ($_SERVER["HTTP_IF_NONE_MATCH"] == "foo") {
+        send304();
+    } else {
+        header("Cache-Control: must-revalidate");
+        header("ETag: foo");
+        header("Content-Type: application/json");
+        echo '{"version": 2}';
+    }
+} else {
+    send304();
+}
+
+?>
index 4ac36dc..aa760ef 100644 (file)
@@ -1,3 +1,21 @@
+2016-04-05  Chris Dumez  <cdumez@apple.com>
+
+        We sometimes fail to remove outdated entry from the disk cache after revalidation and when the resource is no longer cacheable
+        https://bugs.webkit.org/show_bug.cgi?id=156048
+        <rdar://problem/25514480>
+
+        Reviewed by Antti Koivisto.
+
+        We would sometimes fail to remove outdated entry from the disk cache
+        after revalidation and when the resource is no longer cacheable. This
+        was due to Storage::removeFromPendingWriteOperations() only removing
+        the first pending write operation with a given key instead of actually
+        removing all of the operations with this key.
+
+        * NetworkProcess/cache/NetworkCacheStorage.cpp:
+        (WebKit::NetworkCache::Storage::removeFromPendingWriteOperations):
+        * NetworkProcess/cache/NetworkCacheStorage.h:
+
 2016-04-05  Antoine Quint  <graouts@apple.com>
 
         [WebGL2] Allow enabling WebGL2 with a runtime flag
index 94ab3f1..1f650bf 100644 (file)
@@ -523,16 +523,18 @@ Data Storage::encodeRecord(const Record& record, Optional<BlobStorage::Blob> blo
     return { headerData };
 }
 
-bool Storage::removeFromPendingWriteOperations(const Key& key)
+void Storage::removeFromPendingWriteOperations(const Key& key)
 {
-    auto end = m_pendingWriteOperations.end();
-    for (auto it = m_pendingWriteOperations.begin(); it != end; ++it) {
-        if ((*it)->record.key == key) {
-            m_pendingWriteOperations.remove(it);
-            return true;
-        }
+    while (true) {
+        auto found = m_pendingWriteOperations.findIf([&key](const std::unique_ptr<WriteOperation>& operation) {
+            return operation->record.key == key;
+        });
+
+        if (found == m_pendingWriteOperations.end())
+            break;
+
+        m_pendingWriteOperations.remove(found);
     }
-    return false;
 }
 
 void Storage::remove(const Key& key)
index ca8da8f..c4dd6de 100644 (file)
@@ -122,7 +122,7 @@ private:
     void readRecord(ReadOperation&, const Data&);
 
     void updateFileModificationTime(const String& path);
-    bool removeFromPendingWriteOperations(const Key&);
+    void removeFromPendingWriteOperations(const Key&);
 
     WorkQueue& ioQueue() { return m_ioQueue.get(); }
     WorkQueue& backgroundIOQueue() { return m_backgroundIOQueue.get(); }