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

Do not clear content filter when loading after serviceWorkerDidNotHandle #14561

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Do not clear content filter when loading after serviceWorkerDidNotHandle #14561

merged 1 commit into from
Jun 9, 2023

Conversation

pascoej
Copy link
Member

@pascoej pascoej commented May 31, 2023 •
edited by webkit-early-warning-system

36a507b

Do not clear content filter when loading after serviceWorkerDidNotHandle
https://bugs.webkit.org/show_bug.cgi?id=257569
rdar://109619110

Reviewed by Youenn Fablet.

The content filter is cleared after serviceWorkerDidNotHandle is called, but another
request can be made using the same NetworkResourceLoader, without a content filter.

This patch fixes that by not clearing the filter.

* Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::serviceWorkerDidNotHandle):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:
(TEST):

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

aac5f01

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 ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@pascoej pascoej self-assigned this May 31, 2023
@pascoej pascoej added the Service Workers Component for Service Workers bugs. label May 31, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash b4a081a)

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 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 1, 2023
@pascoej pascoej removed the merging-blocked Applied to prevent a change from being merged label Jun 8, 2023
@pascoej pascoej force-pushed the eng/Do-not-clear-content-filter-when-loading-after-serviceWorkerDidNotHandle branch from b4a081a to cdc1e05 Compare June 8, 2023 18:24
@pascoej pascoej marked this pull request as ready for review June 8, 2023 18:25
@pascoej pascoej requested a review from cdumez as a code owner June 8, 2023 18:25
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash cdc1e05)

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   🛠 gtk
❌ 🧪 ios-wk2-wpt   🧪 mac-wk1   🧪 gtk-wk2
⏳ 🛠 🧪 jsc ❌ 🧪 api-ios   🧪 mac-wk2   🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim   🧪 mac-wk2-stress
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 8, 2023
@pascoej pascoej removed the merging-blocked Applied to prevent a change from being merged label Jun 8, 2023
@pascoej pascoej force-pushed the eng/Do-not-clear-content-filter-when-loading-after-serviceWorkerDidNotHandle branch from cdc1e05 to 7bca3da Compare June 8, 2023 18:29
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 7bca3da)

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 ✅ 🛠 gtk
❌ 🧪 ios-wk2-wpt   🧪 mac-wk1   🧪 gtk-wk2
⏳ 🛠 🧪 jsc ❌ 🧪 api-ios   🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim   🧪 mac-wk2-stress
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged labels Jun 8, 2023
@pascoej pascoej removed the merging-blocked Applied to prevent a change from being merged label Jun 8, 2023
@pascoej pascoej force-pushed the eng/Do-not-clear-content-filter-when-loading-after-serviceWorkerDidNotHandle branch from 7bca3da to 668c2a6 Compare June 8, 2023 18:43
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 668c2a6)

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 ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1   🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

youennf
youennf approved these changes Jun 8, 2023
View reviewed changes
Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm Outdated
"{"
" frame = document.createElement('iframe');"
" frame.src = \"/test.html\";"
" frame.onload = function() { window.webkit.messageHandlers.sw.postMessage(frame.contentDocument.body.innerHTML); }"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use log instead

Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm Outdated
"{"
" frame = document.createElement('iframe');"
" frame.src = \"/test.html\";"
" frame.onload = function() { window.webkit.messageHandlers.sw.postMessage(frame.contentDocument.body.innerHTML); }"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use log instead

Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm Outdated

@end

static void loadAlternateTest(Decision decision, DecisionPoint decisionPoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to move this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

In order to make the test able to find all the functions where I initially placed it, yes. But I think I can just add the new test to the bottom to avoid this. I will make that change as not to pollute the blame too much.

Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm Outdated
[webView evaluateJavaScript:@"document.body.innerText" completionHandler:^(id result, NSError *error) {
EXPECT_TRUE([result isKindOfClass:[NSString class]]);
EXPECT_WK_STREQ(@"blocked", result);
isDone = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is existing code but having EXPECT checks here makes the test a bit hard to read.
The test itself has no EXPECT checks, it feels like magic.
It could be easier to read if we would wait until done is true in the test, then do a EXPECT_TRUE... in the test itself.

Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm Outdated

TestWebKitAPI::HTTPServer server({
{ "/"_s, { mainForFetchTestBytes } },
{ "/sw.js"_s, { { { "Content-Type"_s, "application/javascript"_s } }, serviceWorkerJS } },
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a "/test.html" entry for good measure.

@pascoej pascoej force-pushed the eng/Do-not-clear-content-filter-when-loading-after-serviceWorkerDidNotHandle branch from 668c2a6 to aac5f01 Compare June 8, 2023 20:37
@webkit-early-warning-system
Copy link
Collaborator

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

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 ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@pascoej pascoej added the merge-queue Applied to send a pull request to merge-queue label Jun 9, 2023
@pascoej
Do not clear content filter when loading after serviceWorkerDidNotHandle
36a507b 
https://bugs.webkit.org/show_bug.cgi?id=257569
rdar://109619110

Reviewed by Youenn Fablet.

The content filter is cleared after serviceWorkerDidNotHandle is called, but another
request can be made using the same NetworkResourceLoader, without a content filter.

This patch fixes that by not clearing the filter.

* Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::serviceWorkerDidNotHandle):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:
(TEST):

Canonical link: https://commits.webkit.org/265022@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Do-not-clear-content-filter-when-loading-after-serviceWorkerDidNotHandle branch from aac5f01 to 36a507b Compare June 9, 2023 16:09
@webkit-commit-queue
Copy link
Collaborator

Committed 265022@main (36a507b): https://commits.webkit.org/265022@main

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

@webkit-commit-queue webkit-commit-queue merged commit 36a507b into WebKit:main Jun 9, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youennf youennf youennf approved these changes

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

Assignees

@pascoej pascoej

Labels
Service Workers Component for Service Workers bugs.
Projects
None yet
Milestone
No milestone
5 participants
@pascoej @webkit-early-warning-system @webkit-commit-queue @youennf @webkit-ews-buildbot