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.

Sign up for GitHub

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

Jump to bottom

Add missing service worker content filter check #12715

Merged
merged 1 commit into from Apr 14, 2023
Merged

Add missing service worker content filter check #12715

merged 1 commit into from Apr 14, 2023

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Apr 13, 2023 •
edited by webkit-early-warning-system

09061ad

Add missing service worker content filter check
https://bugs.webkit.org/show_bug.cgi?id=255418
rdar://107453214

Reviewed by Brent Fulgham.

This patch was authored by Ben Nham and Per Arne Vollan.

Add missing service worker content filter check after finished adding data. This fixes an issue where
service workers are incorrectly blocked.

* LayoutTests/TestExpectations:
* LayoutTests/http/wpt/service-workers/basic-fetch-with-contentfilter-allow-after-adding-data-expected.txt: Added.
* LayoutTests/http/wpt/service-workers/basic-fetch-with-contentfilter-allow-after-adding-data.html: Added.
* LayoutTests/http/wpt/service-workers/basic-fetch-with-contentfilter.js:
(doTest):
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac/TestExpectations:
* Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::serviceWorkerDidFinish):

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

c454908

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
  🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@pvollan pvollan requested a review from cdumez as a code owner April 13, 2023 22:25
@pvollan pvollan self-assigned this Apr 13, 2023
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Apr 13, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 657d1a8)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

brentfulgham
brentfulgham approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@brentfulgham brentfulgham left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is small, and well contained -- and the tests seem good.

Since this uses internals state to adjust content filters, there is probably no way to do WPT-style testing here, so this seems good to add to our custom suite.

brentfulgham
brentfulgham approved these changes Apr 13, 2023
View reviewed changes
...utTests/http/wpt/service-workers/basic-fetch-with-contentfilter-allow-after-adding-data.html
var activeWorker;

if (window.internals) {
var settings = window.internals.mockContentFilterSettings;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see -- we are passing custom filter settings here.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 14, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Apr 14, 2023
@pvollan pvollan force-pushed the eng/Add-missing-service-worker-content-filter-check branch from 657d1a8 to c454908 Compare April 14, 2023 14:08
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash c454908)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
  🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@pvollan
Copy link
Contributor Author

pvollan commented Apr 14, 2023

The change is small, and well contained -- and the tests seem good.

Since this uses internals state to adjust content filters, there is probably no way to do WPT-style testing here, so this seems good to add to our custom suite.

Thanks for reviewing!

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 14, 2023
@pvollan pvollan added merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Apr 14, 2023
@pvollan
Add missing service worker content filter check
09061ad 
https://bugs.webkit.org/show_bug.cgi?id=255418
rdar://107453214

Reviewed by Brent Fulgham.

This patch was authored by Ben Nham and Per Arne Vollan.

Add missing service worker content filter check after finished adding data. This fixes an issue where
service workers are incorrectly blocked.

* LayoutTests/TestExpectations:
* LayoutTests/http/wpt/service-workers/basic-fetch-with-contentfilter-allow-after-adding-data-expected.txt: Added.
* LayoutTests/http/wpt/service-workers/basic-fetch-with-contentfilter-allow-after-adding-data.html: Added.
* LayoutTests/http/wpt/service-workers/basic-fetch-with-contentfilter.js:
(doTest):
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac/TestExpectations:
* Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::serviceWorkerDidFinish):

Canonical link: https://commits.webkit.org/262972@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Add-missing-service-worker-content-filter-check branch from c454908 to 09061ad Compare April 14, 2023 16:27
@webkit-commit-queue
Copy link
Collaborator

Committed 262972@main (09061ad): https://commits.webkit.org/262972@main

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

@webkit-commit-queue webkit-commit-queue merged commit 09061ad into WebKit:main Apr 14, 2023
@webkit-commit-queue webkit-commit-queue removed merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels Apr 14, 2023
@turbomaster95 turbomaster95 mentioned this pull request Apr 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brentfulgham brentfulgham brentfulgham approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@bnham bnham Awaiting requested review from bnham

@geoffreygaren geoffreygaren Awaiting requested review from geoffreygaren

@szewai szewai Awaiting requested review from szewai

Assignees

@pvollan pvollan

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
5 participants
@pvollan @webkit-early-warning-system @webkit-commit-queue @brentfulgham @webkit-ews-buildbot