Skip to content

Commit f76b24a

Browse files
cdumezcdumez
committed
DOMCache should not prevent pages from entering the back/forward cache
https://bugs.webkit.org/show_bug.cgi?id=202590 Reviewed by Youenn Fablet. Source/WebCore: Allow DOMCache to suspend in the back/forward cache unconditionally, by appending all promise resolutions to a SuspendableTaskQueue. This guarantees that any asynchronous promise resolution is delayed while in the page cache, until the page gets resumed. Test: http/tests/navigation/page-cache-domcache-pending-promise.html * Modules/cache/DOMCache.cpp: (WebCore::DOMCache::DOMCache): (WebCore::DOMCache::match): (WebCore::DOMCache::matchAll): (WebCore::DOMCache::addAll): (WebCore::DOMCache::putWithResponseData): (WebCore::DOMCache::put): (WebCore::DOMCache::remove): (WebCore::DOMCache::keys): (WebCore::DOMCache::canSuspendForDocumentSuspension const): * Modules/cache/DOMCache.h: LayoutTests: Add layout test coverage. * TestExpectations: * http/tests/navigation/page-cache-domcache-pending-promise-expected.txt: Added. * http/tests/navigation/page-cache-domcache-pending-promise.html: Added. Canonical link: https://commits.webkit.org/216115@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250786 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent af90c50 commit f76b24a

11 files changed

Lines changed: 186 additions & 47 deletions

File tree

‎LayoutTests/ChangeLog‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,16 @@
1+
2019-10-07 Chris Dumez <[email protected]>
2+
3+
DOMCache should not prevent pages from entering the back/forward cache
4+
https://bugs.webkit.org/show_bug.cgi?id=202590
5+
6+
Reviewed by Youenn Fablet.
7+
8+
Add layout test coverage.
9+
10+
* TestExpectations:
11+
* http/tests/navigation/page-cache-domcache-pending-promise-expected.txt: Added.
12+
* http/tests/navigation/page-cache-domcache-pending-promise.html: Added.
13+
114
2019-10-07 Alexey Shvayka <[email protected]>
215

316
Sync web-platform-tests/css/css-overflow from upstream

‎LayoutTests/TestExpectations‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,8 @@ imported/w3c/web-platform-tests/workers/modules/dedicated-worker-options-credent
454454
imported/w3c/web-platform-tests/workers/constructors/SharedWorker [ Skip ]
455455
imported/w3c/web-platform-tests/workers/SharedWorker_blobUrl.html [ Skip ]
456456

457+
http/tests/navigation/page-cache-domcache-pending-promise.html [ DumpJSConsoleLogInStdErr ]
458+
457459
imported/w3c/web-platform-tests/beacon/beacon-basic-blob.html [ DumpJSConsoleLogInStdErr ]
458460
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-redirect.any.worker.html [ DumpJSConsoleLogInStdErr ]
459461
imported/w3c/web-platform-tests/xhr/access-control-and-redirects.htm [ DumpJSConsoleLogInStdErr ]

‎LayoutTests/http/tests/navigation/page-cache-domcache-pending-promise-expected.txt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
Tests that a page with pending DOMCache activity goes into the page cache.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
pageshow - not from cache
7+
pagehide - entering cache
8+
pageshow - from cache
9+
PASS Page was restored from Page Cache
10+
PASS Cache.add() succeeded
11+
PASS !!restoredFromPageCache is true
12+
PASS successfullyParsed is true
13+
14+
TEST COMPLETE
15+

‎LayoutTests/http/tests/navigation/page-cache-domcache-pending-promise.html‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
<!-- webkit-test-runner [ enablePageCache=true ] -->
2+
<!DOCTYPE html>
3+
<html>
4+
<head>
5+
<script src="/js-test-resources/js-test.js"></script>
6+
</head>
7+
<body>
8+
<script>
9+
description('Tests that a page with pending DOMCache activity goes into the page cache.');
10+
jsTestIsAsync = true;
11+
let restoredFromPageCache = false;
12+
13+
window.addEventListener("pageshow", function(event) {
14+
debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
15+
if (event.persisted) {
16+
testPassed("Page was restored from Page Cache");
17+
restoredFromPageCache = true;
18+
}
19+
});
20+
21+
function tryCache()
22+
{
23+
cache.add('resources/blank.txt').then(() => {
24+
testPassed("Cache.add() succeeded");
25+
shouldBeTrue("!!restoredFromPageCache");
26+
finishJSTest();
27+
}, (e) => {
28+
tryCache();
29+
});
30+
}
31+
32+
window.addEventListener("pagehide", function(event) {
33+
debug("pagehide - " + (event.persisted ? "" : "not ") + "entering cache");
34+
if (!event.persisted) {
35+
testFailed("Page failed to enter the Page Cache");
36+
finishJSTest();
37+
}
38+
39+
tryCache();
40+
});
41+
42+
onload = () => {
43+
setTimeout(() => {
44+
caches.open('test').then((_cache) => {
45+
cache = _cache;
46+
testLink.click();
47+
});
48+
}, 0);
49+
}
50+
</script>
51+
<a id="testLink" href="resources/page-cache-helper.html" style="display: none">Link</a>
52+
</body>
53+
</html>

‎LayoutTests/platform/ios-wk1/TestExpectations‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ imported/w3c/web-platform-tests/server-timing/service_worker_idl.html [ Skip ]
1111
http/wpt/service-workers [ Skip ]
1212
http/wpt/cache-storage [ Skip ]
1313
http/tests/cache-storage [ Skip ]
14+
http/tests/navigation/page-cache-domcache-pending-promise.html [ Skip ]
1415
imported/w3c/web-platform-tests/fetch/api/request/destination [ Skip ]
1516
imported/w3c/web-platform-tests/fetch/cross-origin-resource-policy [ Skip ]
1617

‎LayoutTests/platform/mac-wk1/TestExpectations‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ http/wpt/loading/redirect-headers.html [ Skip ]
267267
# No service worker implementation for WK1
268268
http/tests/appcache/main-resource-redirect-with-sw.html [ Skip ]
269269
http/tests/cache-storage [ Skip ]
270+
http/tests/navigation/page-cache-domcache-pending-promise.html [ Skip ]
270271
http/tests/cookies/same-site/fetch-in-cross-origin-service-worker.html [ Skip ]
271272
http/tests/cookies/same-site/fetch-in-same-origin-service-worker.html [ Skip ]
272273
http/wpt/cache-storage [ Skip ]

‎LayoutTests/platform/win/TestExpectations‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3798,6 +3798,7 @@ webkit.org/b/93589 svg/dom/SVGScriptElement/script-change-externalResourcesRequi
37983798
# No service worker implementation for WK1
37993799
http/tests/appcache/main-resource-redirect-with-sw.html [ Skip ]
38003800
http/tests/cache-storage [ Skip ]
3801+
http/tests/navigation/page-cache-domcache-pending-promise.html [ Skip ]
38013802
http/tests/inspector/network/resource-response-service-worker.html [ Skip ]
38023803
http/tests/workers/service [ Skip ]
38033804
http/wpt/cache-storage [ Skip ]

‎Source/WebCore/ChangeLog‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,29 @@
1+
2019-10-07 Chris Dumez <[email protected]>
2+
3+
DOMCache should not prevent pages from entering the back/forward cache
4+
https://bugs.webkit.org/show_bug.cgi?id=202590
5+
6+
Reviewed by Youenn Fablet.
7+
8+
Allow DOMCache to suspend in the back/forward cache unconditionally, by
9+
appending all promise resolutions to a SuspendableTaskQueue. This guarantees
10+
that any asynchronous promise resolution is delayed while in the page cache,
11+
until the page gets resumed.
12+
13+
Test: http/tests/navigation/page-cache-domcache-pending-promise.html
14+
15+
* Modules/cache/DOMCache.cpp:
16+
(WebCore::DOMCache::DOMCache):
17+
(WebCore::DOMCache::match):
18+
(WebCore::DOMCache::matchAll):
19+
(WebCore::DOMCache::addAll):
20+
(WebCore::DOMCache::putWithResponseData):
21+
(WebCore::DOMCache::put):
22+
(WebCore::DOMCache::remove):
23+
(WebCore::DOMCache::keys):
24+
(WebCore::DOMCache::canSuspendForDocumentSuspension const):
25+
* Modules/cache/DOMCache.h:
26+
127
2019-10-07 Chris Dumez <[email protected]>
228

329
PendingImageBitmap should not prevent entering the back/forward cache

‎Source/WebCore/Modules/cache/DOMCache.cpp‎

Lines changed: 67 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include "JSFetchResponse.h"
3434
#include "ReadableStreamChunk.h"
3535
#include "ScriptExecutionContext.h"
36+
#include "SuspendableTaskQueue.h"
3637
#include <wtf/CompletionHandler.h>
3738
#include <wtf/URL.h>
3839

@@ -44,6 +45,7 @@ DOMCache::DOMCache(ScriptExecutionContext& context, String&& name, uint64_t iden
4445
, m_name(WTFMove(name))
4546
, m_identifier(identifier)
4647
, m_connection(WTFMove(connection))
48+
, m_taskQueue(SuspendableTaskQueue::create(&context))
4749
{
4850
suspendIfNeeded();
4951
m_connection->reference(m_identifier);
@@ -57,16 +59,18 @@ DOMCache::~DOMCache()
5759

5860
void DOMCache::match(RequestInfo&& info, CacheQueryOptions&& options, Ref<DeferredPromise>&& promise)
5961
{
60-
doMatch(WTFMove(info), WTFMove(options), [promise = WTFMove(promise)](ExceptionOr<FetchResponse*>&& result) mutable {
61-
if (result.hasException()) {
62-
promise->reject(result.releaseException());
63-
return;
64-
}
65-
if (!result.returnValue()) {
66-
promise->resolve();
67-
return;
68-
}
69-
promise->resolve<IDLInterface<FetchResponse>>(*result.returnValue());
62+
doMatch(WTFMove(info), WTFMove(options), [this, protectedThis = makeRef(*this), promise = WTFMove(promise)](ExceptionOr<RefPtr<FetchResponse>>&& result) mutable {
63+
m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
64+
if (result.hasException()) {
65+
promise->reject(result.releaseException());
66+
return;
67+
}
68+
if (!result.returnValue()) {
69+
promise->resolve();
70+
return;
71+
}
72+
promise->resolve<IDLInterface<FetchResponse>>(*result.returnValue());
73+
});
7074
});
7175
}
7276

@@ -91,7 +95,7 @@ void DOMCache::doMatch(RequestInfo&& info, CacheQueryOptions&& options, MatchCal
9195
callback(nullptr);
9296
return;
9397
}
94-
callback(result.returnValue()[0].response->clone(*scriptExecutionContext()).releaseReturnValue().ptr());
98+
callback(RefPtr<FetchResponse>(result.returnValue()[0].response->clone(*scriptExecutionContext()).releaseReturnValue()));
9599
});
96100
}
97101

@@ -120,20 +124,24 @@ void DOMCache::matchAll(Optional<RequestInfo>&& info, CacheQueryOptions&& option
120124

121125
if (!request) {
122126
retrieveRecords(URL { }, [this, promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
123-
if (exception) {
124-
promise.reject(WTFMove(exception.value()));
125-
return;
126-
}
127-
promise.resolve(cloneResponses(m_records));
127+
m_taskQueue->enqueueTask([this, promise = WTFMove(promise), exception = WTFMove(exception)]() mutable {
128+
if (exception) {
129+
promise.reject(WTFMove(exception.value()));
130+
return;
131+
}
132+
promise.resolve(cloneResponses(m_records));
133+
});
128134
});
129135
return;
130136
}
131137
queryCache(request.releaseNonNull(), WTFMove(options), [this, promise = WTFMove(promise)](ExceptionOr<Vector<CacheStorageRecord>>&& result) mutable {
132-
if (result.hasException()) {
133-
promise.reject(result.releaseException());
134-
return;
135-
}
136-
promise.resolve(cloneResponses(result.releaseReturnValue()));
138+
m_taskQueue->enqueueTask([this, promise = WTFMove(promise), result = WTFMove(result)]() mutable {
139+
if (result.hasException()) {
140+
promise.reject(result.releaseException());
141+
return;
142+
}
143+
promise.resolve(cloneResponses(result.releaseReturnValue()));
144+
});
137145
});
138146
}
139147

@@ -235,11 +243,15 @@ void DOMCache::addAll(Vector<RequestInfo>&& infos, DOMPromiseDeferred<void>&& pr
235243

236244
auto taskHandler = FetchTasksHandler::create(*this, [this, promise = WTFMove(promise)](ExceptionOr<Vector<Record>>&& result) mutable {
237245
if (result.hasException()) {
238-
promise.reject(result.releaseException());
246+
m_taskQueue->enqueueTask([promise = WTFMove(promise), exception = result.releaseException()]() mutable {
247+
promise.reject(WTFMove(exception));
248+
});
239249
return;
240250
}
241-
batchPutOperation(result.releaseReturnValue(), [promise = WTFMove(promise)](ExceptionOr<void>&& result) mutable {
242-
promise.settle(WTFMove(result));
251+
batchPutOperation(result.releaseReturnValue(), [this, protectedThis = makeRef(*this), promise = WTFMove(promise)](ExceptionOr<void>&& result) mutable {
252+
m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
253+
promise.settle(WTFMove(result));
254+
});
243255
});
244256
});
245257

@@ -302,15 +314,19 @@ void DOMCache::addAll(Vector<RequestInfo>&& infos, DOMPromiseDeferred<void>&& pr
302314
void DOMCache::putWithResponseData(DOMPromiseDeferred<void>&& promise, Ref<FetchRequest>&& request, Ref<FetchResponse>&& response, ExceptionOr<RefPtr<SharedBuffer>>&& responseBody)
303315
{
304316
if (responseBody.hasException()) {
305-
promise.reject(responseBody.releaseException());
317+
m_taskQueue->enqueueTask([promise = WTFMove(promise), exception = responseBody.releaseException()]() mutable {
318+
promise.reject(WTFMove(exception));
319+
});
306320
return;
307321
}
308322

309323
DOMCacheEngine::ResponseBody body;
310324
if (auto buffer = responseBody.releaseReturnValue())
311325
body = buffer.releaseNonNull();
312-
batchPutOperation(request.get(), response.get(), WTFMove(body), [promise = WTFMove(promise)](ExceptionOr<void>&& result) mutable {
313-
promise.settle(WTFMove(result));
326+
batchPutOperation(request.get(), response.get(), WTFMove(body), [this, protectedThis = makeRef(*this), promise = WTFMove(promise)](ExceptionOr<void>&& result) mutable {
327+
m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
328+
promise.settle(WTFMove(result));
329+
});
314330
});
315331
}
316332

@@ -373,8 +389,10 @@ void DOMCache::put(RequestInfo&& info, Ref<FetchResponse>&& response, DOMPromise
373389
return;
374390
}
375391

376-
batchPutOperation(request.get(), response.get(), response->consumeBody(), [promise = WTFMove(promise)](ExceptionOr<void>&& result) mutable {
377-
promise.settle(WTFMove(result));
392+
batchPutOperation(request.get(), response.get(), response->consumeBody(), [this, protectedThis = makeRef(*this), promise = WTFMove(promise)](ExceptionOr<void>&& result) mutable {
393+
m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
394+
promise.settle(WTFMove(result));
395+
});
378396
});
379397
}
380398

@@ -389,8 +407,10 @@ void DOMCache::remove(RequestInfo&& info, CacheQueryOptions&& options, DOMPromis
389407
return;
390408
}
391409

392-
batchDeleteOperation(requestOrException.releaseReturnValue(), WTFMove(options), [promise = WTFMove(promise)](ExceptionOr<bool>&& result) mutable {
393-
promise.settle(WTFMove(result));
410+
batchDeleteOperation(requestOrException.releaseReturnValue(), WTFMove(options), [this, protectedThis = makeRef(*this), promise = WTFMove(promise)](ExceptionOr<bool>&& result) mutable {
411+
m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
412+
promise.settle(WTFMove(result));
413+
});
394414
});
395415
}
396416

@@ -416,22 +436,26 @@ void DOMCache::keys(Optional<RequestInfo>&& info, CacheQueryOptions&& options, K
416436

417437
if (!request) {
418438
retrieveRecords(URL { }, [this, promise = WTFMove(promise)](Optional<Exception>&& exception) mutable {
419-
if (exception) {
420-
promise.reject(WTFMove(exception.value()));
421-
return;
422-
}
423-
promise.resolve(WTF::map(m_records, copyRequestRef));
439+
m_taskQueue->enqueueTask([this, promise = WTFMove(promise), exception = WTFMove(exception)]() mutable {
440+
if (exception) {
441+
promise.reject(WTFMove(exception.value()));
442+
return;
443+
}
444+
promise.resolve(WTF::map(m_records, copyRequestRef));
445+
});
424446
});
425447
return;
426448
}
427449

428-
queryCache(request.releaseNonNull(), WTFMove(options), [promise = WTFMove(promise)](ExceptionOr<Vector<CacheStorageRecord>>&& result) mutable {
429-
if (result.hasException()) {
430-
promise.reject(result.releaseException());
431-
return;
432-
}
450+
queryCache(request.releaseNonNull(), WTFMove(options), [this, protectedThis = makeRef(*this), promise = WTFMove(promise)](auto&& result) mutable {
451+
m_taskQueue->enqueueTask([promise = WTFMove(promise), result = WTFMove(result)]() mutable {
452+
if (result.hasException()) {
453+
promise.reject(result.releaseException());
454+
return;
455+
}
433456

434-
promise.resolve(WTF::map(result.releaseReturnValue(), copyRequestRef));
457+
promise.resolve(WTF::map(result.releaseReturnValue(), copyRequestRef));
458+
});
435459
});
436460
}
437461

@@ -586,7 +610,7 @@ const char* DOMCache::activeDOMObjectName() const
586610

587611
bool DOMCache::canSuspendForDocumentSuspension() const
588612
{
589-
return m_records.isEmpty() && !hasPendingActivity();
613+
return true;
590614
}
591615

592616

‎Source/WebCore/Modules/cache/DOMCache.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,12 @@
2828
#include "ActiveDOMObject.h"
2929
#include "CacheStorageConnection.h"
3030
#include "CacheStorageRecord.h"
31+
#include <wtf/UniqueRef.h>
3132

3233
namespace WebCore {
3334

3435
class ScriptExecutionContext;
36+
class SuspendableTaskQueue;
3537

3638
class DOMCache final : public RefCounted<DOMCache>, public ActiveDOMObject {
3739
public:
@@ -56,7 +58,7 @@ class DOMCache final : public RefCounted<DOMCache>, public ActiveDOMObject {
5658
const String& name() const { return m_name; }
5759
uint64_t identifier() const { return m_identifier; }
5860

59-
using MatchCallback = WTF::Function<void(ExceptionOr<FetchResponse*>)>;
61+
using MatchCallback = Function<void(ExceptionOr<RefPtr<FetchResponse>>)>;
6062
void doMatch(RequestInfo&&, CacheQueryOptions&&, MatchCallback&&);
6163

6264
CacheStorageConnection& connection() { return m_connection.get(); }
@@ -90,6 +92,7 @@ class DOMCache final : public RefCounted<DOMCache>, public ActiveDOMObject {
9092

9193
Vector<CacheStorageRecord> m_records;
9294
bool m_isStopped { false };
95+
UniqueRef<SuspendableTaskQueue> m_taskQueue;
9396
};
9497

9598
} // namespace WebCore

0 commit comments

Comments
 (0)