weblayer: fix race condition in setting up field trials

There were two problems here:
1. When FeatureListCreator created WebLayerVariationsServiceClient it
   was triggering creation of the network service (by calling
   SystemNetworkContextManager::GetSharedURLLoaderFactory). The network
   service has code paths that may check the state of features, so that
   by triggering creation of the network service before features were
   initialized the network service could potentially see features in
   the wrong state.
2. FeatureListCreator was calling PerformPreMainMessageLoopStartup
   followed by SetupFieldTrials(). This is not the expected order (in
   fact PerformPreMainMessageLoopStartup may also trigger creation of
   the network service). This makes the order match that of chrome.

BUG=1049426
TEST=test only change

Change-Id: I21508eada373d368c4f4d4b5d5719b636700dd44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2125435
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754528}

weblayer/browser/browser_main_parts_impl.cc

@@ -22,6 +22,7 @@
 #include "services/service_manager/embedder/result_codes.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "weblayer/browser/browser_process.h"
+#include "weblayer/browser/feature_list_creator.h"
 #include "weblayer/browser/host_content_settings_map_factory.h"
 #include "weblayer/browser/permissions/weblayer_permissions_client.h"
 #include "weblayer/browser/stateful_ssl_host_state_delegate_factory.h"
@@ -131,6 +132,8 @@ int BrowserMainPartsImpl::PreEarlyInitialization() {
 }
 
 void BrowserMainPartsImpl::PreMainMessageLoopRun() {
+  FeatureListCreator::GetInstance()->PerformPreMainMessageLoopStartup();
+
   // It's necessary to have a complete dependency graph of
   // BrowserContextKeyedServices before calling out to the delegate (which
   // will potentially create a profile), so that a profile creation message is

weblayer/browser/feature_list_creator.cc

@@ -20,6 +20,7 @@
 #include "components/prefs/pref_service_factory.h"
 #include "components/variations/pref_names.h"
 #include "components/variations/service/ui_string_overrider.h"
+#include "components/variations/service/variations_service.h"
 #include "components/variations/variations_crash_keys.h"
 #include "content/public/browser/network_service_instance.h"
 #include "content/public/common/content_switch_dependent_feature_overrides.h"
@@ -37,6 +38,8 @@ const char kDisableBackgroundNetworking[] = "disable-background-networking";
 namespace weblayer {
 namespace {
 
+FeatureListCreator* feature_list_creator_instance = nullptr;
+
 void HandleReadError(PersistentPrefStore::PrefReadError error) {}
 
 #if defined(OS_ANDROID)
@@ -75,9 +78,20 @@ std::unique_ptr<PrefService> CreatePrefService() {
 
 }  // namespace
 
-FeatureListCreator::FeatureListCreator() = default;
+FeatureListCreator::FeatureListCreator() {
+  DCHECK(!feature_list_creator_instance);
+  feature_list_creator_instance = this;
+}
+
+FeatureListCreator::~FeatureListCreator() {
+  feature_list_creator_instance = nullptr;
+}
 
-FeatureListCreator::~FeatureListCreator() = default;
+// static
+FeatureListCreator* FeatureListCreator::GetInstance() {
+  DCHECK(feature_list_creator_instance);
+  return feature_list_creator_instance;
+}
 
 void FeatureListCreator::SetSystemNetworkContextManager(
     SystemNetworkContextManager* system_network_context_manager) {
@@ -107,7 +121,7 @@ void FeatureListCreator::SetUpFieldTrials() {
   DCHECK(system_network_context_manager_);
   variations_service_ = variations::VariationsService::Create(
       std::make_unique<WebLayerVariationsServiceClient>(
-          system_network_context_manager_->GetSharedURLLoaderFactory()),
+          system_network_context_manager_),
       local_state_.get(), metrics_client->metrics_state_manager(),
       switches::kDisableBackgroundNetworking, variations::UIStringOverrider(),
       base::BindOnce(&content::GetNetworkConnectionTracker));
@@ -118,7 +132,6 @@ void FeatureListCreator::SetUpFieldTrials() {
   std::vector<std::string> variation_ids;
   auto feature_list = std::make_unique<base::FeatureList>();
 
-  variations_service_->PerformPreMainMessageLoopStartup();
   variations_service_->SetupFieldTrials(
       cc::switches::kEnableGpuBenchmarking, switches::kEnableFeatures,
       switches::kDisableFeatures, unforceable_field_trials, variation_ids,
@@ -140,4 +153,12 @@ void FeatureListCreator::CreateFeatureListAndFieldTrials() {
   SetUpFieldTrials();
 }
 
+void FeatureListCreator::PerformPreMainMessageLoopStartup() {
+#if defined(OS_ANDROID)
+  // It is expected this is called after SetUpFieldTrials().
+  DCHECK(variations_service_);
+  variations_service_->PerformPreMainMessageLoopStartup();
+#endif
+}
+
 }  // namespace weblayer

weblayer/browser/feature_list_creator.h

@@ -8,9 +8,12 @@
 #include <memory>
 
 #include "components/prefs/pref_service.h"
-#include "components/variations/service/variations_service.h"
 #include "weblayer/browser/weblayer_field_trials.h"
 
+namespace variations {
+class VariationsService;
+}
+
 namespace weblayer {
 class SystemNetworkContextManager;
 
@@ -23,12 +26,20 @@ class FeatureListCreator {
   FeatureListCreator();
   ~FeatureListCreator();
 
+  // Return the single instance of FeatureListCreator. This does *not* trigger
+  // creation.
+  static FeatureListCreator* GetInstance();
+
   void SetSystemNetworkContextManager(
       SystemNetworkContextManager* system_network_context_manager);
 
   // Must be called after SetSharedURLLoaderFactory.
   void CreateFeatureListAndFieldTrials();
 
+  // Called from content::BrowserMainParts::PreMainMessageLoopRun() to perform
+  // initialization necessary prior to running the main message loop.
+  void PerformPreMainMessageLoopStartup();
+
   PrefService* local_state() const { return local_state_.get(); }
 
  private:

weblayer/browser/weblayer_variations_service_client.cc

@@ -8,6 +8,7 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "build/build_config.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "weblayer/browser/system_network_context_manager.h"
 
 #if defined(OS_ANDROID)
 #include "components/version_info/android/channel_getter.h"
@@ -29,8 +30,10 @@ base::Version GetVersionForSimulation() {
 }  // namespace
 
 WebLayerVariationsServiceClient::WebLayerVariationsServiceClient(
-    scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory)
-    : shared_url_loader_factory_(std::move(shared_url_loader_factory)) {}
+    SystemNetworkContextManager* network_context_manager)
+    : network_context_manager_(network_context_manager) {
+  DCHECK(network_context_manager_);
+}
 
 WebLayerVariationsServiceClient::~WebLayerVariationsServiceClient() = default;
 
@@ -41,7 +44,7 @@ WebLayerVariationsServiceClient::GetVersionForSimulationCallback() {
 
 scoped_refptr<network::SharedURLLoaderFactory>
 WebLayerVariationsServiceClient::GetURLLoaderFactory() {
-  return shared_url_loader_factory_;
+  return network_context_manager_->GetSharedURLLoaderFactory();
 }
 
 network_time::NetworkTimeTracker*

weblayer/browser/weblayer_variations_service_client.h

@@ -11,22 +11,21 @@
 #include "base/memory/scoped_refptr.h"
 #include "components/variations/service/variations_service_client.h"
 
-namespace network {
-class SharedURLLoaderFactory;
-}  // namespace network
-
 namespace weblayer {
 
+class SystemNetworkContextManager;
+
 // WebLayerVariationsServiceClient provides an implementation of
 // VariationsServiceClient, all members are currently stubs for WebLayer.
 class WebLayerVariationsServiceClient
     : public variations::VariationsServiceClient {
  public:
   explicit WebLayerVariationsServiceClient(
-      scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory);
+      SystemNetworkContextManager* network_context_manager);
   ~WebLayerVariationsServiceClient() override;
 
  private:
+  // variations::VariationsServiceClient:
   base::OnceCallback<base::Version(void)> GetVersionForSimulationCallback()
       override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
@@ -35,7 +34,7 @@ class WebLayerVariationsServiceClient
   bool OverridesRestrictParameter(std::string* parameter) override;
   bool IsEnterprise() override;
 
-  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
+  SystemNetworkContextManager* network_context_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(WebLayerVariationsServiceClient);
 };