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

Duplicate requests due to speculative parsing with active service worker #21729

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Dec 13, 2023

c68a38f

Duplicate requests due to speculative parsing with active service worker
https://bugs.webkit.org/show_bug.cgi?id=250454
rdar://104377727

Reviewed by Chris Dumez.

In case a service worker is located in the same process as a controlled document, they both share the same memory cache.
Before the patch, in case of preloads triggered byt he speculative parser and the preloads are not cacheable, the following would happen:
- Trigger a preload, which gets registered in the memory cache.
- Let the service worker handle the preload, which triggers a fetch of the same resource.
- This fetch will remove the preload resource from the memory cache and do the actual network load.
- The real load of the HTML resource kicks in and tries to load the resource from the memory cache.
- Given the resource is not a preload and is not reusable according the HTTP headers, a new load is triggered.

This makes it so that two loads are happening for the same resource (one for the preload and one for the actual load).
To resolve the issue, we do not add the service worker fetch resource in the memory cache in the following case:
- There is an existing cached resource, which is a preload, and not a service worker request.
- The new request is a service worker request.

This ensures that we keep the preload cached resource in the memory cache which allows reuse when the actual load happens.

* LayoutTests/http/wpt/service-workers/resources/serviceworker-and-preloads-iframe.html: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-img.png: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-img.png.headers: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-script.js: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-script.js.headers: Added.
* LayoutTests/http/wpt/service-workers/serviceworker-and-preloads-expected.txt: Added.
* LayoutTests/http/wpt/service-workers/serviceworker-and-preloads-worker.js: Added.
* LayoutTests/http/wpt/service-workers/serviceworker-and-preloads.html: Added.
* Source/WebCore/loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::updateCachedResourceWithCurrentRequest):
(WebCore::computeShouldAddToMemoryCache):
(WebCore::CachedResourceLoader::requestResource):
(WebCore::CachedResourceLoader::loadResource):
* Source/WebCore/loader/cache/CachedResourceLoader.h:

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

09c7f96

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

@youennf youennf requested a review from cdumez as a code owner December 13, 2023 13:50
@youennf youennf self-assigned this Dec 13, 2023
@youennf youennf added the Service Workers Component for Service Workers bugs. label Dec 13, 2023
@youennf youennf force-pushed the eng/Duplicate-requests-due-to-speculative-parsing-with-active-service-worker branch from e9cc9b8 to e4bd6c7 Compare December 13, 2023 15:06
@youennf youennf force-pushed the eng/Duplicate-requests-due-to-speculative-parsing-with-active-service-worker branch from e4bd6c7 to 09c7f96 Compare December 13, 2023 17:16
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 13, 2023
@youennf youennf added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Dec 14, 2023
https://bugs.webkit.org/show_bug.cgi?id=250454
rdar://104377727

Reviewed by Chris Dumez.

In case a service worker is located in the same process as a controlled document, they both share the same memory cache.
Before the patch, in case of preloads triggered byt he speculative parser and the preloads are not cacheable, the following would happen:
- Trigger a preload, which gets registered in the memory cache.
- Let the service worker handle the preload, which triggers a fetch of the same resource.
- This fetch will remove the preload resource from the memory cache and do the actual network load.
- The real load of the HTML resource kicks in and tries to load the resource from the memory cache.
- Given the resource is not a preload and is not reusable according the HTTP headers, a new load is triggered.

This makes it so that two loads are happening for the same resource (one for the preload and one for the actual load).
To resolve the issue, we do not add the service worker fetch resource in the memory cache in the following case:
- There is an existing cached resource, which is a preload, and not a service worker request.
- The new request is a service worker request.

This ensures that we keep the preload cached resource in the memory cache which allows reuse when the actual load happens.

* LayoutTests/http/wpt/service-workers/resources/serviceworker-and-preloads-iframe.html: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-img.png: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-img.png.headers: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-script.js: Added.
* LayoutTests/http/wpt/service-workers/resources/serviceworker-preload-script.js.headers: Added.
* LayoutTests/http/wpt/service-workers/serviceworker-and-preloads-expected.txt: Added.
* LayoutTests/http/wpt/service-workers/serviceworker-and-preloads-worker.js: Added.
* LayoutTests/http/wpt/service-workers/serviceworker-and-preloads.html: Added.
* Source/WebCore/loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::updateCachedResourceWithCurrentRequest):
(WebCore::computeShouldAddToMemoryCache):
(WebCore::CachedResourceLoader::requestResource):
(WebCore::CachedResourceLoader::loadResource):
* Source/WebCore/loader/cache/CachedResourceLoader.h:

Canonical link: https://commits.webkit.org/272024@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Duplicate-requests-due-to-speculative-parsing-with-active-service-worker branch from 09c7f96 to c68a38f Compare December 14, 2023 09:34
@webkit-commit-queue
Copy link
Collaborator

Committed 272024@main (c68a38f): https://commits.webkit.org/272024@main

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

@webkit-commit-queue webkit-commit-queue merged commit c68a38f into WebKit:main Dec 14, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 14, 2023
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
5 participants