Changeset 289533 in webkit

Timestamp:

Feb 10, 2022 7:45:10 AM (5 months ago)

Author:

youenn@apple.com

Message:

Settling of a fetch promise should be delayed in case page is entering page cache
​https://bugs.webkit.org/show_bug.cgi?id=236292
<rdar://88452971>

Reviewed by Chris Dumez.

Source/WebCore:

Make sure to enqueue a task before resolving fetch promise as otherwise, page might continue running JavaScript.
Do this for worker as well for good measure.
We move signal aborted checks to two clients to handle rejecting fetch promise synchronously.

Test: http/tests/navigation/fetch-pagecache.html

• Modules/cache/DOMCache.cpp:
• Modules/fetch/FetchResponse.cpp:
• Modules/fetch/FetchResponse.h:
• Modules/fetch/WindowOrWorkerGlobalScopeFetch.cpp:
• workers/service/FetchEvent.cpp:

LayoutTests:

• http/tests/navigation/fetch-pagecache-expected.txt: Added.
• http/tests/navigation/fetch-pagecache.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/navigation/fetch-pagecache-expected.txt (added)
• LayoutTests/http/tests/navigation/fetch-pagecache.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/Modules/cache/DOMCache.cpp (modified) (3 diffs)
• Source/WebCore/Modules/fetch/FetchResponse.cpp (modified) (2 diffs)
• Source/WebCore/Modules/fetch/FetchResponse.h (modified) (1 diff)
• Source/WebCore/Modules/fetch/WindowOrWorkerGlobalScopeFetch.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2022-02-10  Youenn Fablet  <youenn@apple.com>
+
+        Settling of a fetch promise should be delayed in case page is entering page cache
+        https://bugs.webkit.org/show_bug.cgi?id=236292
+        <rdar://88452971>
+
+        Reviewed by Chris Dumez.
+
+        * http/tests/navigation/fetch-pagecache-expected.txt: Added.
+        * http/tests/navigation/fetch-pagecache.html: Added.
+
 2022-02-10  Antti Koivisto  <antti@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-02-10  Youenn Fablet  <youenn@apple.com>
+
+        Settling of a fetch promise should be delayed in case page is entering page cache
+        https://bugs.webkit.org/show_bug.cgi?id=236292
+        <rdar://88452971>
+
+        Reviewed by Chris Dumez.
+
+        Make sure to enqueue a task before resolving fetch promise as otherwise, page might continue running JavaScript.
+        Do this for worker as well for good measure.
+        We move signal aborted checks to two clients to handle rejecting fetch promise synchronously.
+
+        Test: http/tests/navigation/fetch-pagecache.html
+
+        * Modules/cache/DOMCache.cpp:
+        * Modules/fetch/FetchResponse.cpp:
+        * Modules/fetch/FetchResponse.h:
+        * Modules/fetch/WindowOrWorkerGlobalScopeFetch.cpp:
+        * workers/service/FetchEvent.cpp:
+
 2022-02-10  Chris Dumez  <cdumez@apple.com>
 

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

@@ -260 +260 @@
     for (auto& request : requests) {
         auto& requestReference = request.get();
-        FetchResponse::fetch(*scriptExecutionContext(), requestReference, [this, request = WTFMove(request), taskHandler](ExceptionOr<FetchResponse&>&& result) mutable {
+        if (requestReference.signal().aborted()) {
+            taskHandler->error(Exception { AbortError, "Request signal is aborted"_s });
+            return;
+        }
+        FetchResponse::fetch(*scriptExecutionContext(), requestReference, [this, request = WTFMove(request), taskHandler](auto&& result) mutable {
 
             if (taskHandler->isDone())
@@ -270 +274 @@
             }
 
-            auto& response = result.releaseReturnValue();
+            auto protectedResponse = result.releaseReturnValue();
+            auto& response = protectedResponse.get();
 
             if (!response.ok()) {
@@ -296 +301 @@
             size_t recordPosition = taskHandler->addRecord(toConnectionRecord(request.get(), response, nullptr));
 
-            response.consumeBodyReceivedByChunk([taskHandler = WTFMove(taskHandler), recordPosition, data = SharedBufferBuilder(), response = Ref { response }] (auto&& result) mutable {
+            response.consumeBodyReceivedByChunk([taskHandler = WTFMove(taskHandler), recordPosition, data = SharedBufferBuilder(), response = WTFMove(protectedResponse)] (auto&& result) mutable {
                 if (taskHandler->isDone())
                     return;

trunk/Source/WebCore/Modules/fetch/FetchResponse.cpp

@@ -238 +238 @@
 void FetchResponse::fetch(ScriptExecutionContext& context, FetchRequest& request, NotificationCallback&& responseCallback, const String& initiator)
 {
-    if (request.signal().aborted()) {
-        responseCallback(Exception { AbortError, "Request signal is aborted"_s });
-        // FIXME: Cancel request body if it is a stream.
-        return;
-    }
-
     if (request.isReadableStreamBody()) {
         responseCallback(Exception { NotSupportedError, "ReadableStream uploading is not supported"_s });
@@ -356 +350 @@
 
     if (auto responseCallback = WTFMove(m_responseCallback))
-        responseCallback(m_response);
+        responseCallback(Ref { m_response });
 }
 

trunk/Source/WebCore/Modules/fetch/FetchResponse.h

@@ -64 +64 @@
     static ExceptionOr<Ref<FetchResponse>> redirect(ScriptExecutionContext&, const String& url, int status);
 
-    using NotificationCallback = Function<void(ExceptionOr<FetchResponse&>&&)>;
+    using NotificationCallback = Function<void(ExceptionOr<Ref<FetchResponse>>&&)>;
     static void fetch(ScriptExecutionContext&, FetchRequest&, NotificationCallback&&, const String& initiator);
 

trunk/Source/WebCore/Modules/fetch/WindowOrWorkerGlobalScopeFetch.cpp

@@ -30 +30 @@
 #include "DOMWindow.h"
 #include "Document.h"
+#include "EventLoop.h"
 #include "FetchResponse.h"
 #include "JSDOMPromiseDeferred.h"
@@ -40 +41 @@
 using FetchResponsePromise = DOMPromiseDeferred<IDLInterface<FetchResponse>>;
 
-void WindowOrWorkerGlobalScopeFetch::fetch(DOMWindow& window, FetchRequest::Info&& input, FetchRequest::Init&& init, Ref<DeferredPromise>&& deferred)
+// https://fetch.spec.whatwg.org/#dom-global-fetch
+static void doFetch(ScriptExecutionContext& scope, FetchRequest::Info&& input, FetchRequest::Init&& init, FetchResponsePromise&& promise)
 {
-    FetchResponsePromise promise = WTFMove(deferred);
-
-    auto* document = window.document();
-    if (!document) {
-        promise.reject(InvalidStateError);
+    auto requestOrException = FetchRequest::create(scope, WTFMove(input), WTFMove(init));
+    if (requestOrException.hasException()) {
+        promise.reject(requestOrException.releaseException());
         return;
     }
 
-    auto request = FetchRequest::create(*document, WTFMove(input), WTFMove(init));
-    if (request.hasException()) {
-        promise.reject(request.releaseException());
+    auto request = requestOrException.releaseReturnValue();
+    if (request->signal().aborted()) {
+        promise.reject(Exception { AbortError, "Request signal is aborted"_s });
         return;
     }
 
-    FetchResponse::fetch(*document, request.releaseReturnValue(), [promise = WTFMove(promise), userGestureToken = UserGestureIndicator::currentUserGesture()](ExceptionOr<FetchResponse&>&& result) mutable {
-        if (!userGestureToken || userGestureToken->hasExpired(UserGestureToken::maximumIntervalForUserGestureForwardingForFetch()) || !userGestureToken->processingUserGesture()) {
+    FetchResponse::fetch(scope, request.get(), [promise = WTFMove(promise), scope = Ref { scope }](auto&& result) mutable {
+        scope->eventLoop().queueTask(TaskSource::Networking, [promise = WTFMove(promise), result = WTFMove(result)]() mutable {
             promise.settle(WTFMove(result));
-            return;
-        }
-
-        UserGestureIndicator gestureIndicator(userGestureToken, UserGestureToken::GestureScope::MediaOnly, UserGestureToken::IsPropagatedFromFetch::Yes);
-        promise.settle(WTFMove(result));
+        });
     }, cachedResourceRequestInitiators().fetch);
 }
 
-void WindowOrWorkerGlobalScopeFetch::fetch(WorkerGlobalScope& scope, FetchRequest::Info&& input, FetchRequest::Init&& init, Ref<DeferredPromise>&& deferred)
+void WindowOrWorkerGlobalScopeFetch::fetch(DOMWindow& window, FetchRequest::Info&& input, FetchRequest::Init&& init, Ref<DeferredPromise>&& promise)
 {
-    FetchResponsePromise promise = WTFMove(deferred);
-
-    auto request = FetchRequest::create(scope, WTFMove(input), WTFMove(init));
-    if (request.hasException()) {
-        promise.reject(request.releaseException());
+    auto* document = window.document();
+    if (!document) {
+        promise->reject(InvalidStateError);
         return;
     }
+    doFetch(*document, WTFMove(input), WTFMove(init), WTFMove(promise));
+}
 
-    FetchResponse::fetch(scope, request.releaseReturnValue().get(), [promise = WTFMove(promise)](ExceptionOr<FetchResponse&>&& result) mutable {
-        promise.settle(WTFMove(result));
-    }, cachedResourceRequestInitiators().fetch);
+void WindowOrWorkerGlobalScopeFetch::fetch(WorkerGlobalScope& scope, FetchRequest::Info&& input, FetchRequest::Init&& init, Ref<DeferredPromise>&& promise)
+{
+    doFetch(scope, WTFMove(input), WTFMove(init), WTFMove(promise));
 }