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

Clear-Site-Data: clients uncontrolled by service workers #19132

Merged
merged 1 commit into from Oct 7, 2019

Conversation

@asakusuma
Copy link
Contributor

commented Sep 18, 2019

Adds a failing test, asserting that pages actively controlled by a service worker become uncontrolled after the Clear-Site-Data storage directive.

We agreed on this behavior in a spec issue and confirmed the need for this test at the TPAC Service Workers F2F.

w3c/ServiceWorker#614
F2F notes: https://docs.google.com/document/d/1_Qfw5m3BJEaL1xIzTJd41HXjgJ9gq7mroBDXqSJIzic/edit

@wpt-pr-bot wpt-pr-bot requested review from mikewest and msramek Sep 18, 2019
@asakusuma asakusuma force-pushed the asakusuma:csd-kill-sw branch 5 times, most recently from 787a13c to 53b7aff Sep 18, 2019
clear-site-data/storage.https.html Outdated Show resolved Hide resolved
clear-site-data/storage.https.html Show resolved Hide resolved
@asakusuma asakusuma force-pushed the asakusuma:csd-kill-sw branch from 53b7aff to 0954b39 Sep 27, 2019
clear-site-data/storage.https.html Outdated Show resolved Hide resolved
clear-site-data/support/test_utils.sub.js Outdated Show resolved Hide resolved
@asakusuma asakusuma force-pushed the asakusuma:csd-kill-sw branch 4 times, most recently from df264b5 to 9826852 Sep 28, 2019
@mattto
mattto approved these changes Oct 1, 2019
Copy link
Member

left a comment

This lgtm, thanks.

clear-site-data/storage.https.html Outdated Show resolved Hide resolved
…e directive

Adds a failing test, asserting that pages actively controlled by a service worker become uncontrolled after the Clear-Site-Data storage directive.

We agreed on this behavior in a spec issue and confirmed the need for this test at the TPAC Service Workers F2F.

w3c/ServiceWorker#614
F2F notes: https://docs.google.com/document/d/1_Qfw5m3BJEaL1xIzTJd41HXjgJ9gq7mroBDXqSJIzic/edit
@asakusuma asakusuma force-pushed the asakusuma:csd-kill-sw branch from 9826852 to ec95f4e Oct 1, 2019
@mattto

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

@msramek or @mikewest did you want to review this? Otherwise is it ready to merge? We still need to update the spec itself of course.

@msramek

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

@mattto I haven't worked with this code for a while, so I don't think I have much to add beyond your code review.

I'm not sure what you mean by the spec update though; the Clear-Site-Data spec already states that "Service Workers registered for an origin are terminated and deregistered."

@asakusuma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@msramek thanks for chiming in. I believe @mattto is talking about updating the normative part of the spec, which only says to call unregister(), which does not terminate the worker or uncontrol clients. I'm assuming you are quoting the non-normative goals section of the Clear-Site-Data spec. So really the required spec change is just about updating the normative section to reflect the already stated non-normative goals, which you pointed out.

@msramek

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Ah, right. Makes sense. The Chromium implementation actually already works that way; it directly erases the service worker from the storage, instead of just going through the JavaScript codepath of calling unregister().

@asakusuma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

@msramek the new tests added in this PR do not pass in Chrome. Which suggests that either the tests are incorrect, or Chromium doesn’t actually remove the service worker immediately. My local manual testing shows that the service worker still responds to requests on any page that was open when CSD was delivered.

@wanderview wanderview merged commit 8be01b6 into web-platform-tests:master Oct 7, 2019
8 of 11 checks passed
8 of 11 checks passed
update-pr-preview
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20191001.113 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.