Disable UKM when we couldn't register observers

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4bdb81dd016abb71951a08c86f965a4128be0eed
Reviewed-on: https://chromium-review.googlesource.com/887300Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550424}

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -418,9 +418,10 @@ ChromeMetricsServiceClient::ChromeMetricsServiceClient(
       weak_ptr_factory_(this) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   RecordCommandLineMetrics();
-  RegisterForNotifications();
+  notification_listeners_active_ = RegisterForNotifications();
 #if defined(OS_ANDROID)
   incognito_observer_ = std::make_unique<AndroidIncognitoObserver>(this);
+  notification_listeners_active_ &= (incognito_observer_ != nullptr);
 #endif
 }
 
@@ -850,7 +851,7 @@ void ChromeMetricsServiceClient::RecordCommandLineMetrics() {
                            switch_count - common_commands);
 }
 
-void ChromeMetricsServiceClient::RegisterForNotifications() {
+bool ChromeMetricsServiceClient::RegisterForNotifications() {
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_OPENED,
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_CLOSED,
@@ -878,13 +879,18 @@ void ChromeMetricsServiceClient::RegisterForNotifications() {
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::NotificationService::AllBrowserContextsAndSources());
+
+  bool all_profiles_succeeded = true;
   for (Profile* profile :
        g_browser_process->profile_manager()->GetLoadedProfiles()) {
-    RegisterForProfileEvents(profile);
+    if (!RegisterForProfileEvents(profile)) {
+      all_profiles_succeeded = false;
+    }
   }
+  return all_profiles_succeeded;
 }
 
-void ChromeMetricsServiceClient::RegisterForProfileEvents(Profile* profile) {
+bool ChromeMetricsServiceClient::RegisterForProfileEvents(Profile* profile) {
   // Register chrome://ukm handler
   content::URLDataSource::Add(
       profile, new ukm::debug::DebugPage(base::Bind(
@@ -894,7 +900,8 @@ void ChromeMetricsServiceClient::RegisterForProfileEvents(Profile* profile) {
   // deletion.
   if (chromeos::ProfileHelper::IsSigninProfile(profile) ||
       chromeos::ProfileHelper::IsLockScreenAppProfile(profile)) {
-    return;
+    // No listeners, but still a success case.
+    return true;
   }
 #endif
 #if defined(OS_WIN) || defined(OS_MACOSX) || \
@@ -908,11 +915,19 @@ void ChromeMetricsServiceClient::RegisterForProfileEvents(Profile* profile) {
   history::HistoryService* history_service =
       HistoryServiceFactory::GetForProfile(profile,
                                            ServiceAccessType::IMPLICIT_ACCESS);
+  if (!history_service) {
+    return false;
+  }
+
   ObserveServiceForDeletions(history_service);
+
   browser_sync::ProfileSyncService* sync =
       ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
-  if (sync)
-    ObserveServiceForSyncDisables(static_cast<syncer::SyncService*>(sync));
+  if (!sync) {
+    return false;
+  }
+  ObserveServiceForSyncDisables(static_cast<syncer::SyncService*>(sync));
+  return true;
 }
 
 void ChromeMetricsServiceClient::Observe(
@@ -937,9 +952,15 @@ void ChromeMetricsServiceClient::Observe(
       metrics_service_->OnApplicationNotIdle();
       break;
 
-    case chrome::NOTIFICATION_PROFILE_ADDED:
-      RegisterForProfileEvents(content::Source<Profile>(source).ptr());
+    case chrome::NOTIFICATION_PROFILE_ADDED: {
+      bool success =
+          RegisterForProfileEvents(content::Source<Profile>(source).ptr());
+      if (!success && notification_listeners_active_) {
+        notification_listeners_active_ = false;
+        UpdateRunningServices();
+      }
       break;
+    }
     case chrome::NOTIFICATION_PROFILE_DESTROYED:
       // May have closed last incognito window.
       UpdateRunningServices();
@@ -1035,3 +1056,17 @@ bool ChromeMetricsServiceClient::IsHistorySyncEnabledOnAllProfiles() {
 bool ChromeMetricsServiceClient::IsExtensionSyncEnabledOnAllProfiles() {
   return SyncDisableObserver::IsExtensionSyncEnabledOnAllProfiles();
 }
+
+bool g_notification_listeners_failed = false;
+void ChromeMetricsServiceClient::SetNotificationListenerSetupFailedForTesting(
+    bool simulate_failure) {
+  g_notification_listeners_failed = simulate_failure;
+}
+
+bool ChromeMetricsServiceClient::
+    AreNotificationListenersEnabledOnAllProfiles() {
+  // For testing
+  if (g_notification_listeners_failed)
+    return false;
+  return notification_listeners_active_;
+}

chrome/browser/metrics/chrome_metrics_service_client.h

@@ -89,6 +89,9 @@ class ChromeMetricsServiceClient : public metrics::MetricsServiceClient,
   bool IsUMACellularUploadLogicEnabled() override;
   bool IsHistorySyncEnabledOnAllProfiles() override;
   bool IsExtensionSyncEnabledOnAllProfiles() override;
+  bool AreNotificationListenersEnabledOnAllProfiles() override;
+  static void SetNotificationListenerSetupFailedForTesting(
+      bool simulate_failure);
 
   // ukm::HistoryDeleteObserver:
   void OnHistoryDeleted() override;
@@ -143,10 +146,12 @@ class ChromeMetricsServiceClient : public metrics::MetricsServiceClient,
   // user is performing work. This is useful to allow some features to sleep,
   // until the machine becomes active, such as precluding UMA uploads unless
   // there was recent activity.
-  void RegisterForNotifications();
+  // Returns true if registration was successful for all profiles.
+  bool RegisterForNotifications();
 
   // Call to listen for events on the selected profile's services.
-  void RegisterForProfileEvents(Profile* profile);
+  // Returns true if we registered for all notifications we wanted successfully.
+  bool RegisterForProfileEvents(Profile* profile);
 
   // content::NotificationObserver:
   void Observe(int type,
@@ -185,6 +190,9 @@ class ChromeMetricsServiceClient : public metrics::MetricsServiceClient,
   std::unique_ptr<TabModelListObserver> incognito_observer_;
 #endif  // defined(OS_ANDROID)
 
+  // Whether we registered all notification listeners successfully.
+  bool notification_listeners_active_;
+
   // A queue of tasks for initial metrics gathering. These may be asynchronous
   // or synchronous.
   base::circular_deque<base::Closure> initialize_task_queue_;

chrome/browser/metrics/ukm_browsertest.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/browser/metrics/chrome_metrics_service_client.h"
 #include "chrome/browser/metrics/chrome_metrics_services_manager_client.h"
 #include "chrome/browser/metrics/testing/metrics_reporting_pref_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -715,6 +716,23 @@ IN_PROC_BROWSER_TEST_F(UkmBrowserTest, MultiSyncSignoutCheck) {
   CloseBrowserSynchronously(browser1);
 }
 
+// Make sure that if history/sync services weren't available when we tried to
+// attach listeners, UKM is not enabled.
+IN_PROC_BROWSER_TEST_F(UkmBrowserTest, ServiceListenerInitFailedCheck) {
+  MetricsConsentOverride metrics_consent(true);
+  ChromeMetricsServiceClient::SetNotificationListenerSetupFailedForTesting(
+      true);
+
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  std::unique_ptr<ProfileSyncServiceHarness> harness =
+      EnableSyncForProfile(profile);
+
+  Browser* sync_browser = CreateBrowser(profile);
+  EXPECT_FALSE(ukm_enabled());
+  harness->service()->RequestStop(browser_sync::ProfileSyncService::CLEAR_DATA);
+  CloseBrowserSynchronously(sync_browser);
+}
+
 // Make sure that UKM is not affected by MetricsReporting Feature (sampling).
 IN_PROC_BROWSER_TEST_F(UkmBrowserTest, MetricsReportingCheck) {
   // Need to set the Metrics Default to OPT_OUT to trigger MetricsReporting.

components/metrics/metrics_service_client.cc

@@ -44,6 +44,10 @@ bool MetricsServiceClient::IsExtensionSyncEnabledOnAllProfiles() {
   return false;
 }
 
+bool MetricsServiceClient::AreNotificationListenersEnabledOnAllProfiles() {
+  return false;
+}
+
 void MetricsServiceClient::SetUpdateRunningServicesCallback(
     const base::Closure& callback) {
   update_running_services_ = callback;

components/metrics/metrics_service_client.h

@@ -119,12 +119,15 @@ class MetricsServiceClient {
   // Returns whether cellular logic is enabled for metrics reporting.
   virtual bool IsUMACellularUploadLogicEnabled();
 
-  // Returns if history sync is enabled on all active profiles.
+  // Returns whether history sync is enabled on all active profiles.
   virtual bool IsHistorySyncEnabledOnAllProfiles();
 
   // Returns if extensions sync is enabled on all active profiles.
   virtual bool IsExtensionSyncEnabledOnAllProfiles();
 
+  // Returns whether UKM notification listeners were attached to all profiles.
+  virtual bool AreNotificationListenersEnabledOnAllProfiles();
+
   // Sets the callback to run MetricsServiceManager::UpdateRunningServices.
   void SetUpdateRunningServicesCallback(const base::Closure& callback);
 

components/metrics_services_manager/metrics_services_manager.cc

@@ -138,12 +138,16 @@ void MetricsServicesManager::UpdateUkmService() {
   ukm::UkmService* ukm = GetUkmService();
   if (!ukm)
     return;
+
+  bool listeners_active =
+      GetMetricsServiceClient()->AreNotificationListenersEnabledOnAllProfiles();
   bool sync_enabled =
       client_->IsMetricsReportingForceEnabled() ||
       metrics_service_client_->IsHistorySyncEnabledOnAllProfiles();
   bool is_incognito = client_->IsIncognitoSessionActive();
 
-  if (consent_given_ && sync_enabled && !is_incognito) {
+  if (consent_given_ && listeners_active && sync_enabled && !is_incognito) {
+    // TODO(skare): revise this - merged in a big change
     ukm->EnableRecording(
         metrics_service_client_->IsExtensionSyncEnabledOnAllProfiles());
     if (may_upload_)

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h

@@ -76,6 +76,7 @@ class IOSChromeMetricsServiceClient : public IncognitoWebStateObserver,
   base::TimeDelta GetStandardUploadInterval() override;
   void OnRendererProcessCrash() override;
   bool IsHistorySyncEnabledOnAllProfiles() override;
+  bool AreNotificationListenersEnabledOnAllProfiles() override;
 
   // ukm::HistoryDeleteObserver:
   void OnHistoryDeleted() override;
@@ -108,10 +109,12 @@ class IOSChromeMetricsServiceClient : public IncognitoWebStateObserver,
   // user is performing work. This is useful to allow some features to sleep,
   // until the machine becomes active, such as precluding UMA uploads unless
   // there was recent activity.
-  void RegisterForNotifications();
+  // Returns true if registration was successful.
+  bool RegisterForNotifications();
 
   // Register to observe events on a browser state's services.
-  void RegisterForBrowserStateEvents(ios::ChromeBrowserState* browser_state);
+  // Returns true if registration was successful.
+  bool RegisterForBrowserStateEvents(ios::ChromeBrowserState* browser_state);
 
   // Called when a tab is parented.
   void OnTabParented(web::WebState* web_state);
@@ -130,6 +133,9 @@ class IOSChromeMetricsServiceClient : public IncognitoWebStateObserver,
   // The UkmService that |this| is a client of.
   std::unique_ptr<ukm::UkmService> ukm_service_;
 
+  // Whether we registered all notification listeners successfully.
+  bool notification_listeners_active_;
+
   // The IOSChromeStabilityMetricsProvider instance that was registered with
   // MetricsService. Has the same lifetime as |metrics_service_|.
   IOSChromeStabilityMetricsProvider* stability_metrics_provider_;

ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm

@@ -70,7 +70,7 @@ IOSChromeMetricsServiceClient::IOSChromeMetricsServiceClient(
       stability_metrics_provider_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  RegisterForNotifications();
+  notification_listeners_active_ = RegisterForNotifications();
 }
 
 IOSChromeMetricsServiceClient::~IOSChromeMetricsServiceClient() {
@@ -250,7 +250,7 @@ void IOSChromeMetricsServiceClient::CollectFinalHistograms() {
   collect_final_metrics_done_callback_.Run();
 }
 
-void IOSChromeMetricsServiceClient::RegisterForNotifications() {
+bool IOSChromeMetricsServiceClient::RegisterForNotifications() {
   tab_parented_subscription_ =
       TabParentingGlobalObserver::GetInstance()->RegisterCallback(
           base::Bind(&IOSChromeMetricsServiceClient::OnTabParented,
@@ -264,12 +264,16 @@ void IOSChromeMetricsServiceClient::RegisterForNotifications() {
       GetApplicationContext()
           ->GetChromeBrowserStateManager()
           ->GetLoadedBrowserStates();
+  bool all_profiles_succeeded = true;
   for (ios::ChromeBrowserState* browser_state : loaded_browser_states) {
-    RegisterForBrowserStateEvents(browser_state);
+    if (!RegisterForBrowserStateEvents(browser_state)) {
+      all_profiles_succeeded = false;
+    }
   }
+  return all_profiles_succeeded;
 }
 
-void IOSChromeMetricsServiceClient::RegisterForBrowserStateEvents(
+bool IOSChromeMetricsServiceClient::RegisterForBrowserStateEvents(
     ios::ChromeBrowserState* browser_state) {
   history::HistoryService* history_service =
       ios::HistoryServiceFactory::GetForBrowserState(
@@ -279,6 +283,7 @@ void IOSChromeMetricsServiceClient::RegisterForBrowserStateEvents(
       IOSChromeProfileSyncServiceFactory::GetInstance()->GetForBrowserState(
           browser_state);
   ObserveServiceForSyncDisables(static_cast<syncer::SyncService*>(sync));
+  return (history_service != nullptr && sync != nullptr);
 }
 
 void IOSChromeMetricsServiceClient::OnTabParented(web::WebState* web_state) {
@@ -324,3 +329,8 @@ void IOSChromeMetricsServiceClient::OnIncognitoWebStateRemoved() {
 bool IOSChromeMetricsServiceClient::IsHistorySyncEnabledOnAllProfiles() {
   return SyncDisableObserver::IsHistorySyncEnabledOnAllProfiles();
 }
+
+bool IOSChromeMetricsServiceClient::
+    AreNotificationListenersEnabledOnAllProfiles() {
+  return notification_listeners_active_;
+}