DOMCacheStorage should not prevent pages from entering the back/forward cache
https://bugs.webkit.org/show_bug.cgi?id=202608
Reviewed by Youenn Fablet.
Source/WebCore:
Make DOMCacheStorage fully suspendable by queueing all its asynchronous promise resolutions
to a SuspendableTaskQueue. This makes sure that no promises are resolved while suspended
in the page cache.
Test: http/tests/navigation/page-cache-domcachestorage-pending-promise.html
* Modules/cache/DOMCacheStorage.cpp:
(WebCore::DOMCacheStorage::DOMCacheStorage):
(WebCore::DOMCacheStorage::doSequentialMatch):
(WebCore::DOMCacheStorage::match):
(WebCore::DOMCacheStorage::has):
(WebCore::DOMCacheStorage::open):
(WebCore::DOMCacheStorage::doOpen):
(WebCore::DOMCacheStorage::remove):
(WebCore::DOMCacheStorage::doRemove):
(WebCore::DOMCacheStorage::keys):
(WebCore::DOMCacheStorage::canSuspendForDocumentSuspension const):
* Modules/cache/DOMCacheStorage.h:
LayoutTests:
Add layout test coverage.
* http/tests/navigation/page-cache-domcachestorage-pending-promise-expected.txt: Added.
* http/tests/navigation/page-cache-domcachestorage-pending-promise.html: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@250965 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 24c6cd9..2aa8b3b 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2019-10-10 Chris Dumez <cdumez@apple.com>
+
+ DOMCacheStorage should not prevent pages from entering the back/forward cache
+ https://bugs.webkit.org/show_bug.cgi?id=202608
+
+ Reviewed by Youenn Fablet.
+
+ Add layout test coverage.
+
+ * http/tests/navigation/page-cache-domcachestorage-pending-promise-expected.txt: Added.
+ * http/tests/navigation/page-cache-domcachestorage-pending-promise.html: Added.
+
2019-10-10 Miguel Gomez <magomez@igalia.com>
Unreviewed GTK and WPE gardening after r250954.
diff --git a/LayoutTests/http/tests/navigation/page-cache-domcachestorage-pending-promise-expected.txt b/LayoutTests/http/tests/navigation/page-cache-domcachestorage-pending-promise-expected.txt
new file mode 100644
index 0000000..b7f120e
--- /dev/null
+++ b/LayoutTests/http/tests/navigation/page-cache-domcachestorage-pending-promise-expected.txt
@@ -0,0 +1,15 @@
+Tests that a page with pending DOMCacheStorage activity goes into the page cache.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+pageshow - not from cache
+pagehide - entering cache
+pageshow - from cache
+PASS Page was restored from Page Cache
+PASS Opened the cache
+PASS !!restoredFromPageCache is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/navigation/page-cache-domcachestorage-pending-promise.html b/LayoutTests/http/tests/navigation/page-cache-domcachestorage-pending-promise.html
new file mode 100644
index 0000000..d05e5e0
--- /dev/null
+++ b/LayoutTests/http/tests/navigation/page-cache-domcachestorage-pending-promise.html
@@ -0,0 +1,46 @@
+<!-- webkit-test-runner [ enablePageCache=true ] -->
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/js-test-resources/js-test.js"></script>
+</head>
+<body>
+<script>
+description('Tests that a page with pending DOMCacheStorage activity goes into the page cache.');
+jsTestIsAsync = true;
+let restoredFromPageCache = false;
+
+window.addEventListener("pageshow", function(event) {
+ debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
+ if (event.persisted) {
+ testPassed("Page was restored from Page Cache");
+ restoredFromPageCache = true;
+ }
+});
+
+window.addEventListener("pagehide", function(event) {
+ debug("pagehide - " + (event.persisted ? "" : "not ") + "entering cache");
+ if (!event.persisted) {
+ testFailed("Page failed to enter the Page Cache");
+ finishJSTest();
+ }
+
+ caches.open('test').then(() => {
+ testPassed("Opened the cache");
+ shouldBeTrue("!!restoredFromPageCache");
+ finishJSTest();
+ }, () => {
+ testFailed("Failed to open the cache");
+ finishJSTest();
+ });
+});
+
+onload = () => {
+ setTimeout(() => {
+ testLink.click();
+ }, 0);
+}
+</script>
+<a id="testLink" href="resources/page-cache-helper.html" style="display: none">Link</a>
+</body>
+</html>
diff --git a/LayoutTests/platform/ios-wk1/TestExpectations b/LayoutTests/platform/ios-wk1/TestExpectations
index 6f5ac1f..8a56f87 100644
--- a/LayoutTests/platform/ios-wk1/TestExpectations
+++ b/LayoutTests/platform/ios-wk1/TestExpectations
@@ -12,6 +12,7 @@
http/wpt/cache-storage [ Skip ]
http/tests/cache-storage [ Skip ]
http/tests/navigation/page-cache-domcache-pending-promise.html [ Skip ]
+http/tests/navigation/page-cache-domcachestorage-pending-promise.html [ Skip ]
imported/w3c/web-platform-tests/fetch/api/request/destination [ Skip ]
imported/w3c/web-platform-tests/fetch/cross-origin-resource-policy [ Skip ]
diff --git a/LayoutTests/platform/mac-wk1/TestExpectations b/LayoutTests/platform/mac-wk1/TestExpectations
index 75ee166..ba1aa40 100644
--- a/LayoutTests/platform/mac-wk1/TestExpectations
+++ b/LayoutTests/platform/mac-wk1/TestExpectations
@@ -272,6 +272,7 @@
http/tests/appcache/main-resource-redirect-with-sw.html [ Skip ]
http/tests/cache-storage [ Skip ]
http/tests/navigation/page-cache-domcache-pending-promise.html [ Skip ]
+http/tests/navigation/page-cache-domcachestorage-pending-promise.html [ Skip ]
http/tests/cookies/same-site/fetch-in-cross-origin-service-worker.html [ Skip ]
http/tests/cookies/same-site/fetch-in-same-origin-service-worker.html [ Skip ]
http/wpt/cache-storage [ Skip ]
diff --git a/LayoutTests/platform/win/TestExpectations b/LayoutTests/platform/win/TestExpectations
index 0d9e217..383e35d 100644
--- a/LayoutTests/platform/win/TestExpectations
+++ b/LayoutTests/platform/win/TestExpectations
@@ -3800,6 +3800,7 @@
http/tests/appcache/main-resource-redirect-with-sw.html [ Skip ]
http/tests/cache-storage [ Skip ]
http/tests/navigation/page-cache-domcache-pending-promise.html [ Skip ]
+http/tests/navigation/page-cache-domcachestorage-pending-promise.html [ Skip ]
http/tests/inspector/network/resource-response-service-worker.html [ Skip ]
http/tests/workers/service [ Skip ]
http/wpt/cache-storage [ Skip ]
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 074fefb..da8e8b3 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,29 @@
+2019-10-10 Chris Dumez <cdumez@apple.com>
+
+ DOMCacheStorage should not prevent pages from entering the back/forward cache
+ https://bugs.webkit.org/show_bug.cgi?id=202608
+
+ Reviewed by Youenn Fablet.
+
+ Make DOMCacheStorage fully suspendable by queueing all its asynchronous promise resolutions
+ to a SuspendableTaskQueue. This makes sure that no promises are resolved while suspended
+ in the page cache.
+
+ Test: http/tests/navigation/page-cache-domcachestorage-pending-promise.html
+
+ * Modules/cache/DOMCacheStorage.cpp:
+ (WebCore::DOMCacheStorage::DOMCacheStorage):
+ (WebCore::DOMCacheStorage::doSequentialMatch):
+ (WebCore::DOMCacheStorage::match):
+ (WebCore::DOMCacheStorage::has):
+ (WebCore::DOMCacheStorage::open):
+ (WebCore::DOMCacheStorage::doOpen):
+ (WebCore::DOMCacheStorage::remove):
+ (WebCore::DOMCacheStorage::doRemove):
+ (WebCore::DOMCacheStorage::keys):
+ (WebCore::DOMCacheStorage::canSuspendForDocumentSuspension const):
+ * Modules/cache/DOMCacheStorage.h:
+
2019-10-10 youenn fablet <youenn@apple.com>
Add asserts to MediaStreamPrivate observer routines
diff --git a/Source/WebCore/Modules/cache/DOMCacheStorage.cpp b/Source/WebCore/Modules/cache/DOMCacheStorage.cpp
index edf169d..cc289a6 100644
--- a/Source/WebCore/Modules/cache/DOMCacheStorage.cpp
+++ b/Source/WebCore/Modules/cache/DOMCacheStorage.cpp
@@ -31,7 +31,7 @@
#include "JSDOMCache.h"
#include "JSFetchResponse.h"
#include "ScriptExecutionContext.h"
-
+#include "SuspendableTaskQueue.h"
namespace WebCore {
using namespace WebCore::DOMCacheEngine;
@@ -39,10 +39,13 @@
DOMCacheStorage::DOMCacheStorage(ScriptExecutionContext& context, Ref<CacheStorageConnection>&& connection)
: ActiveDOMObject(&context)
, m_connection(WTFMove(connection))
+ , m_taskQueue(SuspendableTaskQueue::create(&context))
{
suspendIfNeeded();
}
+DOMCacheStorage::~DOMCacheStorage() = default;
+
Optional<ClientOrigin> DOMCacheStorage::origin() const
{
auto* origin = scriptExecutionContext() ? scriptExecutionContext()->securityOrigin() : nullptr;
@@ -86,17 +89,17 @@
void DOMCacheStorage::doSequentialMatch(DOMCache::RequestInfo&& info, CacheQueryOptions&& options, Ref<DeferredPromise>&& promise)
{
startSequentialMatch(WTF::map(m_caches, copyCache), WTFMove(info), WTFMove(options), [this, pendingActivity = makePendingActivity(*this), promise = WTFMove(promise)](auto&& result) mutable {
- if (m_isStopped)
- return;
- if (result.hasException()) {
- promise->reject(result.releaseException());
- return;
- }
- if (!result.returnValue()) {
- promise->resolve();
- return;
- }
- promise->resolve<IDLInterface<FetchResponse>>(*result.returnValue());
+ m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
+ if (result.hasException()) {
+ promise->reject(result.releaseException());
+ return;
+ }
+ if (!result.returnValue()) {
+ promise->resolve();
+ return;
+ }
+ promise->resolve<IDLInterface<FetchResponse>>(*result.returnValue());
+ });
});
}
@@ -104,7 +107,9 @@
{
retrieveCaches([this, info = WTFMove(info), options = WTFMove(options), promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
if (exception) {
- promise->reject(WTFMove(exception.value()));
+ m_taskQueue->enqueueTask([promise = WTFMove(promise), exception = WTFMove(exception.value())]() mutable {
+ promise->reject(WTFMove(exception));
+ });
return;
}
@@ -114,7 +119,9 @@
m_caches[position]->match(WTFMove(info), WTFMove(options), WTFMove(promise));
return;
}
- promise->resolve();
+ m_taskQueue->enqueueTask([promise = WTFMove(promise)]() mutable {
+ promise->resolve();
+ });
return;
}
@@ -125,11 +132,13 @@
void DOMCacheStorage::has(const String& name, DOMPromiseDeferred<IDLBoolean>&& promise)
{
retrieveCaches([this, name, promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
- if (exception) {
- promise.reject(WTFMove(exception.value()));
- return;
- }
- promise.resolve(m_caches.findMatching([&](auto& item) { return item->name() == name; }) != notFound);
+ m_taskQueue->enqueueTask([this, name, promise = WTFMove(promise), exception = WTFMove(exception)]() mutable {
+ if (exception) {
+ promise.reject(WTFMove(exception.value()));
+ return;
+ }
+ promise.resolve(m_caches.findMatching([&](auto& item) { return item->name() == name; }) != notFound);
+ });
});
}
@@ -180,7 +189,9 @@
{
retrieveCaches([this, name, promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
if (exception) {
- promise.reject(WTFMove(exception.value()));
+ m_taskQueue->enqueueTask([promise = WTFMove(promise), exception = WTFMove(exception.value())]() mutable {
+ promise.reject(WTFMove(exception));
+ });
return;
}
doOpen(name, WTFMove(promise));
@@ -191,23 +202,26 @@
{
auto position = m_caches.findMatching([&](auto& item) { return item->name() == name; });
if (position != notFound) {
- auto& cache = m_caches[position];
- promise.resolve(DOMCache::create(*scriptExecutionContext(), String { cache->name() }, cache->identifier(), m_connection.copyRef()));
+ m_taskQueue->enqueueTask([this, promise = WTFMove(promise), cache = m_caches[position].copyRef()]() mutable {
+ promise.resolve(DOMCache::create(*scriptExecutionContext(), String { cache->name() }, cache->identifier(), m_connection.copyRef()));
+ });
return;
}
m_connection->open(*origin(), name, [this, name, promise = WTFMove(promise), pendingActivity = makePendingActivity(*this)](const CacheIdentifierOrError& result) mutable {
- if (!m_isStopped) {
- if (!result.has_value())
- promise.reject(DOMCacheEngine::convertToExceptionAndLog(scriptExecutionContext(), result.error()));
- else {
- if (result.value().hadStorageError)
- logConsolePersistencyError(scriptExecutionContext(), name);
+ if (!result.has_value()) {
+ m_taskQueue->enqueueTask([this, promise = WTFMove(promise), error = result.error()]() mutable {
+ promise.reject(DOMCacheEngine::convertToExceptionAndLog(scriptExecutionContext(), error));
+ });
+ } else {
+ if (result.value().hadStorageError)
+ logConsolePersistencyError(scriptExecutionContext(), name);
- auto cache = DOMCache::create(*scriptExecutionContext(), String { name }, result.value().identifier, m_connection.copyRef());
+ m_taskQueue->enqueueTask([this, name, promise = WTFMove(promise), identifier = result.value().identifier]() mutable {
+ auto cache = DOMCache::create(*scriptExecutionContext(), String { name }, identifier, m_connection.copyRef());
promise.resolve(cache);
m_caches.append(WTFMove(cache));
- }
+ });
}
});
}
@@ -216,7 +230,9 @@
{
retrieveCaches([this, name, promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
if (exception) {
- promise.reject(WTFMove(exception.value()));
+ m_taskQueue->enqueueTask([promise = WTFMove(promise), exception = WTFMove(exception.value())]() mutable {
+ promise.reject(WTFMove(exception));
+ });
return;
}
doRemove(name, WTFMove(promise));
@@ -227,12 +243,14 @@
{
auto position = m_caches.findMatching([&](auto& item) { return item->name() == name; });
if (position == notFound) {
- promise.resolve(false);
+ m_taskQueue->enqueueTask([promise = WTFMove(promise)]() mutable {
+ promise.resolve(false);
+ });
return;
}
m_connection->remove(m_caches[position]->identifier(), [this, name, promise = WTFMove(promise), pendingActivity = makePendingActivity(*this)](const CacheIdentifierOrError& result) mutable {
- if (!m_isStopped) {
+ m_taskQueue->enqueueTask([this, name, promise = WTFMove(promise), result]() mutable {
if (!result.has_value())
promise.reject(DOMCacheEngine::convertToExceptionAndLog(scriptExecutionContext(), result.error()));
else {
@@ -240,21 +258,23 @@
logConsolePersistencyError(scriptExecutionContext(), name);
promise.resolve(!!result.value().identifier);
}
- }
+ });
});
}
void DOMCacheStorage::keys(KeysPromise&& promise)
{
retrieveCaches([this, promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
- if (exception) {
- promise.reject(WTFMove(exception.value()));
- return;
- }
+ m_taskQueue->enqueueTask([this, promise = WTFMove(promise), exception = WTFMove(exception)]() mutable {
+ if (exception) {
+ promise.reject(WTFMove(exception.value()));
+ return;
+ }
- promise.resolve(WTF::map(m_caches, [] (const auto& cache) {
- return cache->name();
- }));
+ promise.resolve(WTF::map(m_caches, [] (const auto& cache) {
+ return cache->name();
+ }));
+ });
});
}
@@ -270,7 +290,7 @@
bool DOMCacheStorage::canSuspendForDocumentSuspension() const
{
- return !hasPendingActivity();
+ return true;
}
} // namespace WebCore
diff --git a/Source/WebCore/Modules/cache/DOMCacheStorage.h b/Source/WebCore/Modules/cache/DOMCacheStorage.h
index a0c4618..9b6d9b1 100644
--- a/Source/WebCore/Modules/cache/DOMCacheStorage.h
+++ b/Source/WebCore/Modules/cache/DOMCacheStorage.h
@@ -32,9 +32,12 @@
namespace WebCore {
+class SuspendableTaskQueue;
+
class DOMCacheStorage : public RefCounted<DOMCacheStorage>, public ActiveDOMObject {
public:
static Ref<DOMCacheStorage> create(ScriptExecutionContext& context, Ref<CacheStorageConnection>&& connection) { return adoptRef(*new DOMCacheStorage(context, WTFMove(connection))); }
+ ~DOMCacheStorage();
using KeysPromise = DOMPromiseDeferred<IDLSequence<IDLDOMString>>;
@@ -62,6 +65,7 @@
Vector<Ref<DOMCache>> m_caches;
uint64_t m_updateCounter { 0 };
Ref<CacheStorageConnection> m_connection;
+ UniqueRef<SuspendableTaskQueue> m_taskQueue;
bool m_isStopped { false };
};
