UseCounter counts during main resource load don't get propagated to the page |
|||
Issue description
If the service worker uses a feature during the fetch event handler for the main resource load, it doesn't get propagated to the controllee.
Example:
addEventListener('fetch', e => {
e.isReload;
});
We get here:
ServiceWorkerVersion::CountFeature
ServiceWorkerProviderHost::CountFeature
(sends CountFeature IPC)
ServiceWorkerDispatcher::OnCountFeature
But in OnCountFeature the WebServiceWorkerProviderClient has not yet been created because the document does not yet exist, so the message is ignored.
This could possibly mean features are under-counted.
,
Jun 28 2017
Hm, that didn't happen in my test, which just timed out. https://chromium-review.googlesource.com/c/549539/ Maybe internals gets confused?
,
Jun 28 2017
Who tells ServiceWorkerProviderContext about the count? In my tracing First, ServiceWorkerDispatcher::OnCountFeature is called for provider and the feature. But the WebServiceWorkerProviderClient for the provider does not exist, so this is ignored. Second, content::WebServiceWorkerProviderImpl::SetClient is called after the document is created and ServiceWorkerContainer is created. context_->provider_id() equals the same provider id as in OnCountFeature. But context_ doesn't have the feature counted in used_features. SetClient seems to want to copy the used_features from the service worker's provider context, but it's getting it from the controllee's provider context, no?
,
Jun 28 2017
The sequence is: 1. ServiceWorkerProviderContext::OnSetControllerServiceWorker is called. The service worker has not yet used the feature because the fetch event hasn't run. 2. ServiceWorkerDispatcher::OnCountFeature is called for provider and the feature. But the WebServiceWorkerProviderClient for the provider does not exist, so this is ignored. 3. content::WebServiceWorkerProviderImpl::SetClient is called after the document is created and ServiceWorkerContainer is created. It copies ServiceWorkerProviderContext's used features. But ServiceWorkerProviderContext never counted the feature.
,
Jun 28 2017
Re c#4: > But ServiceWorkerProviderContext never counted the feature. As we chatted offline, ServiceWorkerDispatcher::OnCountFeature may need to count usage not only in ServiceWorkerProviderClient but also in ServiceWorkerProviderContext. Can you try it?
,
Jun 28 2017
Like this:
void ServiceWorkerDispatcher::OnCountFeature(int thread_id,
int provider_id,
uint32_t feature) {
ProviderContextMap::iterator provider = provider_contexts_.find(provider_id);
if (provider != provider_contexts_.end())
provider->second->CountFeature(feature);
ProviderClientMap::iterator found = provider_clients_.find(provider_id);
if (found != provider_clients_.end())
found->second->CountFeature(feature);
}
void ServiceWorkerProviderContext::CountFeature(uint32_t feature) {
DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
used_features_.insert(feature);
}
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/900dcae2261ada401d6244235def48dedb976619 commit 900dcae2261ada401d6244235def48dedb976619 Author: Matt Falkenhagen <falken@chromium.org> Date: Wed Jun 28 10:13:22 2017 service worker: Add UseCounter for FetchEvent#isReload This feature is at-risk and we need to measure usage. This patch also fixes a bug where use counter for a page wasn't updated for features used by the service worker during the main resource load. Bug: 652994, 737355 Change-Id: Ib62e6c3ea4d1f39345a978c6c8c6c752fc1bd022 Reviewed-on: https://chromium-review.googlesource.com/549539 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#482935} [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/content/child/service_worker/service_worker_dispatcher.cc [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/content/child/service_worker/service_worker_provider_context.cc [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/content/child/service_worker/service_worker_provider_context.h [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/content/child/service_worker/web_service_worker_provider_impl.cc [add] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/use-isReload-worker.js [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/usecounter-window.html [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.cpp [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/third_party/WebKit/public/platform/WebFeature.h [modify] https://crrev.com/900dcae2261ada401d6244235def48dedb976619/tools/metrics/histograms/enums.xml
,
Jun 28 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by nhiroki@chromium.org
, Jun 28 2017In the case, usage counts are notified when ServiceWorkerContainer is created as follows: ServiceWorkerContainer::ServiceWorkerContainer WebServiceWorkerProviderImpl::SetClient (WebServiceWorkerProvider::SetClient) ServiceWorkerContainer::CountFeature (WebServiceWorkerProviderClient::CountFeature) Until the container gets created, ServiceWorkerProviderContext keeps the counts instead (see ServiceWorkerProviderContext::OnSetControllerServiceWorker)