Fix --force-fieldtrials= changing group of unrelated trials.

The command-line flag would turn off UMA, so as not to bias server-
side data. But this had the consequence of changing how all field
trials are randomized (causing the low entropy source to be used),
which would cause unintended side-effects and in particular make it
very hard to effectively debug trial behavior.

This change updates the logic so that specifying that flag does not
change the state of UMA consent, but instead changes the state of
UMA reporting. This way, field trials are still randomized the
same way, but UMA data is not reported.

Includes a unit test.

Bug: 932273
Change-Id: I4216bb23a9784d963f30d0e2516e1e4d31784ab7
Reviewed-on: https://chromium-review.googlesource.com/c/1479632Reviewed-by: Jesse Doherty <jwd@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634885}

chrome/browser/metrics/chrome_metrics_service_accessor.h

@@ -121,6 +121,8 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
   friend class metrics::UkmConsentParamBrowserTest;
   FRIEND_TEST_ALL_PREFIXES(ChromeMetricsServiceAccessorTest,
                            MetricsReportingEnabled);
+  FRIEND_TEST_ALL_PREFIXES(ChromeMetricsServicesManagerClientTest,
+                           ForceTrialsDisablesReporting);
 
   // Returns true if metrics reporting is enabled. This does NOT necessary mean
   // that it is active as configuration may prevent it on some devices (i.e.

chrome/browser/metrics/chrome_metrics_services_manager_client.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/metrics/chrome_metrics_services_manager_client.h"
 
+#include <map>
+#include <string>
+
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
@@ -88,12 +91,24 @@ void AppendSamplingTrialGroup(const std::string& group_name,
 
 // Only clients that were given an opt-out metrics-reporting consent flow are
 // eligible for sampling.
-bool IsClientEligibleForSampling() {
-  return metrics::GetMetricsReportingDefaultState(
-             g_browser_process->local_state()) ==
+bool IsClientEligibleForSampling(PrefService* local_state) {
+  return metrics::GetMetricsReportingDefaultState(local_state) ==
          metrics::EnableMetricsDefault::OPT_OUT;
 }
 
+// Implementation of IsClientInSample() that takes a PrefService param.
+bool IsClientInSampleImpl(PrefService* local_state) {
+  // Only some clients are eligible for sampling. Clients that aren't eligible
+  // will always be considered "in sample". In this case, we don't want the
+  // feature state queried, because we don't want the field trial that controls
+  // sampling to be reported as active.
+  if (!IsClientEligibleForSampling(local_state))
+    return true;
+
+  return base::FeatureList::IsEnabled(
+      metrics::internal::kMetricsReportingFeature);
+}
+
 #if defined(OS_CHROMEOS)
 // Callback to update the metrics reporting state when the Chrome OS metrics
 // reporting setting changes.
@@ -129,8 +144,8 @@ class ChromeMetricsServicesManagerClient::ChromeEnabledStateProvider
   }
 
   bool IsReportingEnabled() const override {
-    return IsConsentGiven() &&
-           ChromeMetricsServicesManagerClient::IsClientInSample();
+    return metrics::EnabledStateProvider::IsReportingEnabled() &&
+           IsClientInSampleImpl(local_state_);
   }
 
  private:
@@ -194,22 +209,14 @@ void ChromeMetricsServicesManagerClient::CreateFallbackSamplingTrial(
 
 // static
 bool ChromeMetricsServicesManagerClient::IsClientInSample() {
-  // Only some clients are eligible for sampling. Clients that aren't eligible
-  // will always be considered "in sample". In this case, we don't want the
-  // feature state queried, because we don't want the field trial that controls
-  // sampling to be reported as active.
-  if (!IsClientEligibleForSampling())
-    return true;
-
-  return base::FeatureList::IsEnabled(
-      metrics::internal::kMetricsReportingFeature);
+  return IsClientInSampleImpl(g_browser_process->local_state());
 }
 
 // static
 bool ChromeMetricsServicesManagerClient::GetSamplingRatePerMille(int* rate) {
   // The population that is NOT eligible for sampling in considered "in sample",
   // but does not have a defined sample rate.
-  if (!IsClientEligibleForSampling())
+  if (!IsClientEligibleForSampling(g_browser_process->local_state()))
     return false;
 
   std::string rate_str = variations::GetVariationParamValueByFeature(
@@ -233,6 +240,11 @@ void ChromeMetricsServicesManagerClient::OnCrosSettingsCreated() {
 }
 #endif
 
+const metrics::EnabledStateProvider&
+ChromeMetricsServicesManagerClient::GetEnabledStateProviderForTesting() {
+  return *enabled_state_provider_;
+}
+
 std::unique_ptr<rappor::RapporServiceImpl>
 ChromeMetricsServicesManagerClient::CreateRapporServiceImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());

chrome/browser/metrics/chrome_metrics_services_manager_client.h

@@ -64,6 +64,9 @@ class ChromeMetricsServicesManagerClient
   void OnCrosSettingsCreated();
 #endif
 
+  // Accessor for the EnabledStateProvider instance used by this object.
+  const metrics::EnabledStateProvider& GetEnabledStateProviderForTesting();
+
  private:
   // This is defined as a member class to get access to
   // ChromeMetricsServiceAccessor through ChromeMetricsServicesManagerClient's

chrome/browser/metrics/chrome_metrics_services_manager_client_unittest.cc

+// Copyright 2019 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 "chrome/browser/metrics/chrome_metrics_services_manager_client.h"
+
+#include "base/base_switches.h"
+#include "base/command_line.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "components/metrics/enabled_state_provider.h"
+#include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/metrics_reporting_default_state.h"
+#include "components/metrics/metrics_service_accessor.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+TEST(ChromeMetricsServicesManagerClientTest, ForceTrialsDisablesReporting) {
+  TestingPrefServiceSimple local_state;
+
+  metrics::RegisterMetricsReportingStatePrefs(local_state.registry());
+
+  // First, test with UMA reporting setting defaulting to off.
+  local_state.registry()->RegisterBooleanPref(
+      metrics::prefs::kMetricsReportingEnabled, false);
+  // Force the pref to be used, even in unofficial builds.
+  ChromeMetricsServiceAccessor::SetForceIsMetricsReportingEnabledPrefLookup(
+      true);
+
+  ChromeMetricsServicesManagerClient client(&local_state);
+  const metrics::EnabledStateProvider& provider =
+      client.GetEnabledStateProviderForTesting();
+  metrics_services_manager::MetricsServicesManagerClient* base_client = &client;
+
+  // The provider and client APIs should agree.
+  EXPECT_EQ(provider.IsConsentGiven(), base_client->IsMetricsConsentGiven());
+  EXPECT_EQ(provider.IsReportingEnabled(),
+            base_client->IsMetricsReportingEnabled());
+
+  // Both consent and reporting should be false.
+  EXPECT_FALSE(provider.IsConsentGiven());
+  EXPECT_FALSE(provider.IsReportingEnabled());
+
+  // Set the pref to true.
+  local_state.SetBoolean(metrics::prefs::kMetricsReportingEnabled, true);
+
+  // The provider and client APIs should agree.
+  EXPECT_EQ(provider.IsConsentGiven(), base_client->IsMetricsConsentGiven());
+  EXPECT_EQ(provider.IsReportingEnabled(),
+            base_client->IsMetricsReportingEnabled());
+
+  // Both consent and reporting should be true.
+  EXPECT_TRUE(provider.IsConsentGiven());
+  EXPECT_TRUE(provider.IsReportingEnabled());
+
+  // Set --force-fieldtrials= command-line flag, which should disable reporting
+  // but not consent.
+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kForceFieldTrials, "Foo/Bar");
+
+  // The provider and client APIs should agree.
+  EXPECT_EQ(provider.IsConsentGiven(), base_client->IsMetricsConsentGiven());
+  EXPECT_EQ(provider.IsReportingEnabled(),
+            base_client->IsMetricsReportingEnabled());
+
+  // Consent should be true but reporting should be false.
+  EXPECT_TRUE(provider.IsConsentGiven());
+  EXPECT_FALSE(provider.IsReportingEnabled());
+}

chrome/test/BUILD.gn

@@ -2669,6 +2669,7 @@ test("unit_tests") {
     "../browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc",
     "../browser/metrics/chrome_metrics_service_accessor_unittest.cc",
     "../browser/metrics/chrome_metrics_service_client_unittest.cc",
+    "../browser/metrics/chrome_metrics_services_manager_client_unittest.cc",
     "../browser/metrics/oom/out_of_memory_reporter_unittest.cc",
     "../browser/metrics/process_memory_metrics_emitter_unittest.cc",
     "../browser/metrics/subprocess_metrics_provider_unittest.cc",

components/metrics/enabled_state_provider.cc

@@ -4,10 +4,18 @@
 
 #include "components/metrics/enabled_state_provider.h"
 
+#include "base/base_switches.h"
+#include "base/command_line.h"
+
 namespace metrics {
 
 bool EnabledStateProvider::IsReportingEnabled() const {
-  return IsConsentGiven();
+  // Disable metrics reporting when field trials are forced, as otherwise this
+  // would pollute experiment results in UMA. Note: This is done here and not
+  // in IsConsentGiven() as we do not want this to affect field trial entropy
+  // logic (i.e. high entropy source should still be used if UMA is on).
+  return IsConsentGiven() && !base::CommandLine::ForCurrentProcess()->HasSwitch(
+                                 switches::kForceFieldTrials);
 }
 
 }  // namespace metrics

components/metrics/metrics_service_accessor.cc

@@ -17,12 +17,7 @@ namespace {
 bool g_force_official_enabled_test = false;
 
 bool IsMetricsReportingEnabledForOfficialBuild(PrefService* pref_service) {
-  // In official builds, disable metrics when reporting field trials are
-  // forced; otherwise, use the value of the user's preference to determine
-  // whether to enable metrics reporting.
-  return !base::CommandLine::ForCurrentProcess()->HasSwitch(
-             switches::kForceFieldTrials) &&
-         pref_service->GetBoolean(prefs::kMetricsReportingEnabled);
+  return pref_service->GetBoolean(prefs::kMetricsReportingEnabled);
 }
 
 }  // namespace