Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FetchEvent should not start its navigation preload response load if the preload was already used #4798

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Sep 28, 2022

757903c

FetchEvent should not start its navigation preload response load if the preload was already used
https://bugs.webkit.org/show_bug.cgi?id=245788
rdar://98144044

Reviewed by Alex Christensen.

Creating the navigation preload promise was scheduling a load which is hitting the network if the preload is no longer in network process.
This case happens if the navigation preload promise is actually used in respondWith.

To circumvent this issue, we do not trigger the load if the preload is used in respondWith.

Add logs to capture the fact that a load is not getting its expected preload.

Drive-by fixes to actually use the current preload request and not the loader original request.
This might change in case of redirections.

Covered by newly added test which mirrors what we were testing for non-exposed navigation preloads.

* LayoutTests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https-expected.txt: Added.
* LayoutTests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https.html: Added.
* LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-worker.js:
(event.event.request.url.includes):
* LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py:
(main):
* Source/WebCore/Modules/fetch/FetchResponse.cpp:
(WebCore::FetchResponse::markAsUsedForPreload):
(WebCore::FetchResponse::markAsDisturbed): Deleted.
* Source/WebCore/Modules/fetch/FetchResponse.h:
* Source/WebCore/workers/service/FetchEvent.cpp:
(WebCore::FetchEvent::navigationPreloadIsReady):
* Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:
(WebCore::ServiceWorkerFetch::processResponse):
* Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:
(WebKit::ServiceWorkerFetchTask::fromNavigationPreloader):
(WebKit::ServiceWorkerFetchTask::ServiceWorkerFetchTask):
(WebKit::ServiceWorkerFetchTask::loadBodyFromPreloader):

Canonical link: https://commits.webkit.org/254992@main

392700d

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios   πŸ›  mac   πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim   πŸ›  mac-debug βœ… πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios   πŸ§ͺ api-mac   πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim   πŸ§ͺ mac-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch-sim   πŸ§ͺ mac-wk2-stress

@youennf youennf self-assigned this Sep 28, 2022
@youennf youennf added Service Workers Component for Service Workers bugs. WebKit Nightly Build labels Sep 28, 2022
@youennf youennf force-pushed the eng/FetchEvent-should-not-start-its-navigation-preload-response-load-if-the-preload-was-already-used branch from b971468 to 392700d Compare September 29, 2022 08:24
@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label Sep 29, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 254992@main (757903c): https://commits.webkit.org/254992@main

Reviewed commits have been landed. Closing PR #4798 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/FetchEvent-should-not-start-its-navigation-preload-response-load-if-the-preload-was-already-used branch from 392700d to 757903c Compare September 29, 2022 09:42
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Service Workers Component for Service Workers bugs.
Projects
None yet
4 participants