New issue
Advanced search Search tips

Issue 866353 link

Starred by 1 user
Status: Started
Owner:
falken@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Remove SetAllowAssociation

Project Member Reported by falken@chromium.org, Jul 23 
This is used to stop a registration from claiming a provider host while it's undergoing its main resource request. But it should be sufficient to use is_execution_ready_ for that.

Unfortunately there is still some trickiness with shared workers (before NetS13nSW) and about:blank iframes, since they initialize with is_execution_ready_ to true.

We can revisit after NetS13nSW.

The current implementation is also probably buggy, since we SetAllowAssociation(true) in between PrepareForMainResource calls. This probably means a registration can claim a provider host during a redirect.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 23 

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

commit 2c47ce6a617cc70b5bf50245e4b0de5638e9e849
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jul 23 10:21:07 2018

service worker: Clarify SetAllowAssociation().

This is one of the tricky parts of service worker code, and its role has
changed after various changes over the years.

There is no longer such thing as a "new registration" associating
with a provider host. Association happens only during:
1) main resource requests in ServiceWorkerControlleeRequestHandler
2) claim() in ServiceWorkerRegistration.

There is no need for ServiceWorkerControlleeRequestHandler to defend
against itself. So claim() is the only thing to defend against.

So the whole point of it is to stop claim() from claiming provider
hosts that are still undergoing their main resource request. This
should be the same thing as |is_execution_ready_|. But
unfortunately there is still edge cases/trickiness around
shared workers and about:blank iframes. Add some comments and
revisit after NetS13nServiceWorker.

Change-Id: I508f13c52fc30aa40776e17d4966ff05c0c57e14
Bug: 866353
Reviewed-on: https://chromium-review.googlesource.com/1146414
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577134}
[modify] https://crrev.com/2c47ce6a617cc70b5bf50245e4b0de5638e9e849/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/2c47ce6a617cc70b5bf50245e4b0de5638e9e849/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/2c47ce6a617cc70b5bf50245e4b0de5638e9e849/content/browser/service_worker/service_worker_provider_host.h

Comment 2 by falken@chromium.org, Jul 24 

I'm realizing there a lot of refactoring opportunity with the whole associated registration thing. It's from an earlier spec where we had navigator.serviceWorker.waiting, navigator.serviceWorker.installed. We don't need this complexity so much. Instead there should just be a "used_registration_" which is only set when the registration's active worker is the host's controller.
But it will take some patches to untangle all this, esp since we have things like NotifyControllerLost.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 25 

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

commit a6d9c824ceff792e8cf8750704ca5a638c12a5c1
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Jul 25 09:43:25 2018

service worker: Slim down semantics of provider host's "associated registration".

This should really just be the registration of the controller. Slim down
some of the meaning so it's more like that.

Bug: 866353
Change-Id: I98a412dd4456e5600125ea7fccb44d6e0e049faf
Reviewed-on: https://chromium-review.googlesource.com/1149439
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577829}
[modify] https://crrev.com/a6d9c824ceff792e8cf8750704ca5a638c12a5c1/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/a6d9c824ceff792e8cf8750704ca5a638c12a5c1/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
[modify] https://crrev.com/a6d9c824ceff792e8cf8750704ca5a638c12a5c1/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/a6d9c824ceff792e8cf8750704ca5a638c12a5c1/content/browser/service_worker/service_worker_provider_host.h
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 26 

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

commit 7484dc4442b918afb8de2ce8fd6334e79f52797e
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Jul 26 04:30:48 2018

service worker: Make "associated_registration_" just the controller's registration.

This further reduces weird combination possiblities and matches the
spec's concept of "using a registration":
"When a service worker client is controlled by an active worker, it is
considered that the service worker client is using the active worker’s
containing service worker registration."
https://w3c.github.io/ServiceWorker/#dfn-use

Bug: 866353
Change-Id: I37a668db3696d092384deaafee6cdcbd4923e18e
Reviewed-on: https://chromium-review.googlesource.com/1149765
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578199}
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_provider_host_unittest.cc
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_request_handler.cc
[modify] https://crrev.com/7484dc4442b918afb8de2ce8fd6334e79f52797e/content/browser/service_worker/service_worker_version.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 26 

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

commit 18f0d9943981659d1f5732bd424c394dfc7ac471
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Jul 26 09:00:13 2018

service worker: Rename AssociateRegistration to SetControllerRegistration.

Increase consistency around the registration, controller, and matching
registrations.

Bug: 866353
Change-Id: I2e4f22cee1a542c6037bb7aa2c79469bf6818aa9
Reviewed-on: https://chromium-review.googlesource.com/1151028
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578243}
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_provider_host_unittest.cc
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/18f0d9943981659d1f5732bd424c394dfc7ac471/content/browser/service_worker/service_worker_version_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 26 

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

commit 7cfcc0ee54ab804580b42caf098b03fc2427fc1c
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Jul 26 09:46:57 2018

service worker: Remove ServiceWorkerProviderHost::active_version().

Callsites are really interested in the controller.

Bug: 866353
Change-Id: I263d5c874702839fdbca8b2b4be105b6b26e08ec
Reviewed-on: https://chromium-review.googlesource.com/1151072
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578250}
[modify] https://crrev.com/7cfcc0ee54ab804580b42caf098b03fc2427fc1c/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/7cfcc0ee54ab804580b42caf098b03fc2427fc1c/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
[modify] https://crrev.com/7cfcc0ee54ab804580b42caf098b03fc2427fc1c/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/7cfcc0ee54ab804580b42caf098b03fc2427fc1c/content/browser/service_worker/service_worker_provider_host_unittest.cc
[modify] https://crrev.com/7cfcc0ee54ab804580b42caf098b03fc2427fc1c/content/browser/service_worker/service_worker_url_request_job_unittest.cc
[modify] https://crrev.com/7cfcc0ee54ab804580b42caf098b03fc2427fc1c/content/browser/service_worker/service_worker_version.cc

Comment 7 by shimazu@chromium.org, Jul 27

Owner: falken@chromium.org
Status: Started (was: Available)
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 2 

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

commit 4caad7be8b7552745c79403f5a10592f5043f030
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Aug 02 07:18:13 2018

service worker: Various style cleanup in ServiceWorkerControlleeRequestHandler.

Including:
- std::move() when appropriate.
- Change diagnostic CHECKs to DCHECKs.
- |self| doesn't seem like Chromium style.
- Don't repeat base class documentation in overrides.

Bug: 866353
Change-Id: I9d7d3653cd801770251936bcae330bf1610d0fdb
Reviewed-on: https://chromium-review.googlesource.com/1159933
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580099}
[modify] https://crrev.com/4caad7be8b7552745c79403f5a10592f5043f030/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/4caad7be8b7552745c79403f5a10592f5043f030/content/browser/service_worker/service_worker_controllee_request_handler.h
►

Sign in to add a comment