Bug 233471 - Preload navigation request if the service worker is not immediately ready to handle the navigation request fetch event
Summary: Preload navigation request if the service worker is not immediately ready to ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-24 03:39 PST by youenn fablet
Modified: 2021-11-30 02:49 PST (History)
8 users (show)

See Also:


Attachments
Patch (39.48 KB, patch)
2021-11-24 05:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (39.90 KB, patch)
2021-11-25 03:25 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (42.74 KB, patch)
2021-11-26 06:49 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (42.85 KB, patch)
2021-11-30 01:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-11-24 03:39:39 PST
Preload navigation request if the service worker is not immediately ready to handle the navigation request fetch event
Comment 1 Thomas Steiner 2021-11-24 04:20:24 PST
Is this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852?
Comment 2 youenn fablet 2021-11-24 05:16:29 PST
Created attachment 445090 [details]
Patch
Comment 3 youenn fablet 2021-11-24 05:19:03 PST
(In reply to Thomas Steiner from comment #1)
> Is this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=182852?

Not really, this is restricted to the case where the service worker is not running.
When service worker is running, going to the service worker is usually fast: added delay to the network is a few milliseconds. When service worker is not running, the delay is larger, depending on the size of the service worker script.
Comment 4 youenn fablet 2021-11-25 03:25:10 PST
Created attachment 445130 [details]
Patch
Comment 5 youenn fablet 2021-11-26 06:49:32 PST
Created attachment 445200 [details]
Patch
Comment 6 Chris Dumez 2021-11-29 13:17:02 PST
Comment on attachment 445200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445200&action=review

r=me with nits

> Source/WebKit/ChangeLog:10
> +        To optimize this code path, we preload the navigation request in case service worker ins not running or not yet activated.

typo: ins

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:82
> +    if (m_preloader) {

This logic comes up so many times, we may want to consider moving it to a cancelPreloadIfNecessary() function.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:345
> +    m_preloader->waitForResponse([weakThis = WeakPtr { this }, this]() {

WeakPtr { *this } is slightly better as it avoids a null check.

`()` is not needed.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:370
> +    m_preloader->waitForBody([weakThis = WeakPtr { this }, this](auto&& chunk, int length) {

WeakPtr { *this }

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:121
> +    bool m_isLoadingFromPreloader { false };

We may want to group the boolean data members together for better padding.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:77
> +ServiceWorkerNavigationPreloader::~ServiceWorkerNavigationPreloader()

= default;

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:92
> +    didReceiveResponse(ResourceResponse { entry.response() }, [body = RefPtr { entry.buffer() }, weakThis = WeakPtr { this }](auto) mutable {

WeakPtr { *this }

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:132
> +        auto cacheEntry = WTFMove(m_cacheEntry);

Not sure using a local variable improves things here.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:142
> +        auto callback = std::exchange(m_responseCallback, { });

This could go inside the condition:
```
if (auto callback = std::exchange(m_responseCallback, { }))
    callback();
```

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:174
> +        auto callback = std::exchange(m_responseCallback, { });

This could go inside the if condition.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h:52
> +    using ResponseCallback = Function<void()>;

Is this a Function instead of a CompletionHandler because it might not get called?
It feels like it could be a CompletionHandler otherwise.
Comment 7 youenn fablet 2021-11-30 01:07:17 PST
Thanks for the review, will update patch accordingly.

> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.h:52
> > +    using ResponseCallback = Function<void()>;
> 
> Is this a Function instead of a CompletionHandler because it might not get
> called?
> It feels like it could be a CompletionHandler otherwise.

Yes, if preloaded gets cancelled, it does not seem great to call this callback.
Comment 8 youenn fablet 2021-11-30 01:30:44 PST
Created attachment 445394 [details]
Patch for landing
Comment 9 EWS 2021-11-30 02:48:11 PST
Committed r286288 (244647@main): <https://commits.webkit.org/244647@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445394 [details].
Comment 10 Radar WebKit Bug Importer 2021-11-30 02:49:23 PST
<rdar://problem/85857877>