New issue
Advanced search Search tips

Issue 866335 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner:
Closed: Nov 27
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 846235



Sign in to add a comment

S13nServiceWorker: Decrease in main resource requests handled by service worker.

Project Member Reported by falken@chromium.org, Jul 23

Issue description

UMA is showing the count of attempted fetch event dispatches decreases for main resource requests, when S13nSW is on compared to off:
ServiceWorker.FetchEvent.MainResource.Status

Naturally it's also decreasing for Subresources, but we expect that is just a consequence of MainResource decreasing.
 
Description: Show this description
Description: Show this description
Summary: S13nServiceWorker: Decrease in main resource requests handled by service worker. (was: S13ServiceWorker: Decrease in main resource requests handled by service worker.)
Owner: falken@chromium.org
Status: Assigned (was: Available)
The difference is much more pronounced on Android vs Desktop:
https://uma.googleplex.com/p/chrome/timeline_v2/?sid=76def1597befa23f987e06d0eea93c35
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 7

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

commit b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Aug 07 04:16:09 2018

service worker: Add MainResourceRequestDestination UMA.

ServicifiedServiceWorker has a lower count of main resource requests
that result in a fetch event dispatch. Add UMA to try to debug the
issue.

Bug:  866335 
Change-Id: Iad5906f550722d073f0bfc6c788668aef18abd25
Reviewed-on: https://chromium-review.googlesource.com/1163353
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581129}
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_controllee_request_handler.cc
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_controllee_request_handler.h
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_metrics.cc
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_metrics.h
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_navigation_loader.h
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_url_job_wrapper.h
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/content/browser/service_worker/service_worker_url_request_job.h
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/b145a456d221b02ad0b7ab3ee73fa23eeb85ea6e/tools/metrics/histograms/histograms.xml

Google Search reports missing navigation preload requests but we couldn't repro it.
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 9

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

commit b054745cad93b18bf719d98005730c3b7e8f3b96
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Aug 09 10:52:51 2018

service worker: Remove DelegatingURLLoader.

I *think* this was added for "associated URL loaders" in
https://crrev.com/fe95d9b23ae824, which was reversed, so
I'm thinking DelegatingURLLoader isn't needed. At least, the tests
seem to pass.

We were holding on to the delegating loader on the browser side for
lifetime in URLLoaderAssets, but it looks like the renderer holds
on to its loader anyway while the nav preload is ongoing in
ServiceWorkerContextClient::NavigationPreloadRequest, so it looks
correct to rely on the renderer to do it.

This was inspired by the linked bug, but not expected to solve it.

Bug:  866335 
Change-Id: I3b87887e72613e089b99b9d4a91cc76ebc0d620d
Reviewed-on: https://chromium-review.googlesource.com/1168943
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581857}
[modify] https://crrev.com/b054745cad93b18bf719d98005730c3b7e8f3b96/content/browser/service_worker/service_worker_fetch_dispatcher.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 9

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

commit e1784f5670ba46b30d9e1e4fa9b1b09ff0a1b4a2
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Aug 09 23:29:52 2018

S13nServiceWorker: Add nav preload UMA.

This will help debug the linked bug, by comparing S13nServiceWorker on
vs off.

Bug:  866335 
Change-Id: Idd1d027971cb9fd5122cda4f1cf383b55119fa83
Reviewed-on: https://chromium-review.googlesource.com/1168952
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581955}
[modify] https://crrev.com/e1784f5670ba46b30d9e1e4fa9b1b09ff0a1b4a2/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/e1784f5670ba46b30d9e1e4fa9b1b09ff0a1b4a2/tools/metrics/histograms/histograms.xml

It turned out navigation preload was red herring: they are just missing the variation header.

Our internal metrics still show a decreased number of main resource requests totally in S13nServiceWorker. Looking at the UMA in c#6, there's no big bucket showing that requests are being dropped, it's just that the count of main resource requests that go through potential service worker interception (i.e., SW registration lookup occurs) is lower. Maybe we are filtering out some non-http requests earlier in the S13nSW path, that go through reg look up in the legacy path? Still a mystery.

https://uma.googleplex.com/p/chrome/variations/?sid=368f8e4846cacb59bb4fba76c202e991

https://uma.googleplex.com/p/chrome/variations/?sid=1beecbd8a9c8b651324f4fa1f6c608a4 

Quite strangely, the decreased count is only visible on Android. On Windows it doesn't happen.

I'm thinking optimistically it's not a bug but just a difference in counting. Another thing to try to look at is the # of total navigations, if we have such a metric.
The number of page loads (PageLoad.ParseTiming.NavigationToParseStart count) looks equal. But that counts main frame navigations and the SW UMA in question counts main frames + subframes + shared workers.
Status: WontFix (was: Assigned)
The latest numbers showed 2x increase of main resource fetch events to a service worker on Desktop, and a 5%-10% decrease on Android. The cause of these counts changing is unknown. On Desktop we saw a bump in absolute number of total main resource requests, and a slightly larger percentage of requests going to a service worker, which resulted in a 2x increase when comparing Enabled vs Control.

It could be a change in how we count fetch events (something about extensions could explain Desktop vs Android difference?).

The cause of the discrepency is still unknown but I don't intend to take further action on the bug. We'll keep ramping up the experiment and see if there are user reports or other metrics indicating problems.

Sign in to add a comment