+msramek to take a look.

Here's a video reproduction as well: https://www.youtube.com/watch?v=JV7SNIpGEks

@mkwst @msramek perhaps the priority of this bug should be more urgent? The impact of the bug on the user is extreme.

Thanks for the report.

Service worker deletion should work; this is quite extensively tested, and I re-tested manually. On the other hand, Clear-Site-Data coming from a service worker should not work at all; this is spec'd and implemented (and also covered by tests).

It seems that this issue only occurs when a deletion request is issued at the same time a service worker is being installed.

While this is definitely a bug, I'm not sure why is the impact on the user extreme? This seems very unlikely to occur in real-life usage of the header; and if it's implemented intentionally, it's easy for the web developer to spot.

For practical purposes, waiting for navigator.serviceWorker.ready before triggering the deletion should solve the issue.

For the actual bug, I think you're right about issue 795691 being possibly related. My guess would be that ServiceWorkerContext doesn't handle deletions via StoragePartition well? That is, we're not unregistering the service worker through the same codepath the JS API would, we're simply nuking it.

I'll have a closer look, but in the meantime, falken@, do you have ideas why this might fail? It reminds me of the old  issue 776711 .

Thanks for the response.

Regarding your statement: "Clear-Site-Data coming from a service worker should not work at all: this is spec'd and implemented"

Can you point me to the part of the spec that indicates that Clear-Site-Data in a service worker script response should not work? There are two parts of the non-normative spec language that I believe suggest otherwise. Am I interpreting this language incorrectly?

https://www.w3.org/TR/clear-site-data/#example-killswitch
"Note: Installing a Service Worker guarantees that a request will go out to a server every ~24 hours. That update ping would be a wonderful time to send a header like this one in case of catastrophe."

https://www.w3.org/TR/clear-site-data/#service-workers
"Note also that a service worker update is a network response, and is therefore not affected by this restriction. This is important in order to support the use case in §1.1.4 Kill Switch."

When I said the impact to the user is extreme, I meant that, given a scenario where the bug happens, the impact is extreme. I don't think this bug would necessarily be easy to spot. If Clear-Site-Data was only sent in certain scenarios, and via multiple URLs, it could potentially be difficult to trace. Also, given the fact that Clear-Site-Data is only supposed to be used if there's already a problem, it could be difficult to detect a secondary problem, originating from Clear-Site-Data.

#5 I don't have a specific idea but some random thoughts. I think you're on the right track that it's about  issue 795691 and  issue 776711 .

Deletion of service worker registrations is also complicated because we try to support the "resurrection" behavior as per the specification. If you unregister and then re-register while the service worker still has a tab open, things behave as if the unregister was never called. Fortunately there's been a spec decision to remove resurrection, which could simplify things.

Also reloading a controlled page is deceptively complicated. I think there is a complex sequence where it creates a new page, then unloads the old page, so if the old page was still using the service worker (and it wasn't unregistered), the new page gets that service worker.

Regarding the "not working when coming from a service worker" part.

https://www.w3.org/TR/clear-site-data/#fetch-integration describes that Clear-Site-Data would be part of the "HTTP-network fetch" algorithm. Fetches to which a SW responds are not network fetches.

https://www.w3.org/TR/clear-site-data/#service-workers describes why that's the case. You're quoting the last paragraph which mentions the update ping as an exception from the rule. You didn't read the first three which describe the rule :)

+dullweber@

Thanks for providing the reproduction steps. I see the issue you described.

I noticed a few things:
 - After the service worker is in the "Installing" state, deleting Cookies from Clear Browsing Data doesn't finish anymore. 
 - After Restarting Chrome, everything seems fine again if you reset the server, so that it doesn't respond with CSD anymore.

I will look a bit more to find out where it hangs.

I bisected the issue and the exciting result is that the following change broke it:
https://crrev.com/7d08794ea091f1a9d5684b0056fbf88fed090298 - "Enable Clear-Site-Data by default."

The CLs from  https://crbug.com/776711  were submitted afterwards, so they are not the issue. (But might be the reason that CBD doesn't finish when the service worker gets into this weird state)

I removed all code from sw.js and it still breaks. So it doesn't matter what the service worker is doing.

I added some logs to the server to see when CSD is send: 
Shortly after each pageload, Chrome tries to update the service worker. This update call is the first request, where CSD is sent. Afterwards, the service worker goes into the broken "installing" state.

Steps:
1. Install service worker on "/"
2. Request "clearSiteDataOn"
3. Chrome updates "sw.js" and receives the CSD header
4. Service worker is broken

If I instead do the following:
1. Install service worker on "/"
2. Request "clearSiteDataOn" via CURL to avoid the "sw.js" refresh
3. Request "/" and receive CSD

Chrome continues to work and uninstalls the service worker correctly. 

This makes it seem like the issue only occurs if CSD is sent in the service worker update request.
Do we have any tests for CSD during the update call? As explained above this is explicitly supported by the spec but it looks like it never worked ;)

@msramek I think there's some confusion here: I'm talking specifically about the request for the service worker script itself, i.e. the update ping, i.e. the exact exception you are referring to. I have read the spec and understand that the clear-site-data on a response coming from the service worker itself cannot be respected. However, I would still expect that a network response directly from the server, handled by the service worker should still respect clear-site-header. Is that an incorrect expectation?

To your point, @msramek, my reproduction app does add the header for all requests, so I made a branch where only the sw.js file has clear-site-data. The hanging bug does not happen, but the service worker doesn't seem to get cleared out, and is still responding to requests: https://github.com/asakusuma/sw-clear-site-data/tree/sw-header-only

Assuming I am reading the spec correctly and network requests made from the service worker to the server still respect clear-site-data, I think it's totally reasonable to want to serve clear-site-data from any endpoint on the server, not just the service worker script itself. If that is not the case, it seems like you are basically saying that if your app has a service worker, you can't serve clear-site-data for any endpoint that the service worker interacts with.

@dullweber thanks a ton for your progress here!

The issue seems to be that ClearSiteDataThrottle expects every request to be associated with a WebContext. The update call is not associated with a WebContents and the Throttle doesn't finish its task. This causes the network request to never finish.
https://cs.chromium.org/chromium/src/content/browser/browsing_data/clear_site_data_throttle.cc?l=96&rcl=d22ab991da0abcd793f0cbbeb79ae29d9cbcb831

We will need to find a different way to get a Profile for a UrlRequest and then it will hopefully work. 

*WebContents*

Re #13: Correct, Clear-Site-Data should work on SW install and update requests, because in both cases the response comes from the server.

My statement "Clear-Site-Data coming from a service worker should not work at all" was about responses from service workers, and was meant to exclude situations where the bug isn't reproducible, as the first step in debugging.

I created a CL that fixes the hanging requests but it doesn't perform Clear-Site-Data on service worker updates: https://crrev.com/c/1346468

Fixing CSD needs some more investigation as I haven't found a way to get access to a BrowserContext from the ResourceThrottle yet. (except through a WebContents)

It is possible to get a BrowserContext without a WebContents by
calling RenderProcessHostImpl::FromID(process_id)->GetBrowserContext()
(https://crrev.com/c/1348058)

This allows BrowsingDataRemover to start but it doesn't finish, so I need to investigate further.

I think this is the issue:
If we send CSD during a service worker update, we run into a deadlock.

1. The service worker update job won't finish before the request finishes.
2. During the request, CSD creates an unregister job but because there is already a running update job, this job won't start.
3. Because the unregister job never starts, CSD doesn't finish and the request is on hold indefinitely.

:(

I think that we either have to cancel the update or skip the uninstallation.

What is the expected behavior if we send CSD during an update? 
Should the service worker still exist afterwards?

> I think that we either have to cancel the update or skip the uninstallation.

Getting rid of a service worker is one of the key use cases for CSD, so cancelling the update (in order to remove the service worker entirely) seems like the right behavior to me. I'd expect that if a service worker update request received a response containing the CSD header, that we'd abort the update, clear the service worker registration, and go on our way service workerless.

@dullweber thanks for all your work here!

I agree with @mkwst, cancelling the update would be the correct behavior, at least for the storage directive.

Does this deadlock happen for all directives? I believe the service worker is only supposed to be removed on the storage CSD directive.

Right now, sending Clear-Site-Data during a request that is not associated to a WebContents (e.g. ServiceWorker updates, possibly more), will hang for all directives. 

There are two separate bugs, one is related to "non webcontents" requests, the other is related to deleting a ServiceWorker during an update. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71d688ab9e6e722cd391716904b24ecc4476565b

commit 71d688ab9e6e722cd391716904b24ecc4476565b
Author: Christian Dullweber <dullweber@chromium.org>
Date: Wed Nov 28 10:06:46 2018

Fix hanging requests from Clear-Site-Data

Some requests, e.g. requests for service worker updates, are not
associated with a WebContents. In these cases Clear-Site-Data returns
early and doesn't mark its task as finished, which leads to hanging
requests.
This CL fixes the hanging requests but does not actually perform the
site data deletion. That will be fixed in a followup CL.

Bug:  898465 
Change-Id: I99bbf4339f7b123d4a46552e9f117d4d95b7ec67
Reviewed-on: https://chromium-review.googlesource.com/c/1346468
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611625}
[modify] https://crrev.com/71d688ab9e6e722cd391716904b24ecc4476565b/content/browser/browsing_data/clear_site_data_handler.cc
[modify] https://crrev.com/71d688ab9e6e722cd391716904b24ecc4476565b/content/browser/browsing_data/clear_site_data_handler_browsertest.cc
[modify] https://crrev.com/71d688ab9e6e722cd391716904b24ecc4476565b/content/browser/browsing_data/clear_site_data_throttle.cc
[add] https://crrev.com/71d688ab9e6e722cd391716904b24ecc4476565b/content/test/data/browsing_data/worker_test.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0932269d2140abafe08058d7f30ef074fbd7a8b

commit b0932269d2140abafe08058d7f30ef074fbd7a8b
Author: Christian Dullweber <dullweber@chromium.org>
Date: Wed Nov 28 10:33:04 2018

Refactor ClearSiteData to remove duplicate code

To simplify fixing Clear-Site-Data, I move some of the code that was
duplicated for the network_service into a shared location.

Bug:  898465 
Change-Id: I25eac82e7a6f3481b03ac88946a1e7a6828cbfc9
Reviewed-on: https://chromium-review.googlesource.com/c/1350971
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611633}
[modify] https://crrev.com/b0932269d2140abafe08058d7f30ef074fbd7a8b/content/browser/BUILD.gn
[modify] https://crrev.com/b0932269d2140abafe08058d7f30ef074fbd7a8b/content/browser/browsing_data/clear_site_data_handler.cc
[modify] https://crrev.com/b0932269d2140abafe08058d7f30ef074fbd7a8b/content/browser/browsing_data/clear_site_data_throttle.cc
[add] https://crrev.com/b0932269d2140abafe08058d7f30ef074fbd7a8b/content/browser/browsing_data/clear_site_data_utils.cc
[add] https://crrev.com/b0932269d2140abafe08058d7f30ef074fbd7a8b/content/browser/browsing_data/clear_site_data_utils.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/091e5e9ba169034ef06e35425d304fda5f58551f

commit 091e5e9ba169034ef06e35425d304fda5f58551f
Author: Christian Dullweber <dullweber@chromium.org>
Date: Fri Nov 30 13:59:51 2018

Fix Clear-Site-Data during service workers updates

When Clear-Site-Data is triggered during a ServiceWorker update request,
there is a deadlock because the update task is waiting for the request,
the request is waiting for Clear-Site-Data, Clear-Site-Data is waiting
for the unregistration of the service worker and the unregistration is
waiting for the update task.
This is fixed by cancelling the update task and removing the service
worker without updating it.
Also requests from Service Workers are not associated with a
WebContents, so we need a better way to get a BrowserContext for these
requests.

Bug:  898465 
Change-Id: I543b44e07720cf1849d1d4245ee7d36ec762efb6
Reviewed-on: https://chromium-review.googlesource.com/c/1348058
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612634}
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_handler.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_handler.h
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_handler_browsertest.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_handler_unittest.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_throttle.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_utils.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/browsing_data/clear_site_data_utils.h
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/network_service_client.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/service_worker/service_worker_context_core.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/service_worker/service_worker_job_coordinator.cc
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/service_worker/service_worker_job_coordinator.h
[modify] https://crrev.com/091e5e9ba169034ef06e35425d304fda5f58551f/content/browser/service_worker/service_worker_job_unittest.cc

The issue is fixed in Chrome Canary 73.0.3627.0 and newer versions. 

Requesting merge to 72 as the last commit (#28) missed the branch point by a few hours.

Thanks @dullweber!

Is there any possibility that the hanging fix (#26, 71d688ab9e6e722cd391716904b24ecc4476565b) could get backported to 70?

70 is already shipped. That's too late.
71 stable release is today, so that's a bit difficult as well. We would only merge very critical fixes at this point.

For now I would recommend not to send Clear-Site-Data in service worker update requests and wait for 72.

Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7

commit b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7
Author: Christian Dullweber <dullweber@chromium.org>
Date: Tue Dec 04 09:48:13 2018

Fix Clear-Site-Data during service workers updates

When Clear-Site-Data is triggered during a ServiceWorker update request,
there is a deadlock because the update task is waiting for the request,
the request is waiting for Clear-Site-Data, Clear-Site-Data is waiting
for the unregistration of the service worker and the unregistration is
waiting for the update task.
This is fixed by cancelling the update task and removing the service
worker without updating it.
Also requests from Service Workers are not associated with a
WebContents, so we need a better way to get a BrowserContext for these
requests.

Bug:  898465 
Change-Id: I543b44e07720cf1849d1d4245ee7d36ec762efb6
Reviewed-on: https://chromium-review.googlesource.com/c/1348058
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612634}(cherry picked from commit 091e5e9ba169034ef06e35425d304fda5f58551f)
Reviewed-on: https://chromium-review.googlesource.com/c/1360610
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#23}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_handler.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_handler.h
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_handler_browsertest.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_handler_unittest.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_throttle.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_utils.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/browsing_data/clear_site_data_utils.h
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/network_service_client.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/service_worker/service_worker_context_core.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/service_worker/service_worker_job_coordinator.cc
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/service_worker/service_worker_job_coordinator.h
[modify] https://crrev.com/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7/content/browser/service_worker/service_worker_job_unittest.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7

Commit: b9b8bda29bf310ed88dbf02d0a054b7ac2cff4b7
Author: dullweber@chromium.org
Commiter: dullweber@chromium.org
Date: 2018-12-04 09:48:13 +0000 UTC

Fix Clear-Site-Data during service workers updates

When Clear-Site-Data is triggered during a ServiceWorker update request,
there is a deadlock because the update task is waiting for the request,
the request is waiting for Clear-Site-Data, Clear-Site-Data is waiting
for the unregistration of the service worker and the unregistration is
waiting for the update task.
This is fixed by cancelling the update task and removing the service
worker without updating it.
Also requests from Service Workers are not associated with a
WebContents, so we need a better way to get a BrowserContext for these
requests.

Bug:  898465 
Change-Id: I543b44e07720cf1849d1d4245ee7d36ec762efb6
Reviewed-on: https://chromium-review.googlesource.com/c/1348058
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612634}(cherry picked from commit 091e5e9ba169034ef06e35425d304fda5f58551f)
Reviewed-on: https://chromium-review.googlesource.com/c/1360610
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#23}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}