Reserved clients don't get notified of new matching registrations so can't be claim()'ed when ready |
||
Issue descriptionThis is a race introduced with PlzNavigate exposed by internal b/72180655: 1. Start navigation (precreate provider host) 2. ProviderHost::set_document_url() is called and adds matching_ registrations_. 3. In another frame, a SW registration finishes and looks for provider hosts. The precreated host can't be found since it's not yet added to contextcore. 4. The navigation finishes and provider host is added to ContextCore. 5. A SW from (3) calls claim(). But matching registrations in the provider host from (1) don't have the registration from (3). Therefore the provider host isn't claimed. One solution could be to sync with live registrations in (4). But this is insufficient because the registration from (3) might no longer be live. There's another inconsistency already about when SWProviderHost is added to ContextCore. GetProviderHostByClientID() will find precreated hosts and ProviderHostIterator will not. I'm leaning toward adding all SWProviderHosts to ContextCore and the users will just need to use is_execution_ready() to determine reserved vs ready clients. This would also be more aligned with spec algorithms. It also seems long-term we should remove provider_id and just use client_uuid as in the comment note: // (Note: instead of maintaining 2 maps we might be able to uniformly use // UUID instead of process_id+provider_id elsewhere. For now I'm leaving // these as provider_id is deeply wired everywhere) This would also address the TODO: // TODO(falken) Try to remove this call. It should be unnecessary because // provider hosts remove themselves when their Mojo connection to the renderer // is destroyed. But if we don't remove the hosts immediately here, collisions // of <process_id, provider_id> can occur if a new dispatcher host is // created for a reused RenderProcessHost. https://crbug.com/736203 RemoveAllProviderHostsForProcess(process_id); But removing all provider_id is going to be a big refactoring.
,
May 9 2018
FWIW we decided claim() should not control reserved clients: https://github.com/w3c/ServiceWorker/issues/1090#issuecomment-291048928 Not sure if that helps here, though, since it seems this is an issue with claiming after the client is moved to execution ready?
,
May 9 2018
Yep, strongly agree that claim() shouldn't control reserved clients. As you mention, this is a bug that's preventing controlling the client once it's execution ready.
,
May 9 2018
i.e., claim() should ignore reserved clients. You have to call claim() after the client is execution ready in order to have any effect.
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ce272e07f6aa48a4b73173c438c5c7b51419ad3 commit 0ce272e07f6aa48a4b73173c438c5c7b51419ad3 Author: Matt Falkenhagen <falken@chromium.org> Date: Thu May 10 02:03:32 2018 service worker: Make provider hosts for reserved clients first-class members of SWContextCore. Before this CL, provider hosts for documents that were still undergoing navigation (i.e., hosts for "reserved clients"), were not owned by SWContextCore. Instead they were owned by ServiceWorkerNavigationHandle* and added to SWContextCore after navigation commit and the renderer ACKs back an OnProviderCreated IPC. This CL makes SWContextCore own such provider hosts. This means that users of SWContextCore will start needing to be aware of reserved vs ready clients. There are some motivations: - Sometimes we need to do operations on reserved clients. For example, the linked bug happens because a new registration can't find reserved clients. - SWContextCore already implicitly knows about reserved clients, since GetProviderByClientUUID can return them. This is a bit of a confusing inconsistency. - The spec has evolved to make "reserved clients" a concept, so it's generally useful to have them clearly defined to more clearly match the spec algorithms. This CL isn't expected to have a behavior change but clears the way to fixing the linked bug. Bug: 841070 Change-Id: I31ba40029e1b7247f41d69abcf4fa6ef97300a0d Reviewed-on: https://chromium-review.googlesource.com/1051145 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#557423} [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_context_core.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_context_core.h [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_dispatcher_host.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_navigation_handle.h [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_navigation_handle_core.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_navigation_handle_core.h [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_provider_host.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_provider_host.h [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_provider_host_unittest.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/browser/service_worker/service_worker_request_handler.cc [modify] https://crrev.com/0ce272e07f6aa48a4b73173c438c5c7b51419ad3/content/common/service_worker/service_worker_utils.h
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c commit e8d1243bd7a2c823a0b778e11150ec6e0a3df90c Author: Matt Falkenhagen <falken@chromium.org> Date: Thu May 10 05:27:08 2018 Revert "service worker: Make provider hosts for reserved clients first-class members of SWContextCore." This reverts commit 0ce272e07f6aa48a4b73173c438c5c7b51419ad3. Reason for revert: Causes crashes on layout tests on MSAN. Original change's description: > service worker: Make provider hosts for reserved clients first-class members of SWContextCore. > > Before this CL, provider hosts for documents that were still undergoing > navigation (i.e., hosts for "reserved clients"), were not owned by > SWContextCore. Instead they were owned by > ServiceWorkerNavigationHandle* and added to SWContextCore after > navigation commit and the renderer ACKs back an OnProviderCreated IPC. > > This CL makes SWContextCore own such provider hosts. This means that > users of SWContextCore will start needing to be aware of reserved vs > ready clients. There are some motivations: > - Sometimes we need to do operations on reserved clients. For example, > the linked bug happens because a new registration can't find reserved > clients. > - SWContextCore already implicitly knows about reserved clients, since > GetProviderByClientUUID can return them. This is a bit of a confusing > inconsistency. > - The spec has evolved to make "reserved clients" a concept, so > it's generally useful to have them clearly defined to more clearly > match the spec algorithms. > > This CL isn't expected to have a behavior change but clears the way > to fixing the linked bug. > > Bug: 841070 > Change-Id: I31ba40029e1b7247f41d69abcf4fa6ef97300a0d > Reviewed-on: https://chromium-review.googlesource.com/1051145 > Reviewed-by: Makoto Shimazu <shimazu@chromium.org> > Commit-Queue: Matt Falkenhagen <falken@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557423} TBR=falken@chromium.org,kinuko@chromium.org,clamy@chromium.org,shimazu@chromium.org Change-Id: I72b3f4360b764d3d5882582d8ef0f7e07ad96e46 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 841070 Reviewed-on: https://chromium-review.googlesource.com/1053469 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#557457} [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_context_core.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_context_core.h [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_dispatcher_host.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_navigation_handle.h [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_navigation_handle_core.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_navigation_handle_core.h [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_provider_host.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_provider_host.h [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_provider_host_unittest.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/browser/service_worker/service_worker_request_handler.cc [modify] https://crrev.com/e8d1243bd7a2c823a0b778e11150ec6e0a3df90c/content/common/service_worker/service_worker_utils.h
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e commit 4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e Author: Matt Falkenhagen <falken@chromium.org> Date: Thu May 10 07:23:26 2018 Reland "service worker: Make provider hosts for reserved clients first-class members of SWContextCore." Relands 0ce272e07f6aa48a4b73173c438c5c7b51419ad3. Original code review: https://chromium-review.googlesource.com/1051145 Reverted in: https://chromium-review.googlesource.com/1053469 The difference is this initializes ServiceWorkerNavigationHandleCore's |provider_id_| to -1. So the dtor won't use unintitialized memory when a provider was never created. Bug: 841070 Change-Id: I2ec537f76f093dd35a99f14c9033d75d50a7313d Reviewed-on: https://chromium-review.googlesource.com/1051145 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#557423} Reviewed-on: https://chromium-review.googlesource.com/1053609 Cr-Commit-Position: refs/heads/master@{#557473} [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_context_core.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_context_core.h [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_dispatcher_host.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_navigation_handle.h [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_navigation_handle_core.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_navigation_handle_core.h [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_provider_host.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_provider_host.h [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_provider_host_unittest.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/browser/service_worker/service_worker_request_handler.cc [modify] https://crrev.com/4d07d2d705fa93e09813ee0f0e0a2f9d2211bc2e/content/common/service_worker/service_worker_utils.h
,
May 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e89b4b1bce4fd814ca70513a93a1279dcac5056 commit 4e89b4b1bce4fd814ca70513a93a1279dcac5056 Author: Matt Falkenhagen <falken@chromium.org> Date: Fri May 11 07:04:57 2018 service worker: Update matching registrations for reserved clients. When a new registration is made, it attempts to notify all ServiceWorkerProviderHosts for an in-scope clients. This way navigator.serviceWorker.ready and Clients#claim etc work correctly, as the ServiceWorkerProviderHost is aware of the best matching registration. Before this CL, new registrations would miss reserved clients. So if a page was still undergoing navigation while a registration was created, the page would never be aware of the registration. So if a SW from that registration called clients.claim(), the page would not be claimed. Bug: 841070 Change-Id: I615f416ac27cadb88dcae0bde368226a92a6c481 Reviewed-on: https://chromium-review.googlesource.com/1053928 Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#557806} [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_client_utils.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_context_core.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_context_core.h [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_context_unittest.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_context_wrapper.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_job_unittest.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_provider_host_unittest.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_register_job.cc [modify] https://crrev.com/4e89b4b1bce4fd814ca70513a93a1279dcac5056/content/browser/service_worker/service_worker_registration.cc
,
May 11 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by piatek@chromium.org
, May 8 2018