Introduce an allowlist for external experiment ids.

The allowlist provides a way to specify which external experiment
ids can be sent and also provides a way to associate a study name
for such external experiment ids.

Since the API now uses an allowlist, its use is no longer restricted
via metrics service accessor, which allows us to simplify the plumbing.

A metrics client API is provided to disable the allowlist, which is
needed for some configurations such as WebLayer.

Additionally, the new allowlist support is implemented behind a
base::Feature, which is off by default in this CL. When the
feature is disabled, the behavior from before is unchanged.

Bug: 1111941
Change-Id: I63b92254e27a32dd5236c37f0b70fb3683f447e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2324172
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795502}

chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaSessionStats.java

@@ -208,9 +208,9 @@ public class UmaSessionStats {
         prefManager.syncUsageAndCrashReportingPrefs();
     }
 
-    public static void registerExternalExperiment(String studyName, int[] experimentIds) {
+    public static void registerExternalExperiment(String fallbackStudyName, int[] experimentIds) {
         assert isMetricsServiceAvailable();
-        UmaSessionStatsJni.get().registerExternalExperiment(studyName, experimentIds);
+        UmaSessionStatsJni.get().registerExternalExperiment(fallbackStudyName, experimentIds);
     }
 
     public static void registerSyntheticFieldTrial(String trialName, String groupName) {

chrome/browser/android/metrics/uma_session_stats.cc

@@ -11,7 +11,6 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/no_destructor.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/android/chrome_jni_headers/UmaSessionStats_jni.h"
@@ -28,8 +27,6 @@
 #include "components/metrics_services_manager/metrics_services_manager.h"
 #include "components/prefs/pref_service.h"
 #include "components/ukm/ukm_service.h"
-#include "components/variations/hashing.h"
-#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 
 using base::android::ConvertJavaStringToUTF8;
@@ -188,14 +185,6 @@ void UmaSessionStats::RegisterSyntheticFieldTrial(
                                                             group_name);
 }
 
-// static
-void UmaSessionStats::RegisterSyntheticMultiGroupFieldTrial(
-    const std::string& trial_name,
-    const std::vector<uint32_t>& group_name_hashes) {
-  ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial(
-      trial_name, group_name_hashes);
-}
-
 bool UmaSessionStats::IsBackgroundSessionStartForTesting() {
   return !GetInstance()
               ->session_time_tracker_.background_session_start_time()
@@ -286,9 +275,10 @@ static void JNI_UmaSessionStats_UpdateMetricsServiceState(
 
 static void JNI_UmaSessionStats_RegisterExternalExperiment(
     JNIEnv* env,
-    const JavaParamRef<jstring>& jtrial_name,
+    const JavaParamRef<jstring>& jfallback_study_name,
     const JavaParamRef<jintArray>& jexperiment_ids) {
-  const std::string trial_name_utf8(ConvertJavaStringToUTF8(env, jtrial_name));
+  std::string fallback_study_name(
+      ConvertJavaStringToUTF8(env, jfallback_study_name));
   std::vector<int> experiment_ids;
   // A null |jexperiment_ids| is the same as an empty list.
   if (jexperiment_ids) {
@@ -296,28 +286,11 @@ static void JNI_UmaSessionStats_RegisterExternalExperiment(
                                            &experiment_ids);
   }
 
-  UMA_HISTOGRAM_COUNTS_100("UMA.ExternalExperiment.GroupCount",
-                           experiment_ids.size());
-
-  std::vector<uint32_t> group_name_hashes;
-  group_name_hashes.reserve(experiment_ids.size());
-
-  variations::ActiveGroupId active_group;
-  active_group.name = variations::HashName(trial_name_utf8);
-  for (int experiment_id : experiment_ids) {
-    active_group.group =
-        variations::HashName(base::NumberToString(experiment_id));
-    // Since external experiments are not based on Chrome's low entropy source,
-    // they are only sent to Google web properties for signed in users to make
-    // sure that this couldn't be used to identify a user that's not signed in.
-    variations::AssociateGoogleVariationIDForceHashes(
-        variations::GOOGLE_WEB_PROPERTIES_SIGNED_IN, active_group,
-        static_cast<variations::VariationID>(experiment_id));
-    group_name_hashes.push_back(active_group.group);
-  }
-
-  UmaSessionStats::RegisterSyntheticMultiGroupFieldTrial(trial_name_utf8,
-                                                         group_name_hashes);
+  g_browser_process->metrics_service()
+      ->synthetic_trial_registry()
+      ->RegisterExternalExperiments(
+          fallback_study_name, experiment_ids,
+          variations::SyntheticTrialRegistry::kOverrideExistingIds);
 }
 
 static void JNI_UmaSessionStats_RegisterSyntheticFieldTrial(

chrome/browser/android/metrics/uma_session_stats.h

@@ -36,10 +36,6 @@ class UmaSessionStats {
   static void RegisterSyntheticFieldTrial(const std::string& trial_name,
                                           const std::string& group_name);
 
-  static void RegisterSyntheticMultiGroupFieldTrial(
-      const std::string& trial_name,
-      const std::vector<uint32_t>& group_name_hashes);
-
   static bool IsBackgroundSessionStartForTesting();
 
  private:

chrome/browser/metrics/chrome_metrics_service_accessor.cc

@@ -67,15 +67,6 @@ bool ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
       g_browser_process->metrics_service(), trial_name, group_name);
 }
 
-// static
-bool ChromeMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial(
-    base::StringPiece trial_name,
-    const std::vector<uint32_t>& group_name_hashes) {
-  return metrics::MetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial(
-      g_browser_process->metrics_service(), trial_name, group_name_hashes);
-}
-
-// static
 void ChromeMetricsServiceAccessor::SetForceIsMetricsReportingEnabledPrefLookup(
     bool value) {
   metrics::MetricsServiceAccessor::SetForceIsMetricsReportingEnabledPrefLookup(

chrome/browser/metrics/chrome_metrics_service_accessor.h

@@ -146,13 +146,6 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
   static bool RegisterSyntheticFieldTrial(base::StringPiece trial_name,
                                           base::StringPiece group_name);
 
-  // Calls MetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial() with
-  // g_browser_process->metrics_service(). See that function's declaration for
-  // details.
-  static bool RegisterSyntheticMultiGroupFieldTrial(
-      base::StringPiece trial_name,
-      const std::vector<uint32_t>& group_name_hashes);
-
   // Cover for function of same name in MetricsServiceAccssor. See
   // ChromeMetricsServiceAccessor for details.
   static void SetForceIsMetricsReportingEnabledPrefLookup(bool value);

components/metrics/metrics_service.cc

@@ -219,7 +219,9 @@ MetricsService::MetricsService(MetricsStateManager* state_manager,
       test_mode_active_(false),
       state_(INITIALIZED),
       idle_since_last_transmission_(false),
-      session_id_(-1) {
+      session_id_(-1),
+      synthetic_trial_registry_(
+          client->IsExternalExperimentAllowlistEnabled()) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(state_manager_);
   DCHECK(client_);

components/metrics/metrics_service_accessor.cc

@@ -46,20 +46,6 @@ bool MetricsServiceAccessor::RegisterSyntheticFieldTrial(
       variations::HashName(group_name));
 }
 
-// static
-bool MetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial(
-    MetricsService* metrics_service,
-    base::StringPiece trial_name,
-    const std::vector<uint32_t>& group_name_hashes) {
-  if (!metrics_service)
-    return false;
-
-  metrics_service->synthetic_trial_registry()
-      ->RegisterSyntheticMultiGroupFieldTrial(variations::HashName(trial_name),
-                                              group_name_hashes);
-  return true;
-}
-
 // static
 bool MetricsServiceAccessor::RegisterSyntheticFieldTrialWithNameAndGroupHash(
     MetricsService* metrics_service,

components/metrics/metrics_service_accessor.h

@@ -41,16 +41,6 @@ class MetricsServiceAccessor {
                                           base::StringPiece trial_name,
                                           base::StringPiece group_name);
 
-  // Registers a field trial name and set of groups with |metrics_service| (if
-  // not null), to be used to annotate a UMA report with a particular
-  // configuration state. Returns true on success.
-  // See the comment on MetricsService::RegisterSyntheticMultiGroupFieldTrial()
-  // for details.
-  static bool RegisterSyntheticMultiGroupFieldTrial(
-      MetricsService* metrics_service,
-      base::StringPiece trial_name,
-      const std::vector<uint32_t>& group_name_hashes);
-
   // Same as RegisterSyntheticFieldTrial above, but takes in the trial and group
   // names as hashes rather than computing those hashes from the strings.
   static bool RegisterSyntheticFieldTrialWithNameAndGroupHash(

components/metrics/metrics_service_client.cc

@@ -31,18 +31,6 @@ ukm::UkmService* MetricsServiceClient::GetUkmService() {
   return nullptr;
 }
 
-bool MetricsServiceClient::IsReportingPolicyManaged() {
-  return false;
-}
-
-EnableMetricsDefault MetricsServiceClient::GetMetricsReportingDefaultState() {
-  return EnableMetricsDefault::DEFAULT_UNKNOWN;
-}
-
-bool MetricsServiceClient::IsUMACellularUploadLogicEnabled() {
-  return false;
-}
-
 GURL MetricsServiceClient::GetMetricsServerUrl() {
   return GURL(kNewMetricsServerUrl);
 }
@@ -74,6 +62,22 @@ bool MetricsServiceClient::ShouldStartUpFastForTesting() const {
   return false;
 }
 
+bool MetricsServiceClient::IsReportingPolicyManaged() {
+  return false;
+}
+
+EnableMetricsDefault MetricsServiceClient::GetMetricsReportingDefaultState() {
+  return EnableMetricsDefault::DEFAULT_UNKNOWN;
+}
+
+bool MetricsServiceClient::IsUMACellularUploadLogicEnabled() {
+  return false;
+}
+
+bool MetricsServiceClient::IsExternalExperimentAllowlistEnabled() {
+  return true;
+}
+
 bool MetricsServiceClient::IsUkmAllowedForAllProfiles() {
   return false;
 }

components/metrics/metrics_service_client.h

@@ -130,6 +130,10 @@ class MetricsServiceClient {
   // Returns whether cellular logic is enabled for metrics reporting.
   virtual bool IsUMACellularUploadLogicEnabled();
 
+  // Returns whether the allowlist for external experiment ids is enabled. Some
+  // embedders like WebLayer disable it. For Chrome, it should be enabled.
+  virtual bool IsExternalExperimentAllowlistEnabled();
+
   // Returns true iff UKM is allowed for all profiles.
   // See //components/ukm/observers/ukm_consent_state_observer.h for details.
   virtual bool IsUkmAllowedForAllProfiles();

components/variations/synthetic_trial_registry.cc

@@ -4,11 +4,32 @@
 
 #include "components/variations/synthetic_trial_registry.h"
 
+#include <algorithm>
+
+#include "base/metrics/histogram_functions.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "components/variations/hashing.h"
+#include "components/variations/variations_associated_data.h"
 
 namespace variations {
+namespace internal {
+
+const base::Feature kExternalExperimentAllowlist{
+    "ExternalExperimentAllowlist", base::FEATURE_DISABLED_BY_DEFAULT};
+
+}  // namespace internal
 
-SyntheticTrialRegistry::SyntheticTrialRegistry() = default;
+SyntheticTrialRegistry::SyntheticTrialRegistry(
+    bool enable_external_experiment_allowlist)
+    : enable_external_experiment_allowlist_(
+          enable_external_experiment_allowlist &&
+          base::FeatureList::IsEnabled(
+              internal::kExternalExperimentAllowlist)) {}
+
+SyntheticTrialRegistry::SyntheticTrialRegistry()
+    : enable_external_experiment_allowlist_(base::FeatureList::IsEnabled(
+          internal::kExternalExperimentAllowlist)) {}
 SyntheticTrialRegistry::~SyntheticTrialRegistry() = default;
 
 void SyntheticTrialRegistry::AddSyntheticTrialObserver(
@@ -23,13 +44,78 @@ void SyntheticTrialRegistry::RemoveSyntheticTrialObserver(
   synthetic_trial_observer_list_.RemoveObserver(observer);
 }
 
+void SyntheticTrialRegistry::RegisterExternalExperiments(
+    const std::string& fallback_study_name,
+    const std::vector<int>& experiment_ids,
+    SyntheticTrialRegistry::OverrideMode mode) {
+  DCHECK(!fallback_study_name.empty());
+
+  base::FieldTrialParams params;
+  if (enable_external_experiment_allowlist_ &&
+      !GetFieldTrialParamsByFeature(internal::kExternalExperimentAllowlist,
+                                    &params)) {
+    return;
+  }
+
+  // When overriding previous external experiments, remove them now.
+  if (mode == kOverrideExistingIds) {
+    auto is_external = [](const SyntheticTrialGroup& group) {
+      return group.is_external;
+    };
+    base::EraseIf(synthetic_trial_groups_, is_external);
+  }
+
+  const base::TimeTicks start_time = base::TimeTicks::Now();
+  int trials_added = 0;
+  for (int experiment_id : experiment_ids) {
+    const std::string experiment_id_str = base::NumberToString(experiment_id);
+    const base::StringPiece study_name =
+        GetStudyNameForExpId(fallback_study_name, params, experiment_id_str);
+    if (study_name.empty())
+      continue;
+
+    const uint32_t trial_hash = HashName(study_name);
+    // If existing ids shouldn't be overridden, skip entries whose study names
+    // are already registered.
+    if (mode == kDoNotOverrideExistingIds) {
+      auto matches_trial = [trial_hash](const SyntheticTrialGroup& group) {
+        return group.id.name == trial_hash;
+      };
+      const auto& groups = synthetic_trial_groups_;
+      if (std::any_of(groups.begin(), groups.end(), matches_trial)) {
+        continue;
+      }
+    }
+
+    const uint32_t group_hash = HashName(experiment_id_str);
+
+    // Since external experiments are not based on Chrome's low entropy source,
+    // they are only sent to Google web properties for signed-in users to make
+    // sure they couldn't be used to identify a user that's not signed-in.
+    AssociateGoogleVariationIDForceHashes(
+        GOOGLE_WEB_PROPERTIES_SIGNED_IN, {trial_hash, group_hash},
+        static_cast<VariationID>(experiment_id));
+    SyntheticTrialGroup entry(trial_hash, group_hash);
+    entry.start_time = start_time;
+    entry.is_external = true;
+    synthetic_trial_groups_.push_back(entry);
+    trials_added++;
+  }
+
+  base::UmaHistogramCounts100("UMA.ExternalExperiment.GroupCount",
+                              trials_added);
+
+  if (trials_added > 0)
+    NotifySyntheticTrialObservers();
+}
+
 void SyntheticTrialRegistry::RegisterSyntheticFieldTrial(
     const SyntheticTrialGroup& trial) {
-  for (size_t i = 0; i < synthetic_trial_groups_.size(); ++i) {
-    if (synthetic_trial_groups_[i].id.name == trial.id.name) {
-      if (synthetic_trial_groups_[i].id.group != trial.id.group) {
-        synthetic_trial_groups_[i].id.group = trial.id.group;
-        synthetic_trial_groups_[i].start_time = base::TimeTicks::Now();
+  for (auto& entry : synthetic_trial_groups_) {
+    if (entry.id.name == trial.id.name) {
+      if (entry.id.group != trial.id.group) {
+        entry.id.group = trial.id.group;
+        entry.start_time = base::TimeTicks::Now();
         NotifySyntheticTrialObservers();
       }
       return;
@@ -42,26 +128,26 @@ void SyntheticTrialRegistry::RegisterSyntheticFieldTrial(
   NotifySyntheticTrialObservers();
 }
 
-void SyntheticTrialRegistry::RegisterSyntheticMultiGroupFieldTrial(
-    uint32_t trial_name_hash,
-    const std::vector<uint32_t>& group_name_hashes) {
-  auto has_same_trial_name = [trial_name_hash](const SyntheticTrialGroup& x) {
-    return x.id.name == trial_name_hash;
-  };
-  base::EraseIf(synthetic_trial_groups_, has_same_trial_name);
+base::StringPiece SyntheticTrialRegistry::GetStudyNameForExpId(
+    const std::string& fallback_study_name,
+    const base::FieldTrialParams& params,
+    const std::string& experiment_id) {
+  if (!enable_external_experiment_allowlist_)
+    return fallback_study_name;
 
-  if (group_name_hashes.empty())
-    return;
+  const auto it = params.find(experiment_id);
+  if (it == params.end())
+    return base::StringPiece();
 
-  SyntheticTrialGroup trial_group(trial_name_hash, group_name_hashes[0]);
-  trial_group.start_time = base::TimeTicks::Now();
-  for (uint32_t group_name_hash : group_name_hashes) {
-    // Note: Adding the trial group will copy it, so this re-uses the same
-    // |trial_group| struct for convenience (e.g. so start_time's all match).
-    trial_group.id.group = group_name_hash;
-    synthetic_trial_groups_.push_back(trial_group);
-  }
-  NotifySyntheticTrialObservers();
+  // To support additional parameters being passed, besides the study name,
+  // truncate the study name at the first ',' character.
+  // For example, for an entry like {"1234": "StudyName,FOO"}, we only want the
+  // "StudyName" part. This allows adding support for additional things like FOO
+  // in the future without backwards compatibility problems.
+  const size_t comma_pos = it->second.find(',');
+  const size_t truncate_pos =
+      (comma_pos == std::string::npos ? it->second.length() : comma_pos);
+  return base::StringPiece(it->second.data(), truncate_pos);
 }
 
 void SyntheticTrialRegistry::NotifySyntheticTrialObservers() {
@@ -72,12 +158,12 @@ void SyntheticTrialRegistry::NotifySyntheticTrialObservers() {
 
 void SyntheticTrialRegistry::GetSyntheticFieldTrialsOlderThan(
     base::TimeTicks time,
-    std::vector<ActiveGroupId>* synthetic_trials) {
+    std::vector<ActiveGroupId>* synthetic_trials) const {
   DCHECK(synthetic_trials);
   synthetic_trials->clear();
-  for (size_t i = 0; i < synthetic_trial_groups_.size(); ++i) {
-    if (synthetic_trial_groups_[i].start_time <= time)
-      synthetic_trials->push_back(synthetic_trial_groups_[i].id);
+  for (const auto& entry : synthetic_trial_groups_) {
+    if (entry.start_time <= time)
+      synthetic_trials->push_back(entry.id);
   }
 }
 

components/variations/synthetic_trial_registry.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/feature_list.h"
 #include "base/observer_list.h"
 #include "components/variations/synthetic_trials.h"
 
@@ -19,9 +20,22 @@ namespace variations {
 struct ActiveGroupId;
 class FieldTrialsProvider;
 class FieldTrialsProviderTest;
+class SyntheticTrialRegistryTest;
+
+namespace internal {
+extern const base::Feature kExternalExperimentAllowlist;
+}  // namespace internal
 
 class SyntheticTrialRegistry {
  public:
+  // Constructor that specifies whether the SyntheticTrialRegistry should use
+  // an allowlist for external experiments. Some embedders such as WebLayer
+  // do not run as Chrome and do not use the allowlist.
+  // Note: The allowlist is enabled only if |kExternalExperimentAllowlist| is
+  // also enabled, even if the parameter value is true. The default constructor
+  // defaults to the feature state.
+  explicit SyntheticTrialRegistry(bool enable_external_experiment_allowlist);
+
   SyntheticTrialRegistry();
   ~SyntheticTrialRegistry();
 
@@ -31,13 +45,41 @@ class SyntheticTrialRegistry {
   // Removes an existing observer of synthetic trials list changes.
   void RemoveSyntheticTrialObserver(SyntheticTrialObserver* observer);
 
+  // Specifies the mode of RegisterExternalExperiments() operation.
+  enum OverrideMode {
+    // Previously-registered external experiment ids are overridden (replaced)
+    // with the new list.
+    kOverrideExistingIds,
+    // Previously-registered external experiment ids are not overridden, but
+    // new experiment ids may be added.
+    kDoNotOverrideExistingIds,
+  };
+
+  // Registers a list of experiment ids coming from an external application.
+  // The input ids are in the VariationID format.
+  //
+  // When |enable_external_experiment_allowlist| is true, the supplied ids must
+  // have corresponding entries in the "ExternalExperimentAllowlist" (coming via
+  // a feature param) to be applied. The allowlist also supplies the
+  // corresponding trial name that should be used for reporting to UMA.
+  //
+  // When |enable_external_experiment_allowlist| is false, |fallback_study_name|
+  // will be used as the trial name for all provided experiment ids.
+  //
+  // If |mode| is kOverrideExistingIds, this API clears previously-registered
+  // external experiment ids, replacing them with the new list (which may be
+  // empty). If |mode| is kDoNotOverrideExistingIds, any new ids that are not
+  // already registered will be added, but existing ones will not be replaced.
+  void RegisterExternalExperiments(const std::string& fallback_study_name,
+                                   const std::vector<int>& experiment_ids,
+                                   OverrideMode mode);
+
  private:
   friend metrics::MetricsServiceAccessor;
   friend FieldTrialsProvider;
   friend FieldTrialsProviderTest;
+  friend SyntheticTrialRegistryTest;
   FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest, RegisterSyntheticTrial);
-  FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest,
-                           RegisterSyntheticMultiGroupFieldTrial);
   FRIEND_TEST_ALL_PREFIXES(SyntheticTrialRegistryTest,
                            GetSyntheticFieldTrialActiveGroups);
   FRIEND_TEST_ALL_PREFIXES(VariationsCrashKeysTest, BasicFunctionality);
@@ -50,24 +92,29 @@ class SyntheticTrialRegistry {
   // is registered for a given trial name will be recorded. The values passed
   // in must not correspond to any real field trial in the code.
   // Note: Should not be used to replace trials that were registered with
-  // RegisterSyntheticMultiGroupFieldTrial().
+  // RegisterExternalExperiments().
   void RegisterSyntheticFieldTrial(const SyntheticTrialGroup& trial_group);
 
-  // Similar to RegisterSyntheticFieldTrial(), but registers a synthetic trial
-  // that has multiple active groups for a given trial name hash. Any previous
-  // groups registered for |trial_name_hash| will be replaced.
-  void RegisterSyntheticMultiGroupFieldTrial(
-      uint32_t trial_name_hash,
-      const std::vector<uint32_t>& group_name_hashes);
+  // Returns the study name corresponding to |experiment_id| from the allowlist
+  // contained in |params| if the allowlist is enabled, otherwise returns
+  // |fallback_study_name|. An empty string piece is returned when the
+  // experiment is not in the allowlist.
+  base::StringPiece GetStudyNameForExpId(const std::string& fallback_study_name,
+                                         const base::FieldTrialParams& params,
+                                         const std::string& experiment_id);
 
   // Returns a list of synthetic field trials that are older than |time|.
   void GetSyntheticFieldTrialsOlderThan(
       base::TimeTicks time,
-      std::vector<ActiveGroupId>* synthetic_trials);
+      std::vector<ActiveGroupId>* synthetic_trials) const;
 
   // Notifies observers on a synthetic trial list change.
   void NotifySyntheticTrialObservers();
 
+  // Whether the allowlist is enabled. Some configurations, like WebLayer
+  // do not use the allowlist.
+  bool enable_external_experiment_allowlist_ = true;
+
   // Field trial groups that map to Chrome configuration states.
   std::vector<SyntheticTrialGroup> synthetic_trial_groups_;
 

components/variations/synthetic_trial_registry_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/metrics/field_trial.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "components/variations/active_field_trials.h"
 #include "components/variations/hashing.h"
@@ -17,8 +18,6 @@
 
 namespace variations {
 
-namespace {
-
 class SyntheticTrialRegistryTest : public ::testing::Test {
  public:
   SyntheticTrialRegistryTest() { InitCrashKeys(); }
@@ -46,14 +45,21 @@ class SyntheticTrialRegistryTest : public ::testing::Test {
     }
   }
 
+  // Gets the current synthetic trials.
+  void GetSyntheticTrials(const SyntheticTrialRegistry& registry,
+                          std::vector<ActiveGroupId>* synthetic_trials) {
+    // Ensure that time has advanced by at least a tick before proceeding.
+    WaitUntilTimeChanges(base::TimeTicks::Now());
+    registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
+                                              synthetic_trials);
+  }
+
  private:
   base::test::SingleThreadTaskEnvironment task_environment_;
 
   DISALLOW_COPY_AND_ASSIGN(SyntheticTrialRegistryTest);
 };
 
-}  // namespace
-
 TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticTrial) {
   SyntheticTrialRegistry registry;
 
@@ -95,57 +101,89 @@ TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticTrial) {
   EXPECT_EQ(1U, synthetic_trials.size());
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial2", "Group2"));
 
-  // Ensure that time has advanced by at least a tick before proceeding.
-  WaitUntilTimeChanges(base::TimeTicks::Now());
-
   // Start a new log and ensure all three trials appear in it.
-  registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
-                                            &synthetic_trials);
+  GetSyntheticTrials(registry, &synthetic_trials);
   EXPECT_EQ(3U, synthetic_trials.size());
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "Group2"));
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial2", "Group2"));
   EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial3", "Group3"));
 }
 
-TEST_F(SyntheticTrialRegistryTest, RegisterSyntheticMultiGroupFieldTrial) {
-  SyntheticTrialRegistry registry;
-
-  // Register a synthetic trial TestTrial1 with groups A and B.
-  uint32_t trial_name_hash = HashName("TestTrial1");
-  std::vector<uint32_t> group_name_hashes = {HashName("A"), HashName("B")};
-  registry.RegisterSyntheticMultiGroupFieldTrial(trial_name_hash,
-                                                 group_name_hashes);
-  // Ensure that time has advanced by at least a tick before proceeding.
-  WaitUntilTimeChanges(base::TimeTicks::Now());
+TEST_F(SyntheticTrialRegistryTest, RegisterExternalExperiments_NoAllowlist) {
+  SyntheticTrialRegistry registry(false);
+  const std::string context = "TestTrial1";
+  const auto mode = SyntheticTrialRegistry::kOverrideExistingIds;
 
+  registry.RegisterExternalExperiments(context, {100, 200}, mode);
   std::vector<ActiveGroupId> synthetic_trials;
-  registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
-                                            &synthetic_trials);
+  GetSyntheticTrials(registry, &synthetic_trials);
   EXPECT_EQ(2U, synthetic_trials.size());
-  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "A"));
-  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "B"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "100"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "200"));
 
   // Change the group for the trial to a single group.
-  group_name_hashes = {HashName("X")};
-  registry.RegisterSyntheticMultiGroupFieldTrial(trial_name_hash,
-                                                 group_name_hashes);
-  // Ensure that time has advanced by at least a tick before proceeding.
-  WaitUntilTimeChanges(base::TimeTicks::Now());
-
-  registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
-                                            &synthetic_trials);
+  registry.RegisterExternalExperiments(context, {500}, mode);
+  GetSyntheticTrials(registry, &synthetic_trials);
   EXPECT_EQ(1U, synthetic_trials.size());
-  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "X"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "TestTrial1", "500"));
 
   // Register a trial with no groups, which should effectively remove the trial.
-  group_name_hashes.clear();
-  registry.RegisterSyntheticMultiGroupFieldTrial(trial_name_hash,
-                                                 group_name_hashes);
-  // Ensure that time has advanced by at least a tick before proceeding.
-  WaitUntilTimeChanges(base::TimeTicks::Now());
+  registry.RegisterExternalExperiments(context, {}, mode);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(0U, synthetic_trials.size());
+}
 
-  registry.GetSyntheticFieldTrialsOlderThan(base::TimeTicks::Now(),
-                                            &synthetic_trials);
+TEST_F(SyntheticTrialRegistryTest, RegisterExternalExperiments_WithAllowlist) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      internal::kExternalExperimentAllowlist,
+      {{"100", "A"}, {"101", "A"}, {"300", "C,xyz"}});
+
+  const std::string context = "Test";
+  const auto override_mode = SyntheticTrialRegistry::kOverrideExistingIds;
+  SyntheticTrialRegistry registry;
+  std::vector<ActiveGroupId> synthetic_trials;
+
+  // Register a synthetic trial TestTrial1 with groups A and B.
+  registry.RegisterExternalExperiments(context, {100, 200, 300}, override_mode);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(2U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "100"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300"));
+
+  // A new call that only contains 100 will clear the other ones.
+  registry.RegisterExternalExperiments(context, {101}, override_mode);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(1U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101"));
+
+  const auto dont_override = SyntheticTrialRegistry::kDoNotOverrideExistingIds;
+  // Now, register another id that doesn't exist with kDoNotOverrideExistingIds.
+  registry.RegisterExternalExperiments(context, {300}, dont_override);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(2U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300"));
+
+  // Registering 100, which already has a trial A registered, shouldn't work.
+  registry.RegisterExternalExperiments(context, {100}, dont_override);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(2U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300"));
+
+  // Registering an empty set should also do nothing.
+  registry.RegisterExternalExperiments(context, {}, dont_override);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(2U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "101"));
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "C", "300"));
+
+  // Registering with an override should reset existing ones.
+  registry.RegisterExternalExperiments(context, {100}, override_mode);
+  GetSyntheticTrials(registry, &synthetic_trials);
+  EXPECT_EQ(1U, synthetic_trials.size());
+  EXPECT_TRUE(HasSyntheticTrial(synthetic_trials, "A", "100"));
 }
 
 TEST_F(SyntheticTrialRegistryTest, GetSyntheticFieldTrialActiveGroups) {

components/variations/synthetic_trials.h

@@ -25,6 +25,9 @@ struct SyntheticTrialGroup {
 
   ActiveGroupId id;
   base::TimeTicks start_time;
+
+  // If this is an external experiment.
+  bool is_external = false;
 };
 
 // Interface class to observe changes to synthetic trials in MetricsService.

weblayer/BUILD.gn

@@ -473,7 +473,6 @@ source_set("weblayer_lib_base") {
       "browser/android/exception_filter.h",
       "browser/android/metrics/uma_utils.cc",
       "browser/android/metrics/uma_utils.h",
-      "browser/android/metrics/weblayer_metrics_service_accessor.h",
       "browser/android/metrics/weblayer_metrics_service_client.cc",
       "browser/android/metrics/weblayer_metrics_service_client.h",
       "browser/android/permission_request_utils.cc",

weblayer/browser/android/metrics/weblayer_metrics_service_accessor.h

-// 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.
-
-#ifndef WEBLAYER_BROWSER_ANDROID_METRICS_WEBLAYER_METRICS_SERVICE_ACCESSOR_H_
-#define WEBLAYER_BROWSER_ANDROID_METRICS_WEBLAYER_METRICS_SERVICE_ACCESSOR_H_
-
-#include <stdint.h>
-#include <vector>
-
-#include "components/metrics/metrics_service_accessor.h"
-
-namespace weblayer {
-
-// This class limits and documents access to metrics service helper methods.
-// Since these methods are private, each user has to be explicitly declared
-// as a 'friend' below.
-class WebLayerMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
- private:
-  // For RegisterSyntheticMultiGroupFieldTrial.
-  friend class WebLayerMetricsServiceClient;
-
-  DISALLOW_IMPLICIT_CONSTRUCTORS(WebLayerMetricsServiceAccessor);
-};
-
-}  // namespace weblayer
-
-#endif  // WEBLAYER_BROWSER_ANDROID_METRICS_WEBLAYER_METRICS_SERVICE_ACCESSOR_H_
\ No newline at end of file

weblayer/browser/android/metrics/weblayer_metrics_service_client.cc

@@ -10,16 +10,12 @@
 
 #include "base/base64.h"
 #include "base/no_destructor.h"
-#include "base/strings/string_number_conversions.h"
 #include "components/metrics/metrics_provider.h"
 #include "components/metrics/metrics_service.h"
 #include "components/page_load_metrics/browser/metrics_web_contents_observer.h"
-#include "components/variations/hashing.h"
-#include "components/variations/variations_associated_data.h"
 #include "components/version_info/android/channel_getter.h"
 #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"
@@ -87,44 +83,33 @@ WebLayerMetricsServiceClient::~WebLayerMetricsServiceClient() {
   ProfileImpl::RemoveProfileObserver(this);
 }
 
-void WebLayerMetricsServiceClient::RegisterSyntheticMultiGroupFieldTrial(
-    base::StringPiece trial_name,
+void WebLayerMetricsServiceClient::RegisterExternalExperiments(
     const std::vector<int>& experiment_ids) {
   if (!GetMetricsService()) {
     if (!IsConsentDetermined()) {
       post_start_tasks_.push_back(base::BindOnce(
-          &WebLayerMetricsServiceClient::RegisterSyntheticMultiGroupFieldTrial,
-          base::Unretained(this), trial_name, experiment_ids));
+          &WebLayerMetricsServiceClient::RegisterExternalExperiments,
+          base::Unretained(this), experiment_ids));
     }
     return;
   }
 
-  std::vector<uint32_t> group_name_hashes;
-  group_name_hashes.reserve(experiment_ids.size());
-
-  variations::ActiveGroupId active_group;
-  active_group.name = variations::HashName(trial_name);
-  for (int experiment_id : experiment_ids) {
-    active_group.group =
-        variations::HashName(base::NumberToString(experiment_id));
-
-    // Since external experiments are not based on Chrome's low entropy source,
-    // they are only sent to Google web properties for signed in users to make
-    // sure that this couldn't be used to identify a user that's not signed in.
-    variations::AssociateGoogleVariationIDForceHashes(
-        variations::GOOGLE_WEB_PROPERTIES_SIGNED_IN, active_group,
-        static_cast<variations::VariationID>(experiment_id));
-    group_name_hashes.push_back(active_group.group);
-  }
-
-  WebLayerMetricsServiceAccessor::RegisterSyntheticMultiGroupFieldTrial(
-      GetMetricsService(), trial_name, group_name_hashes);
+  GetMetricsService()->synthetic_trial_registry()->RegisterExternalExperiments(
+      "WebLayerExperiments", experiment_ids,
+      variations::SyntheticTrialRegistry::kOverrideExistingIds);
 }
 
 int32_t WebLayerMetricsServiceClient::GetProduct() {
   return metrics::ChromeUserMetricsExtension::ANDROID_WEBLAYER;
 }
 
+bool WebLayerMetricsServiceClient::IsExternalExperimentAllowlistEnabled() {
+  // RegisterExternalExperiments() is actually used to register experiment ids
+  // coming from the app embedding WebLayer itself, rather than externally. So
+  // the allowlist shouldn't be applied.
+  return false;
+}
+
 bool WebLayerMetricsServiceClient::IsUkmAllowedForAllProfiles() {
   for (auto* profile : ProfileImpl::GetAllProfiles()) {
     if (!profile->GetBooleanSetting(SettingType::UKM_ENABLED))

weblayer/browser/android/metrics/weblayer_metrics_service_client.h

@@ -33,12 +33,11 @@ class WebLayerMetricsServiceClient
   WebLayerMetricsServiceClient();
   ~WebLayerMetricsServiceClient() override;
 
-  void RegisterSyntheticMultiGroupFieldTrial(
-      base::StringPiece trial_name,
-      const std::vector<int>& experiment_ids);
+  void RegisterExternalExperiments(const std::vector<int>& experiment_ids);
 
   // metrics::MetricsServiceClient
   int32_t GetProduct() override;
+  bool IsExternalExperimentAllowlistEnabled() override;
   bool IsUkmAllowedForAllProfiles() override;
   std::string GetUploadSigningKey() override;
 

weblayer/browser/weblayer_impl_android.cc

@@ -47,7 +47,6 @@ static void JNI_WebLayerImpl_RegisterExternalExperimentIDs(
     JNIEnv* env,
     const JavaParamRef<jstring>& jtrial_name,
     const JavaParamRef<jintArray>& jexperiment_ids) {
-  const std::string trial_name_utf8(ConvertJavaStringToUTF8(env, jtrial_name));
   std::vector<int> experiment_ids;
   // A null |jexperiment_ids| is the same as an empty list.
   if (jexperiment_ids) {
@@ -55,8 +54,8 @@ static void JNI_WebLayerImpl_RegisterExternalExperimentIDs(
                                            &experiment_ids);
   }
 
-  WebLayerMetricsServiceClient::GetInstance()
-      ->RegisterSyntheticMultiGroupFieldTrial(trial_name_utf8, experiment_ids);
+  WebLayerMetricsServiceClient::GetInstance()->RegisterExternalExperiments(
+      experiment_ids);
 }
 
 base::string16 GetClientApplicationName() {