Fix enabling UKM setting not persisting.

This exposed a bug in the existing implementation (see test failures in patchset 1) since the UKM enabled state was being set based on the last profile's state instead of all profiles.

Bug: 1107439, 1088488
Change-Id: I8191e69f4abfa7a7a0e0fab7093378372b00310a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308938
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790896}

components/embedder_support/android/metrics/android_metrics_service_client.cc

@@ -339,10 +339,7 @@ AndroidMetricsServiceClient::CreateLowEntropyProvider() {
   return metrics_state_manager_->CreateLowEntropyProvider();
 }
 
-void AndroidMetricsServiceClient::EnableUkm(bool enable) {
-  bool must_purge = ukm_enabled_ && !enable;
-  ukm_enabled_ = enable;
-
+void AndroidMetricsServiceClient::UpdateUkm(bool must_purge) {
   if (!ukm_service_)
     return;
   if (must_purge) {
@@ -479,7 +476,7 @@ base::TimeDelta AndroidMetricsServiceClient::GetStandardUploadInterval() {
 }
 
 bool AndroidMetricsServiceClient::IsUkmAllowedForAllProfiles() {
-  return ukm_enabled_;
+  return false;
 }
 
 bool AndroidMetricsServiceClient::ShouldStartUpFastForTesting() const {

components/embedder_support/android/metrics/android_metrics_service_client.h

@@ -107,8 +107,10 @@ class AndroidMetricsServiceClient : public MetricsServiceClient,
   std::unique_ptr<const base::FieldTrial::EntropyProvider>
   CreateLowEntropyProvider();
 
-  // Enables or disables URL-Keyed Metrics. This is disabled by default.
-  void EnableUkm(bool enable);
+  // Updates the state of whether UKM is enabled or not by calling back into
+  // IsUkmAllowedForAllProfiles(). If |must_purge| is true then currently
+  // collected data will be purged.
+  void UpdateUkm(bool must_purge);
 
   // Updates the state of the UKM service if it's running. This should be called
   // when a BrowserContext is created or destroyed which would change the value
@@ -248,7 +250,6 @@ class AndroidMetricsServiceClient : public MetricsServiceClient,
   bool app_consent_ = false;
   bool is_in_sample_ = false;
   bool fast_startup_for_testing_ = false;
-  bool ukm_enabled_ = false;
 
   // When non-zero, this overrides the default value in
   // GetStandardUploadInterval().

weblayer/browser/android/metrics/weblayer_metrics_service_client.cc

@@ -20,6 +20,7 @@
 #include "content/public/browser/browser_context.h"
 #include "google_apis/google_api_keys.h"
 #include "weblayer/browser/android/metrics/weblayer_metrics_service_accessor.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/java/jni/MetricsServiceClient_jni.h"
 #include "weblayer/browser/system_network_context_manager.h"
 #include "weblayer/browser/tab_impl.h"
@@ -124,6 +125,14 @@ int32_t WebLayerMetricsServiceClient::GetProduct() {
   return metrics::ChromeUserMetricsExtension::ANDROID_WEBLAYER;
 }
 
+bool WebLayerMetricsServiceClient::IsUkmAllowedForAllProfiles() {
+  for (auto* profile : ProfileImpl::GetAllProfiles()) {
+    if (!profile->GetBooleanSetting(SettingType::UKM_ENABLED))
+      return false;
+  }
+  return true;
+}
+
 std::string WebLayerMetricsServiceClient::GetUploadSigningKey() {
   std::string decoded_key;
   base::Base64Decode(google_apis::GetMetricsKey(), &decoded_key);
@@ -164,8 +173,7 @@ bool WebLayerMetricsServiceClient::EnablePersistentHistograms() {
 }
 
 bool WebLayerMetricsServiceClient::IsOffTheRecordSessionActive() {
-  auto profiles = ProfileImpl::GetAllProfiles();
-  for (auto* profile : profiles) {
+  for (auto* profile : ProfileImpl::GetAllProfiles()) {
     if (profile->GetBrowserContext()->IsOffTheRecord())
       return true;
   }

weblayer/browser/android/metrics/weblayer_metrics_service_client.h

@@ -39,6 +39,7 @@ class WebLayerMetricsServiceClient
 
   // metrics::MetricsServiceClient
   int32_t GetProduct() override;
+  bool IsUkmAllowedForAllProfiles() override;
   std::string GetUploadSigningKey() override;
 
   // metrics::AndroidMetricsServiceClient:

weblayer/browser/browser_context_impl.cc

@@ -69,6 +69,11 @@ void BindWakeLockProvider(
 
 }  // namespace
 
+namespace prefs {
+// Used to persist the public SettingType::UKM_ENABLED API.
+const char kUkmEnabled[] = "weblayer.ukm_enabled";
+}  // namespace prefs
+
 class ResourceContextImpl : public content::ResourceContext {
  public:
   ResourceContextImpl() = default;
@@ -242,6 +247,8 @@ void BrowserContextImpl::CreateUserPrefService() {
 
 void BrowserContextImpl::RegisterPrefs(
     user_prefs::PrefRegistrySyncable* pref_registry) {
+  pref_registry->RegisterBooleanPref(prefs::kUkmEnabled, false);
+
   // This pref is used by captive_portal::CaptivePortalService (as well as other
   // potential use cases in the future, as it is used for various purposes
   // through //chrome).
@@ -258,7 +265,7 @@ void BrowserContextImpl::RegisterPrefs(
   blocked_content::SafeBrowsingTriggeredPopupBlocker::RegisterProfilePrefs(
       pref_registry);
   pref_registry->RegisterBooleanPref(
-      prefs::kOfferTranslateEnabled, true,
+      ::prefs::kOfferTranslateEnabled, true,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 #if defined(OS_ANDROID)
   cdm::MediaDrmStorageImpl::RegisterProfilePrefs(pref_registry);

weblayer/browser/browser_context_impl.h

@@ -21,6 +21,11 @@ namespace weblayer {
 class ProfileImpl;
 class ResourceContextImpl;
 
+namespace prefs {
+// WebLayer specific pref names.
+extern const char kUkmEnabled[];
+}  // namespace prefs
+
 class BrowserContextImpl : public content::BrowserContext {
  public:
   BrowserContextImpl(ProfileImpl* profile_impl, const base::FilePath& path);

weblayer/browser/browser_impl.cc

@@ -12,9 +12,9 @@
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "components/base32/base32.h"
-#include "content/public/browser/browser_context.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/web_preferences.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/browser_list.h"
 #include "weblayer/browser/feature_list_creator.h"
 #include "weblayer/browser/persistence/browser_persister.h"

weblayer/browser/persistence/browser_persistence_common.cc

@@ -9,11 +9,11 @@
 #include "components/sessions/core/session_command.h"
 #include "components/sessions/core/session_service_commands.h"
 #include "components/sessions/core/session_types.h"
-#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_url_handler.h"
 #include "content/public/browser/dom_storage_context.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/storage_partition.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/browser_impl.h"
 #include "weblayer/browser/profile_impl.h"
 #include "weblayer/browser/tab_impl.h"

weblayer/browser/persistence/browser_persister.cc

@@ -18,13 +18,13 @@
 #include "components/sessions/core/session_constants.h"
 #include "components/sessions/core/session_id.h"
 #include "components/sessions/core/session_types.h"
-#include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/restore_type.h"
 #include "content/public/browser/session_storage_namespace.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/browser_impl.h"
 #include "weblayer/browser/persistence/browser_persistence_common.h"
 #include "weblayer/browser/profile_impl.h"

weblayer/browser/profile_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "build/build_config.h"
+#include "weblayer/browser/profile_impl.h"
+#include "weblayer/test/weblayer_browser_test.h"
+
+namespace weblayer {
+
+using ProfileBrowsertest = WebLayerBrowserTest;
+
+// TODO(crbug.com/654704): Android does not support PRE_ tests.
+#if !defined(OS_ANDROID)
+
+// UKM enabling via Profile persists across restarts.
+IN_PROC_BROWSER_TEST_F(ProfileBrowsertest, PRE_PersistUKM) {
+  GetProfile()->SetBooleanSetting(SettingType::UKM_ENABLED, true);
+}
+
+IN_PROC_BROWSER_TEST_F(ProfileBrowsertest, PersistUKM) {
+  ASSERT_TRUE(GetProfile()->GetBooleanSetting(SettingType::UKM_ENABLED));
+}
+
+#endif  // !defined(OS_ANDROID)
+
+}  // namespace weblayer

weblayer/browser/profile_impl.cc

@@ -170,6 +170,10 @@ ProfileImpl::ProfileImpl(const std::string& name)
   // Ensure WebCacheManager is created so that it starts observing
   // OnRenderProcessHostCreated events.
   web_cache::WebCacheManager::GetInstance();
+
+#if defined(OS_ANDROID)
+  WebLayerMetricsServiceClient::GetInstance()->UpdateUkm(false);
+#endif
 }
 
 ProfileImpl::~ProfileImpl() {
@@ -197,7 +201,7 @@ void ProfileImpl::RemoveProfileObserver(ProfileObserver* observer) {
   GetObservers().RemoveObserver(observer);
 }
 
-content::BrowserContext* ProfileImpl::GetBrowserContext() {
+BrowserContextImpl* ProfileImpl::GetBrowserContext() {
   if (browser_context_)
     return browser_context_.get();
 
@@ -360,9 +364,7 @@ std::unique_ptr<ProfileImpl> ProfileImpl::DestroyAndDeleteDataFromDisk(
 void ProfileImpl::OnProfileMarked(std::unique_ptr<ProfileImpl> profile,
                                   base::OnceClosure done_callback) {
   // Try to finish all writes and remove all data before nuking the profile.
-  static_cast<BrowserContextImpl*>(profile->GetBrowserContext())
-      ->pref_service()
-      ->CommitPendingWrite();
+  profile->GetBrowserContext()->pref_service()->CommitPendingWrite();
 
   // Unretained is safe here because DataClearer is owned by
   // BrowserContextImpl which is owned by this.
@@ -502,6 +504,7 @@ base::FilePath ProfileImpl::GetBrowserPersisterDataBaseDir() const {
 }
 
 void ProfileImpl::SetBooleanSetting(SettingType type, bool value) {
+  auto* pref_service = GetBrowserContext()->pref_service();
   switch (type) {
     case SettingType::BASIC_SAFE_BROWSING_ENABLED:
       basic_safe_browsing_enabled_ = value;
@@ -511,50 +514,51 @@ void ProfileImpl::SetBooleanSetting(SettingType type, bool value) {
           ->SetSafeBrowsingDisabled(!basic_safe_browsing_enabled_);
 #endif
       break;
-    case SettingType::UKM_ENABLED:
-      ukm_enabled_ = value;
+    case SettingType::UKM_ENABLED: {
 #if defined(OS_ANDROID)
-      WebLayerMetricsServiceClient::GetInstance()->EnableUkm(ukm_enabled_);
+      bool old_value = pref_service->GetBoolean(prefs::kUkmEnabled);
+#endif
+      pref_service->SetBoolean(prefs::kUkmEnabled, value);
+#if defined(OS_ANDROID)
+      // Trigger a purge if the current state no longer allows UKM.
+      bool must_purge = old_value && !value;
+      WebLayerMetricsServiceClient::GetInstance()->UpdateUkm(must_purge);
 #endif
       break;
+    }
     case SettingType::EXTENDED_REPORTING_SAFE_BROWSING_ENABLED:
 #if defined(OS_ANDROID)
-      static_cast<BrowserContextImpl*>(GetBrowserContext())
-          ->pref_service()
-          ->SetBoolean(prefs::kSafeBrowsingScoutReportingEnabled, value);
+      pref_service->SetBoolean(::prefs::kSafeBrowsingScoutReportingEnabled,
+                               value);
 #endif
       break;
     case SettingType::REAL_TIME_SAFE_BROWSING_ENABLED:
 #if defined(OS_ANDROID)
-      static_cast<BrowserContextImpl*>(GetBrowserContext())
-          ->pref_service()
-          ->SetBoolean(
-              unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
-              value);
+      pref_service->SetBoolean(
+          unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled,
+          value);
 #endif
       break;
   }
 }
 
 bool ProfileImpl::GetBooleanSetting(SettingType type) {
+  auto* pref_service = GetBrowserContext()->pref_service();
   switch (type) {
     case SettingType::BASIC_SAFE_BROWSING_ENABLED:
       return basic_safe_browsing_enabled_;
     case SettingType::UKM_ENABLED:
-      return ukm_enabled_;
+      return pref_service->GetBoolean(prefs::kUkmEnabled);
     case SettingType::EXTENDED_REPORTING_SAFE_BROWSING_ENABLED:
 #if defined(OS_ANDROID)
-      return static_cast<BrowserContextImpl*>(GetBrowserContext())
-          ->pref_service()
-          ->GetBoolean(prefs::kSafeBrowsingScoutReportingEnabled);
+      return pref_service->GetBoolean(
+          ::prefs::kSafeBrowsingScoutReportingEnabled);
 #endif
       return false;
     case SettingType::REAL_TIME_SAFE_BROWSING_ENABLED:
 #if defined(OS_ANDROID)
-      return static_cast<BrowserContextImpl*>(GetBrowserContext())
-          ->pref_service()
-          ->GetBoolean(
-              unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
+      return pref_service->GetBoolean(
+          unified_consent::prefs::kUrlKeyedAnonymizedDataCollectionEnabled);
 #endif
       return false;
   }

weblayer/browser/profile_impl.h

@@ -63,7 +63,7 @@ class ProfileImpl : public Profile {
   static void AddProfileObserver(ProfileObserver* observer);
   static void RemoveProfileObserver(ProfileObserver* observer);
 
-  content::BrowserContext* GetBrowserContext();
+  BrowserContextImpl* GetBrowserContext();
 
   // Called when the download subsystem has finished initializing. By this point
   // information about downloads that were interrupted by a previous crash would
@@ -72,6 +72,7 @@ class ProfileImpl : public Profile {
 
   // Path data is stored at, empty if off-the-record.
   const base::FilePath& data_path() const { return info_.data_path; }
+  const std::string& name() const { return info_.name; }
   DownloadDelegate* download_delegate() { return download_delegate_; }
 
   // Profile implementation:
@@ -158,7 +159,6 @@ class ProfileImpl : public Profile {
   std::unique_ptr<CookieManagerImpl> cookie_manager_;
 
   bool basic_safe_browsing_enabled_ = true;
-  bool ukm_enabled_ = false;
 
 #if defined(OS_ANDROID)
   base::android::ScopedJavaGlobalRef<jobject> java_profile_;

weblayer/browser/site_isolation_browsertest.cc

@@ -10,13 +10,13 @@
 #include "components/site_isolation/features.h"
 #include "components/site_isolation/pref_names.h"
 #include "components/user_prefs/user_prefs.h"
-#include "content/public/browser/browser_context.h"
 #include "content/public/browser/site_isolation_policy.h"
 #include "content/public/common/content_client.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "net/dns/mock_host_resolver.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/browser_impl.h"
 #include "weblayer/browser/content_browser_client_impl.h"
 #include "weblayer/browser/profile_impl.h"

weblayer/browser/tab_impl.cc

@@ -50,6 +50,7 @@
 #include "third_party/blink/public/mojom/window_features/window_features.mojom.h"
 #include "ui/base/window_open_disposition.h"
 #include "weblayer/browser/autofill_client_impl.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/browser_impl.h"
 #include "weblayer/browser/browser_process.h"
 #include "weblayer/browser/content_browser_client_impl.h"

weblayer/browser/translate_browsertest.cc

@@ -12,6 +12,7 @@
 #include "net/base/mock_network_change_notifier.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "weblayer/browser/browser_context_impl.h"
 #include "weblayer/browser/profile_impl.h"
 #include "weblayer/browser/tab_impl.h"
 #include "weblayer/browser/translate_client_impl.h"

weblayer/browser/ukm_browsertest.cc

@@ -7,7 +7,6 @@
 #include "weblayer/browser/android/metrics/weblayer_metrics_service_client.h"
 #include "weblayer/browser/profile_impl.h"
 #include "weblayer/public/navigation_controller.h"
-#include "weblayer/public/profile.h"
 #include "weblayer/public/tab.h"
 #include "weblayer/shell/android/browsertests_apk/metrics_test_helper.h"
 #include "weblayer/shell/browser/shell.h"
@@ -15,6 +14,19 @@
 
 namespace weblayer {
 
+namespace {
+
+ProfileImpl* GetProfileByName(const std::string& name) {
+  for (auto* profile : ProfileImpl::GetAllProfiles()) {
+    if (profile->name() == name)
+      return profile;
+  }
+
+  return nullptr;
+}
+
+}  // namespace
+
 class UkmBrowserTest : public WebLayerBrowserTest {
  public:
   void SetUp() override {
@@ -79,6 +91,7 @@ IN_PROC_BROWSER_TEST_F(UkmBrowserTest, RegularPlusIncognitoCheck) {
 
   // Creating another regular profile mustn't enable UKM.
   CreateProfile("foo");
+  GetProfileByName("foo")->SetBooleanSetting(SettingType::UKM_ENABLED, true);
   EXPECT_FALSE(ukm_test_helper.IsRecordingEnabled());
 
   // Note WebLayer can only have one incognito profile so we can't test creating
@@ -104,6 +117,7 @@ IN_PROC_BROWSER_TEST_F(UkmBrowserTest, IncognitoPlusRegularCheck) {
   EXPECT_FALSE(ukm_test_helper.IsRecordingEnabled());
 
   CreateProfile("foo");
+  GetProfileByName("foo")->SetBooleanSetting(SettingType::UKM_ENABLED, true);
   EXPECT_FALSE(ukm_test_helper.IsRecordingEnabled());
 
   DestroyProfile(std::string());

weblayer/public/profile.h

@@ -10,6 +10,7 @@
 
 #include "base/callback_forward.h"
 #include "base/containers/flat_set.h"
+#include "base/time/time.h"
 
 namespace base {
 class FilePath;

weblayer/test/BUILD.gn

@@ -132,6 +132,7 @@ test("weblayer_browsertests") {
     "../browser/page_load_metrics_browsertest.cc",
     "../browser/popup_blocker_browsertest.cc",
     "../browser/prefetch_browsertest.cc",
+    "../browser/profile_browsertest.cc",
     "../browser/site_isolation_browsertest.cc",
     "../browser/ssl_browsertest.cc",
     "../browser/translate_browsertest.cc",