⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 278274 in webkit


Ignore:
Timestamp:
May 31, 2021, 1:29:02 AM (5 years ago)
Author:
youenn@apple.com
Message:

Go to network in case fetch event is not yet responded when being destroyed instead of failing the load
​https://bugs.webkit.org/show_bug.cgi?id=226374
<rdar://78298472>

Reviewed by Alex Christensen.

Source/WebCore:

In case worker is terminated, instead of failing fetch events that are pending a response, we should go to the network.
This mirrors what is already done in ServiceWorkerFetchTask.

This can for instance happen in case a lot of fetches are done in parallel on the same service worker.
The service worker will do the fetch itself but given there are lots of fetches, some fetch might not start until other loads are complete.
This may trigger the fetch timeout which might then trigger terminating the worker.
We should probably revisit our fetch timeout policy now that we have added worker spin detection.

Test: http/wpt/service-workers/fetch-worker-terminate.https.html

  • testing/ServiceWorkerInternals.cpp:

(WebCore::ServiceWorkerInternals::terminate):
(WebCore::ServiceWorkerInternals::waitForFetchEventToFinish):

  • testing/ServiceWorkerInternals.h:
  • testing/ServiceWorkerInternals.idl:
  • workers/service/FetchEvent.cpp:

(WebCore::FetchEvent::~FetchEvent):
Update logging to only log the case where respondWith is called but fetch event is destroyed before processing the response.
Otherwise, we would log the case of respondWith being never called, which is happening often and leads to go to the network.
(WebCore::FetchEvent::processResponse):

  • workers/service/FetchEvent.h:
  • workers/service/context/ServiceWorkerFetch.cpp:

(WebCore::ServiceWorkerFetch::processResponse):

LayoutTests:

  • http/wpt/service-workers/fetch-worker-terminate-worker.js: Added.

(doTest):

  • http/wpt/service-workers/fetch-worker-terminate.https-expected.txt: Added.
  • http/wpt/service-workers/fetch-worker-terminate.https.html: Added.
Location:
trunk
Files:
3 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r278271 r278274  
     12021-05-31  Youenn Fablet  <youenn@apple.com>
     2
     3        Go to network in case fetch event is not yet responded when being destroyed instead of failing the load
     4        https://bugs.webkit.org/show_bug.cgi?id=226374
     5        <rdar://78298472>
     6
     7        Reviewed by Alex Christensen.
     8
     9        * http/wpt/service-workers/fetch-worker-terminate-worker.js: Added.
     10        (doTest):
     11        * http/wpt/service-workers/fetch-worker-terminate.https-expected.txt: Added.
     12        * http/wpt/service-workers/fetch-worker-terminate.https.html: Added.
     13
    1142021-05-30  Wenson Hsieh  <wenson_hsieh@apple.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r278271 r278274  
     12021-05-31  Youenn Fablet  <youenn@apple.com>
     2
     3        Go to network in case fetch event is not yet responded when being destroyed instead of failing the load
     4        https://bugs.webkit.org/show_bug.cgi?id=226374
     5        <rdar://78298472>
     6
     7        Reviewed by Alex Christensen.
     8
     9        In case worker is terminated, instead of failing fetch events that are pending a response, we should go to the network.
     10        This mirrors what is already done in ServiceWorkerFetchTask.
     11
     12        This can for instance happen in case a lot of fetches are done in parallel on the same service worker.
     13        The service worker will do the fetch itself but given there are lots of fetches, some fetch might not start until other loads are complete.
     14        This may trigger the fetch timeout which might then trigger terminating the worker.
     15        We should probably revisit our fetch timeout policy now that we have added worker spin detection.
     16
     17        Test: http/wpt/service-workers/fetch-worker-terminate.https.html
     18
     19        * testing/ServiceWorkerInternals.cpp:
     20        (WebCore::ServiceWorkerInternals::terminate):
     21        (WebCore::ServiceWorkerInternals::waitForFetchEventToFinish):
     22        * testing/ServiceWorkerInternals.h:
     23        * testing/ServiceWorkerInternals.idl:
     24        * workers/service/FetchEvent.cpp:
     25        (WebCore::FetchEvent::~FetchEvent):
     26        Update logging to only log the case where respondWith is called but fetch event is destroyed before processing the response.
     27        Otherwise, we would log the case of respondWith being never called, which is happening often and leads to go to the network.
     28        (WebCore::FetchEvent::processResponse):
     29        * workers/service/FetchEvent.h:
     30        * workers/service/context/ServiceWorkerFetch.cpp:
     31        (WebCore::ServiceWorkerFetch::processResponse):
     32
    1332021-05-30  Wenson Hsieh  <wenson_hsieh@apple.com>
    234

  • trunk/Source/WebCore/testing/ServiceWorkerInternals.cpp

    r277837 r278274  
    5151}
    5252
     53void ServiceWorkerInternals::terminate()
     54{
     55    callOnMainThread([identifier = m_identifier] () {
     56        SWContextManager::singleton().terminateWorker(identifier, Seconds::infinity(), [] { });
     57    });
     58}
     59
    5360void ServiceWorkerInternals::waitForFetchEventToFinish(FetchEvent& event, DOMPromiseDeferred<IDLInterface<FetchResponse>>&& promise)
    5461{
    5562    event.onResponse([promise = WTFMove(promise), event = makeRef(event)] (auto&& result) mutable {
    56         if (result.has_value())
    57             promise.resolve(WTFMove(result.value()));
    58         else
    59             promise.reject(TypeError, result.error().localizedDescription());
     63        if (!result.has_value()) {
     64            String description;
     65            if (auto& error = result.error())
     66                description = error->localizedDescription();
     67            promise.reject(TypeError, description);
     68            return;
     69        }
     70        promise.resolve(WTFMove(result.value()));
    6071    });
    6172}

  • trunk/Source/WebCore/testing/ServiceWorkerInternals.h

    r277837 r278274  
    4747
    4848    void setOnline(bool isOnline);
     49    void terminate();
     50
    4951    void waitForFetchEventToFinish(FetchEvent&, DOMPromiseDeferred<IDLInterface<FetchResponse>>&&);
    5052    Ref<FetchEvent> createBeingDispatchedFetchEvent(ScriptExecutionContext&);

  • trunk/Source/WebCore/testing/ServiceWorkerInternals.idl

    r277837 r278274  
    3131] interface ServiceWorkerInternals {
    3232    undefined setOnline(boolean isOnline);
     33    undefined terminate();
     34
    3335    Promise<FetchResponse> waitForFetchEventToFinish(FetchEvent event);
    3436    [CallWith=ScriptExecutionContext] FetchEvent createBeingDispatchedFetchEvent();

  • trunk/Source/WebCore/workers/service/FetchEvent.cpp

    r277864 r278274  
    5757{
    5858    if (auto callback = WTFMove(m_onResponse)) {
    59         RELEASE_LOG_ERROR(ServiceWorker, "Fetch event is destroyed without a response, respondWithEntered=%d, waitToRespond=%d, respondWithError=%d, respondPromise=%d", m_respondWithEntered, m_waitToRespond, m_respondWithError, !!m_respondPromise);
    60         callback(makeUnexpected(ResourceError { errorDomainWebKitServiceWorker, 0, m_request->url(), "Fetch event is destroyed."_s, ResourceError::Type::Cancellation }));
     59        RELEASE_LOG_ERROR_IF(m_respondWithEntered, ServiceWorker, "Fetch event is destroyed without a response, respondWithEntered=%d, waitToRespond=%d, respondWithError=%d, respondPromise=%d", m_respondWithEntered, m_waitToRespond, m_respondWithError, !!m_respondPromise);
     60        callback(makeUnexpected(std::optional<ResourceError> { }));
    6161    }
    6262}
    … …  
    107107}
    108108
    109 void FetchEvent::processResponse(Expected<Ref<FetchResponse>, ResourceError>&& result)
     109void FetchEvent::processResponse(Expected<Ref<FetchResponse>, std::optional<ResourceError>>&& result)
    110110{
    111111    m_respondPromise = nullptr;

  • trunk/Source/WebCore/workers/service/FetchEvent.h

    r250060 r278274  
    6060    ExceptionOr<void> respondWith(Ref<DOMPromise>&&);
    6161
    62     using ResponseCallback = CompletionHandler<void(Expected<Ref<FetchResponse>, ResourceError>&&)>;
     62    using ResponseCallback = CompletionHandler<void(Expected<Ref<FetchResponse>, std::optional<ResourceError>>&&)>;
    6363    WEBCORE_EXPORT void onResponse(ResponseCallback&&);
    6464
    … …  
    7676
    7777    void promiseIsSettled();
    78     void processResponse(Expected<Ref<FetchResponse>, ResourceError>&&);
     78    void processResponse(Expected<Ref<FetchResponse>, std::optional<ResourceError>>&&);
    7979    void respondWithError(ResourceError&&);
    8080

  • trunk/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp

    r278253 r278274  
    6666}
    6767
    68 static void processResponse(Ref<Client>&& client, Expected<Ref<FetchResponse>, ResourceError>&& result, FetchOptions::Mode mode, FetchOptions::Redirect redirect, const URL& requestURL, CertificateInfo&& certificateInfo)
     68static void processResponse(Ref<Client>&& client, Expected<Ref<FetchResponse>, std::optional<ResourceError>>&& result, FetchOptions::Mode mode, FetchOptions::Redirect redirect, const URL& requestURL, CertificateInfo&& certificateInfo)
    6969{
    7070    if (!result.has_value()) {
    71         client->didFail(result.error());
     71        auto& error = result.error();
     72        if (!error) {
     73            client->didNotHandle();
     74            return;
     75        }
     76        client->didFail(*error);
    7277        return;
    7378    }
Note: See TracChangeset for help on using the changeset viewer.