CacheStorage::Engine::Caches::writeRecord is not always calling the c…

…ompletion handler https://bugs.webkit.org/show_bug.cgi?id=183055 Patch by Youenn Fablet <youenn@apple.com> on 2018-02-22 Reviewed by Chris Dumez. Source/WebKit: Add a completion handler to Storage::store. Use it instead in Caches::writeRecord. This ensures that the Cache add/put promise will be called once all write operations have been done. * NetworkProcess/cache/CacheStorageEngineCaches.cpp: (WebKit::CacheStorage::Caches::writeRecord): * NetworkProcess/cache/NetworkCacheStorage.cpp: (WebKit::NetworkCache::Storage::WriteOperation::WriteOperation): (WebKit::NetworkCache::Storage::finishWriteOperation): (WebKit::NetworkCache::Storage::store): * NetworkProcess/cache/NetworkCacheStorage.h: (WebKit::NetworkCache::Storage::store): LayoutTests: * http/tests/cache-storage/resources/cache-persistency-iframe.html: Canonical link: https://commits.webkit.org/198804@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@228935 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Feb 22, 2018 · c09fa9dbc385780ad870f5e3228d7e388793bcca · c09fa9d
1 parent 00a74c2
commit c09fa9dbc385780ad870f5e3228d7e388793bcca
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,12 @@
+2018-02-22  Youenn Fablet  <youenn@apple.com>
+
+        CacheStorage::Engine::Caches::writeRecord is not always calling the completion handler
+        https://bugs.webkit.org/show_bug.cgi?id=183055
+
+        Reviewed by Chris Dumez.
+
+        * http/tests/cache-storage/resources/cache-persistency-iframe.html:
+
 2018-02-22  Chris Dumez  <cdumez@apple.com>
 
         Document.open() cancels existing provisional load but not navigation policy check

diff --git a/LayoutTests/http/tests/cache-storage/resources/cache-persistency-iframe.html b/LayoutTests/http/tests/cache-storage/resources/cache-persistency-iframe.html
@@ -3,31 +3,27 @@
 <body>
     <script>
 var cache;
-function doTest()
+async function doTest()
 {
     if (window.location.hash === "#check") {
-        self.caches.keys().then(keys => {
-            window.parent.postMessage(keys.length === 1 && keys[0] === "testCacheName", "*");
-        });
+        let keys = await self.caches.keys();
+        window.parent.postMessage(keys.length === 1 && keys[0] === "testCacheName", "*");
         return;
     }
 
     if (window.location.hash === "#remove") {
-        self.caches.open("testCacheName").then(c => {
-            cache = c
-            self.caches.delete("testCacheName").then(() => {
-                window.parent.postMessage("removed", "*");
-            });
-        });
+        let cache = await self.caches.open("testCacheName");
+        await self.caches.delete("testCacheName");
+        window.parent.postMessage("removed", "*");
         return;
     }
 
     var cacheName = "testCacheName";
     if (window.location.hash.indexOf("#name=") === 0)
         cacheName = window.location.hash.substring(6);
-    self.caches.open(cacheName).then(() => {
-        window.parent.postMessage("ready", "*");
-    });
+    let cache = await self.caches.open(cacheName);
+    await cache.put(new Request('testurl'), new Response('test body'));
+    window.parent.postMessage("ready", "*");
 }
 doTest();
     </script>

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,23 @@
+2018-02-22  Youenn Fablet  <youenn@apple.com>
+
+        CacheStorage::Engine::Caches::writeRecord is not always calling the completion handler
+        https://bugs.webkit.org/show_bug.cgi?id=183055
+
+        Reviewed by Chris Dumez.
+
+        Add a completion handler to Storage::store.
+        Use it instead in Caches::writeRecord.
+        This ensures that the Cache add/put promise will be called once all write operations have been done.
+
+        * NetworkProcess/cache/CacheStorageEngineCaches.cpp:
+        (WebKit::CacheStorage::Caches::writeRecord):
+        * NetworkProcess/cache/NetworkCacheStorage.cpp:
+        (WebKit::NetworkCache::Storage::WriteOperation::WriteOperation):
+        (WebKit::NetworkCache::Storage::finishWriteOperation):
+        (WebKit::NetworkCache::Storage::store):
+        * NetworkProcess/cache/NetworkCacheStorage.h:
+        (WebKit::NetworkCache::Storage::store):
+
 2018-02-22  Ryosuke Niwa  <rniwa@webkit.org>
 
         Add an entitlement check for service worker on iOS

diff --git a/Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp b/Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp
@@ -470,10 +470,11 @@ void Caches::writeRecord(const Cache& cache, const RecordInformation& recordInfo
 
     if (!shouldPersist()) {
         m_volatileStorage.set(recordInformation.key, WTFMove(record));
+        callback(std::nullopt);
         return;
     }
 
-    m_storage->store(Cache::encode(recordInformation, record), [protectedStorage = makeRef(*m_storage), callback = WTFMove(callback)](const Data&) {
+    m_storage->store(Cache::encode(recordInformation, record), { }, [protectedStorage = makeRef(*m_storage), callback = WTFMove(callback)]() {
         callback(std::nullopt);
     });
 }

diff --git a/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp b/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp
@@ -100,15 +100,17 @@ bool Storage::ReadOperation::finish()
 struct Storage::WriteOperation {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    WriteOperation(Storage& storage, const Record& record, MappedBodyHandler&& mappedBodyHandler)
+    WriteOperation(Storage& storage, const Record& record, MappedBodyHandler&& mappedBodyHandler, CompletionHandler<void()>&& completionHandler)
         : storage(storage)
         , record(record)
         , mappedBodyHandler(WTFMove(mappedBodyHandler))
+        , completionHandler(WTFMove(completionHandler))
     { }
     Ref<Storage> storage;
 
     const Record record;
     const MappedBodyHandler mappedBodyHandler;
+    CompletionHandler<void()> completionHandler;
 
     std::atomic<unsigned> activeCount { 0 };
 };
@@ -800,6 +802,9 @@ void Storage::finishWriteOperation(WriteOperation& writeOperation)
 
     auto protectedThis = makeRef(*this);
 
+    if (writeOperation.completionHandler)
+        writeOperation.completionHandler();
+
     m_activeWriteOperations.remove(&writeOperation);
     dispatchPendingWriteOperations();
 
@@ -832,15 +837,15 @@ void Storage::retrieve(const Key& key, unsigned priority, RetrieveCompletionHand
     dispatchPendingReadOperations();
 }
 
-void Storage::store(const Record& record, MappedBodyHandler&& mappedBodyHandler)
+void Storage::store(const Record& record, MappedBodyHandler&& mappedBodyHandler, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT(RunLoop::isMain());
     ASSERT(!record.key.isNull());
 
     if (!m_capacity)
         return;
 
-    auto writeOperation = std::make_unique<WriteOperation>(*this, record, WTFMove(mappedBodyHandler));
+    auto writeOperation = std::make_unique<WriteOperation>(*this, record, WTFMove(mappedBodyHandler), WTFMove(completionHandler));
     m_pendingWriteOperations.prepend(WTFMove(writeOperation));
 
     // Add key to the filter already here as we do lookups from the pending operations too.

diff --git a/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h b/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h
@@ -30,6 +30,7 @@
 #include "NetworkCacheKey.h"
 #include <WebCore/Timer.h>
 #include <wtf/BloomFilter.h>
+#include <wtf/CompletionHandler.h>
 #include <wtf/Deque.h>
 #include <wtf/Function.h>
 #include <wtf/HashSet.h>
@@ -62,7 +63,7 @@ class Storage : public ThreadSafeRefCounted<Storage> {
     void retrieve(const Key&, unsigned priority, RetrieveCompletionHandler&&);
 
     typedef Function<void (const Data& mappedBody)> MappedBodyHandler;
-    void store(const Record&, MappedBodyHandler&&);
+    void store(const Record&, MappedBodyHandler&&, CompletionHandler<void()>&& = { });
 
     void remove(const Key&);
     void remove(const Vector<Key>&, Function<void ()>&&);