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

Dedicated worker and shared worker global scope should use the response URL #6250

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Nov 8, 2022

99d5398

Dedicated worker and shared worker global scope should use the response URL
https://bugs.webkit.org/show_bug.cgi?id=247615
rdar://problem/102088086

Reviewed by Chris Dumez.

As per https://html.spec.whatwg.org/multipage/workers.html#run-a-worker step 14.3.1, we should use the response URL.
Update the code accordingly, which allows removing the redirectReceived callback.
Update DocumentThreadableLoader to provide a correct response URL, including fragment identifier.
As DocumentThreadableLoader may reuse existing loads with same URL but different fragment identifier, we need to correctly set the response URL fragment identifier ourselves.

Covered by rebased test.

* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/worker-interception-redirect.https-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/service-workers/service-worker/worker-interception-redirect.https-expected.txt:
* Source/WebCore/loader/DocumentThreadableLoader.cpp:
* Source/WebCore/loader/DocumentThreadableLoader.h:
* Source/WebCore/loader/ThreadableLoaderClient.h:
(WebCore::ThreadableLoaderClient::redirectReceived): Deleted.
* Source/WebCore/loader/ThreadableLoaderClientWrapper.h:
(WebCore::ThreadableLoaderClientWrapper::redirectReceived): Deleted.
* Source/WebCore/loader/WorkerThreadableLoader.cpp:
(WebCore::WorkerThreadableLoader::MainThreadBridge::redirectReceived): Deleted.
* Source/WebCore/loader/WorkerThreadableLoader.h:
* Source/WebCore/workers/Worker.cpp:
(WebCore::Worker::notifyFinished):
* Source/WebCore/workers/WorkerFetchResult.h:
(WebCore::WorkerFetchResult::isolatedCopy const):
(WebCore::WorkerFetchResult::encode const):
(WebCore::WorkerFetchResult::decode):
* Source/WebCore/workers/WorkerScriptLoader.cpp:
(WebCore::WorkerScriptLoader::loadSynchronously):
(WebCore::WorkerScriptLoader::loadAsynchronously):
(WebCore::WorkerScriptLoader::fetchResult const):
(WebCore::WorkerScriptLoader::redirectReceived): Deleted.
* Source/WebCore/workers/WorkerScriptLoader.h:
(WebCore::WorkerScriptLoader::url const):
(WebCore::WorkerScriptLoader::lastRequestURL const): Deleted.
* Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.cpp:
(WebCore::generateWorkerParameters):
* Source/WebKit/WebProcess/Storage/WebSharedWorkerContextManagerConnection.cpp:
(WebKit::WebSharedWorkerContextManagerConnection::launchSharedWorker):

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

7f656c5

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 requested a review from cdumez as a code owner November 8, 2022 14:28
@youennf youennf self-assigned this Nov 8, 2022
@youennf youennf added Service Workers Component for Service Workers bugs. WebKit Nightly Build labels Nov 8, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 8, 2022
@youennf youennf force-pushed the eng/Dedicated-worker-and-shared-worker-global-scope-should-use-the-response-URL branch from a087af0 to 3159e12 Compare November 9, 2022 08:03
@youennf youennf force-pushed the eng/Dedicated-worker-and-shared-worker-global-scope-should-use-the-response-URL branch from 3159e12 to fb83411 Compare November 9, 2022 13:49
@youennf
Copy link
Contributor Author

youennf commented Nov 9, 2022

GTK test needs rebasing.

@youennf youennf force-pushed the eng/Dedicated-worker-and-shared-worker-global-scope-should-use-the-response-URL branch from fb83411 to 7f656c5 Compare November 10, 2022 10:49
@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 Nov 10, 2022
…se URL

https://bugs.webkit.org/show_bug.cgi?id=247615
rdar://problem/102088086

Reviewed by Chris Dumez.

As per https://html.spec.whatwg.org/multipage/workers.html#run-a-worker step 14.3.1, we should use the response URL.
Update the code accordingly, which allows removing the redirectReceived callback.
Update DocumentThreadableLoader to provide a correct response URL, including fragment identifier.
As DocumentThreadableLoader may reuse existing loads with same URL but different fragment identifier, we need to correctly set the response URL fragment identifier ourselves.

Covered by rebased test.

* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/worker-interception-redirect.https-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/service-workers/service-worker/worker-interception-redirect.https-expected.txt:
* Source/WebCore/loader/DocumentThreadableLoader.cpp:
* Source/WebCore/loader/DocumentThreadableLoader.h:
* Source/WebCore/loader/ThreadableLoaderClient.h:
(WebCore::ThreadableLoaderClient::redirectReceived): Deleted.
* Source/WebCore/loader/ThreadableLoaderClientWrapper.h:
(WebCore::ThreadableLoaderClientWrapper::redirectReceived): Deleted.
* Source/WebCore/loader/WorkerThreadableLoader.cpp:
(WebCore::WorkerThreadableLoader::MainThreadBridge::redirectReceived): Deleted.
* Source/WebCore/loader/WorkerThreadableLoader.h:
* Source/WebCore/workers/Worker.cpp:
(WebCore::Worker::notifyFinished):
* Source/WebCore/workers/WorkerFetchResult.h:
(WebCore::WorkerFetchResult::isolatedCopy const):
(WebCore::WorkerFetchResult::encode const):
(WebCore::WorkerFetchResult::decode):
* Source/WebCore/workers/WorkerScriptLoader.cpp:
(WebCore::WorkerScriptLoader::loadSynchronously):
(WebCore::WorkerScriptLoader::loadAsynchronously):
(WebCore::WorkerScriptLoader::fetchResult const):
(WebCore::WorkerScriptLoader::redirectReceived): Deleted.
* Source/WebCore/workers/WorkerScriptLoader.h:
(WebCore::WorkerScriptLoader::url const):
(WebCore::WorkerScriptLoader::lastRequestURL const): Deleted.
* Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.cpp:
(WebCore::generateWorkerParameters):
* Source/WebKit/WebProcess/Storage/WebSharedWorkerContextManagerConnection.cpp:
(WebKit::WebSharedWorkerContextManagerConnection::launchSharedWorker):

Canonical link: https://commits.webkit.org/256532@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Dedicated-worker-and-shared-worker-global-scope-should-use-the-response-URL branch from 7f656c5 to 99d5398 Compare November 10, 2022 14:30
@webkit-commit-queue
Copy link
Collaborator

Committed 256532@main (99d5398): https://commits.webkit.org/256532@main

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

@webkit-commit-queue webkit-commit-queue merged commit 99d5398 into WebKit:main Nov 10, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 10, 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
5 participants