New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 739895 link

Starred by 3 users
Status: Fixed
Owner:
█ holte@chromium.org
Last visit > 30 days ago
Closed: Jul 2017
Cc:
bcwh...@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
skare@chromium.org
isherman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ChromeMetricsServiceClient::InitializeSystemProfileMetrics behaves incorrectly when called twice.

Project Member Reported by holte@chromium.org, Jul 6 2017 
With UKM enabled, ChromeMetricsServiceClient::InitializeSystemProfileMetrics can be called twice, once by UMA and once by UKM.  If one has finished by the time the other is called, it seems to work fine, (though some init steps may be repeated), but if they are both started at the same time it may result in incorrect behavior.

In debug builds, this results in a DCHECK, but on release builds, it may result in one the metrics services being started before it's last initialization step is complete.

In normal circumstance, UKM is usually initialized after UMA initialization is finished, probably due to delay from waiting for sync to be initialized, but this could still potentially occur.  It can
be triggered more often when running with --force-enable-metrics-reporting, since that will start both services a soon as possible.

Comment 1 by asvitk...@chromium.org, Jul 11 2017

Cc: bcwh...@chromium.org
+bcwhite

Brian, as you're starting to ramp up on UKM, this might be a good starting bug.

Comment 2 by holte@chromium.org, Jul 11 2017 

Having looked at this a bit, here were my thoughts:

ChromeMetricsServiceClient::InitializeSystemProfileMetrics is called by the services, and then does a bunch of async init methods on a set of MetricsProviders in serial.  These metrics providers are already owned by one of the services, so it probably make sense to just move this method to be part of the MetricsProvider interface and have the services wait on the providers directly.

Hopefully none if this code actually depends on the init being serial.

Comment 3 by holte@chromium.org, Jul 18 2017

Owner: holte@chromium.org
Status: Assigned (was: Untriaged)
I'm actually just going to take ownership of this, since Brian is OOO, and I'd rather have this fixed sooner.
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 21 2017 

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

commit 915c99f3109e974c8b39364c13ec64746acaf5c7
Author: Steven Holte <holte@google.com>
Date: Fri Jul 21 23:34:31 2017

Convert InitializeSystemProfileMetrics to provider:AsyncInit

In addition to simplifying the code, this allows providers to do
their initialization in parallel.  None of the providers currently
appear to depend on the order.

May avoid double initializing some providers.

TBR=olivierrobin,manzagop,boliu

Bug:  739895 
Change-Id: I8ee97e9deace636c92b0ec27fe34e451798922bd
Reviewed-on: https://chromium-review.googlesource.com/576515
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Reviewed-by: Alexei Svitkine (slow) <asvitkine@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488790}
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/android_webview/browser/aw_metrics_service_client.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/android_webview/browser/aw_metrics_service_client.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/antivirus_metrics_provider_win.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/antivirus_metrics_provider_win.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/chrome_metrics_service_client.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/chrome_metrics_service_client.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/chromeos_metrics_provider.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/chromeos_metrics_provider.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/google_update_metrics_provider_win.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/google_update_metrics_provider_win.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/plugin_metrics_provider.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chrome/browser/metrics/plugin_metrics_provider.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chromecast/browser/metrics/cast_metrics_service_client.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/chromecast/browser/metrics/cast_metrics_service_client.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/browser_watcher/watcher_metrics_provider_win.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/browser_watcher/watcher_metrics_provider_win.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/drive_metrics_provider.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/drive_metrics_provider.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/metrics_provider.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/metrics_provider.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/metrics_service.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/metrics_service_client.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/test_metrics_service_client.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/metrics/test_metrics_service_client.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/components/ukm/ukm_service.cc
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm
[modify] https://crrev.com/915c99f3109e974c8b39364c13ec64746acaf5c7/ios/chrome/today_extension/today_metrics_logger.mm

Comment 5 by holte@chromium.org, Jul 26 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment