Passing ChromeFeatureListCreator to BrowserProcessImpl

This CL is based on https://chromium-review.googlesource.com/c/chromium/src/+/1199944
When ServiceManager launches, ChromeFeatureListCreator will be created to
load the user pref store and a simple local state.
Later on the user pref store will be passed to BrowserProcessImpl to
create a new local state.
This CL simplifies the process by passing ChromeFeatureListCreator
to BrowserProcessImpl, so that the simple local state can be transformed
into g_browser_process->local_state.

BUG=866028

Change-Id: Ia2e6f67bc6dc54f31b6efc2cdabe6c7d4d6158ae
Reviewed-on: https://chromium-review.googlesource.com/1204926
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592179}

chrome/browser/browser_process_impl.cc

@@ -38,6 +38,7 @@
 #include "chrome/browser/chrome_child_process_watcher.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/chrome_device_client.h"
+#include "chrome/browser/chrome_feature_list_creator.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/component_updater/chrome_component_updater_configurator.h"
 #include "chrome/browser/component_updater/supervised_user_whitelist_installer.h"
@@ -216,8 +217,8 @@ rappor::RapporService* GetBrowserRapporService() {
 }
 
 BrowserProcessImpl::BrowserProcessImpl(
-    scoped_refptr<PersistentPrefStore> user_pref_store)
-    : user_pref_store_(std::move(user_pref_store)),
+    ChromeFeatureListCreator* chrome_feature_list_creator)
+    : chrome_feature_list_creator_(chrome_feature_list_creator),
       pref_service_factory_(
           std::make_unique<prefs::InProcessPrefServiceFactory>()) {
   g_browser_process = this;
@@ -1110,17 +1111,29 @@ void BrowserProcessImpl::CreateLocalState() {
 
   base::FilePath local_state_path;
   CHECK(base::PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path));
+
   auto pref_registry = base::MakeRefCounted<PrefRegistrySimple>();
+  if (chrome_feature_list_creator_)
+    local_state_ = chrome_feature_list_creator_->TakePrefService();
+  PrefRegistrySimple* pref_pregistry_simple =
+      local_state_ ? static_cast<PrefRegistrySimple*>(
+                         local_state_->DeprecatedGetPrefRegistry())
+                   : pref_registry.get();
 
   // Register local state preferences.
-  RegisterLocalState(pref_registry.get());
+  RegisterLocalState(pref_pregistry_simple);
 
   auto delegate = pref_service_factory_->CreateDelegate();
-  delegate->InitPrefRegistry(pref_registry.get());
-  local_state_ = chrome_prefs::CreateLocalState(
-      local_state_path, policy_service(), std::move(pref_registry), false,
-      std::move(delegate), std::move(user_pref_store_));
-  DCHECK(local_state_);
+  delegate->InitPrefRegistry(pref_pregistry_simple);
+  if (local_state_) {
+    chrome_prefs::InstallPoliciesOnLocalState(
+        local_state_.get(), policy_service(), std::move(delegate));
+  } else {
+    local_state_ = chrome_prefs::CreateLocalState(
+        local_state_path, policy_service(), std::move(pref_registry), false,
+        std::move(delegate));
+    DCHECK(local_state_);
+  }
 
   sessions::SessionIdGenerator::GetInstance()->Init(local_state_.get());
 }

chrome/browser/browser_process_impl.h

@@ -35,6 +35,7 @@
 
 class ChromeChildProcessWatcher;
 class ChromeDeviceClient;
+class ChromeFeatureListCreator;
 class ChromeMetricsServicesManagerClient;
 class ChromeResourceDispatcherHostDelegate;
 class DevToolsAutoOpener;
@@ -82,7 +83,7 @@ class BrowserProcessImpl : public BrowserProcess,
   // destination) of user prefs for Local State instead of loading the JSON file
   // from disk.
   explicit BrowserProcessImpl(
-      scoped_refptr<PersistentPrefStore> user_pref_store);
+      ChromeFeatureListCreator* chrome_feature_list_creator);
   ~BrowserProcessImpl() override;
 
   // Called to complete initialization.
@@ -329,10 +330,9 @@ class BrowserProcessImpl : public BrowserProcess,
 
   scoped_refptr<DownloadRequestLimiter> download_request_limiter_;
 
-  // A pref store that is created from the Local State file. This is handed-off
-  // to |local_state_| when it's created. It will use it, if non-null, instead
-  // of loading the user prefs from disk.
-  scoped_refptr<PersistentPrefStore> user_pref_store_;
+  // If non-null, this object holds a pref store that will be taken by
+  // BrowserProcessImpl to create the |local_state_|.
+  ChromeFeatureListCreator* chrome_feature_list_creator_;
 
   // Ensures that the observers of plugin/print disable/enable state
   // notifications are properly added and removed.

chrome/browser/chrome_browser_main.cc

@@ -405,9 +405,9 @@ void InitializeLocalState() {
       registry->RegisterStringPref(language::prefs::kApplicationLocale,
                                    std::string());
       const std::unique_ptr<PrefService> parent_local_state =
-          chrome_prefs::CreateLocalState(
-              parent_profile, g_browser_process->policy_service(),
-              std::move(registry), false, nullptr, nullptr);
+          chrome_prefs::CreateLocalState(parent_profile,
+                                         g_browser_process->policy_service(),
+                                         std::move(registry), false, nullptr);
       // Right now, we only inherit the locale setting from the parent profile.
       local_state->SetString(
           language::prefs::kApplicationLocale,
@@ -1004,8 +1004,8 @@ int ChromeBrowserMainParts::PreEarlyInitialization() {
 
   // Create BrowserProcess in PreEarlyInitialization() so that we can load
   // field trials (and all it depends upon).
-  browser_process_ = std::make_unique<BrowserProcessImpl>(
-      chrome_feature_list_creator_->GetPrefStore());
+  browser_process_ =
+      std::make_unique<BrowserProcessImpl>(chrome_feature_list_creator_);
 
   bool failed_to_load_resource_bundle = false;
   const int load_local_state_result =

chrome/browser/chrome_feature_list_creator.cc

@@ -7,6 +7,8 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
+#include "chrome/browser/prefs/chrome_command_line_pref_store.h"
+#include "chrome/browser/prefs/chrome_pref_service_factory.h"
 #include "chrome/common/chrome_paths.h"
 #include "components/prefs/json_pref_store.h"
 #include "components/prefs/pref_registry.h"
@@ -28,7 +30,11 @@ void ChromeFeatureListCreator::CreatePrefService() {
 
   PrefServiceFactory factory;
   factory.set_user_prefs(pref_store_);
-
+  factory.set_command_line_prefs(
+      base::MakeRefCounted<ChromeCommandLinePrefStore>(
+          base::CommandLine::ForCurrentProcess()));
+  factory.set_read_error_callback(base::BindRepeating(
+      &chrome_prefs::HandlePersistentPrefStoreReadError, local_state_file));
   scoped_refptr<PrefRegistry> registry = new PrefRegistrySimple();
   simple_local_state_ = factory.Create(registry);
 }
@@ -37,6 +43,10 @@ scoped_refptr<PersistentPrefStore> ChromeFeatureListCreator::GetPrefStore() {
   return pref_store_;
 }
 
+std::unique_ptr<PrefService> ChromeFeatureListCreator::TakePrefService() {
+  return std::move(simple_local_state_);
+}
+
 void ChromeFeatureListCreator::CreateFeatureList() {
   CreatePrefService();
   // TODO(hanxi): Add implementation to create feature list.

chrome/browser/chrome_feature_list_creator.h

@@ -19,6 +19,9 @@ class ChromeFeatureListCreator {
   // Gets the pref store that is used to create feature list.
   scoped_refptr<PersistentPrefStore> GetPrefStore();
 
+  // Passing ownership of the |simple_local_state_| to the caller.
+  std::unique_ptr<PrefService> TakePrefService();
+
   // Initializes all necessary parameters to create the feature list and calls
   // base::FeatureList::SetInstance() to set the global instance.
   void CreateFeatureList();
@@ -30,6 +33,8 @@ class ChromeFeatureListCreator {
 
   scoped_refptr<PersistentPrefStore> pref_store_;
 
+  // If TakePrefService() is called, the caller will take the ownership
+  // of this variable. Stop using this variable afterwards.
   std::unique_ptr<PrefService> simple_local_state_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeFeatureListCreator);

chrome/browser/prefs/chrome_pref_service_factory.cc

@@ -303,56 +303,6 @@ GetTrackingConfiguration() {
   return result;
 }
 
-// Shows notifications which correspond to PersistentPrefStore's reading errors.
-void HandleReadError(const base::FilePath& pref_filename,
-                     PersistentPrefStore::PrefReadError error) {
-  // The error callback is always invoked back on the main thread (which is
-  // BrowserThread::UI unless called during early initialization before the main
-  // thread is promoted to BrowserThread::UI).
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
-         !BrowserThread::IsThreadInitialized(BrowserThread::UI));
-
-  // Sample the histogram also for the successful case in order to get a
-  // baseline on the success rate in addition to the error distribution.
-  UMA_HISTOGRAM_ENUMERATION("PrefService.ReadError", error,
-                            PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM);
-
-  if (error != PersistentPrefStore::PREF_READ_ERROR_NONE) {
-#if !defined(OS_CHROMEOS)
-    // Failing to load prefs on startup is a bad thing(TM). See bug 38352 for
-    // an example problem that this can cause.
-    // Do some diagnosis and try to avoid losing data.
-    int message_id = 0;
-    if (error <= PersistentPrefStore::PREF_READ_ERROR_JSON_TYPE) {
-      message_id = IDS_PREFERENCES_CORRUPT_ERROR;
-    } else if (error != PersistentPrefStore::PREF_READ_ERROR_NO_FILE) {
-      message_id = IDS_PREFERENCES_UNREADABLE_ERROR;
-    }
-
-    if (message_id) {
-      // Note: ThreadTaskRunnerHandle() is usually BrowserThread::UI but during
-      // early startup it can be ChromeBrowserMainParts::DeferringTaskRunner
-      // which will forward to BrowserThread::UI when it's initialized.
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE,
-          base::BindOnce(&ShowProfileErrorDialog, ProfileErrorType::PREFERENCES,
-                         message_id,
-                         sql::GetCorruptFileDiagnosticsInfo(pref_filename)));
-    }
-#else
-    // On ChromeOS error screen with message about broken local state
-    // will be displayed.
-
-    // A supplementary error message about broken local state - is included
-    // in logs and user feedbacks.
-    if (error != PersistentPrefStore::PREF_READ_ERROR_NONE &&
-        error != PersistentPrefStore::PREF_READ_ERROR_NO_FILE) {
-      LOG(ERROR) << "An error happened during prefs loading: " << error;
-    }
-#endif
-  }
-}
-
 std::unique_ptr<ProfilePrefStoreManager> CreateProfilePrefStoreManager(
     const base::FilePath& profile_path) {
   std::string legacy_device_id;
@@ -404,8 +354,8 @@ void PrepareFactory(sync_preferences::PrefServiceSyncableFactory* factory,
   factory->set_command_line_prefs(
       base::MakeRefCounted<ChromeCommandLinePrefStore>(
           base::CommandLine::ForCurrentProcess()));
-  factory->set_read_error_callback(
-      base::BindRepeating(&HandleReadError, pref_filename));
+  factory->set_read_error_callback(base::BindRepeating(
+      &chrome_prefs::HandlePersistentPrefStoreReadError, pref_filename));
   factory->set_user_prefs(std::move(user_pref_store));
   factory->SetPrefModelAssociatorClient(
       ChromePrefModelAssociatorClient::GetInstance());
@@ -456,20 +406,15 @@ std::unique_ptr<PrefService> CreateLocalState(
     policy::PolicyService* policy_service,
     scoped_refptr<PrefRegistry> pref_registry,
     bool async,
-    std::unique_ptr<PrefValueStore::Delegate> delegate,
-    scoped_refptr<PersistentPrefStore> user_pref_store) {
-  if (!user_pref_store) {
-    // The new JsonPrefStore will read content from |pref_filename| in
-    // PrefServiceSyncableFactory. Errors are ignored.
-    user_pref_store = base::MakeRefCounted<JsonPrefStore>(
-        pref_filename, std::unique_ptr<PrefFilter>());
-  }
+    std::unique_ptr<PrefValueStore::Delegate> delegate) {
   sync_preferences::PrefServiceSyncableFactory factory;
   PrepareFactory(&factory, pref_filename, policy_service,
                  nullptr,  // supervised_user_settings
-                 std::move(user_pref_store),
+                 base::MakeRefCounted<JsonPrefStore>(
+                     pref_filename, std::unique_ptr<PrefFilter>()),
                  nullptr,  // extension_prefs
                  async);
+
   return factory.Create(std::move(pref_registry), std::move(delegate));
 }
 
@@ -503,6 +448,18 @@ std::unique_ptr<sync_preferences::PrefServiceSyncable> CreateProfilePrefs(
   return factory.CreateSyncable(std::move(pref_registry), std::move(delegate));
 }
 
+void InstallPoliciesOnLocalState(
+    PrefService* preexisting_local_state,
+    policy::PolicyService* policy_service,
+    std::unique_ptr<PrefValueStore::Delegate> delegate) {
+  sync_preferences::PrefServiceSyncableFactory factory;
+  policy::BrowserPolicyConnector* policy_connector =
+      g_browser_process->browser_policy_connector();
+  factory.SetManagedPolicies(policy_service, policy_connector);
+  factory.SetRecommendedPolicies(policy_service, policy_connector);
+  factory.ChangePrefValueStore(preexisting_local_state, std::move(delegate));
+}
+
 void DisableDomainCheckForTesting() {
 #if defined(OS_WIN)
   g_disable_domain_check_for_testing = true;
@@ -530,4 +487,54 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   ProfilePrefStoreManager::RegisterProfilePrefs(registry);
 }
 
+void HandlePersistentPrefStoreReadError(
+    const base::FilePath& pref_filename,
+    PersistentPrefStore::PrefReadError error) {
+  // The error callback is always invoked back on the main thread (which is
+  // BrowserThread::UI unless called during early initialization before the main
+  // thread is promoted to BrowserThread::UI).
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsThreadInitialized(BrowserThread::UI));
+
+  // Sample the histogram also for the successful case in order to get a
+  // baseline on the success rate in addition to the error distribution.
+  UMA_HISTOGRAM_ENUMERATION("PrefService.ReadError", error,
+                            PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM);
+
+  if (error != PersistentPrefStore::PREF_READ_ERROR_NONE) {
+#if !defined(OS_CHROMEOS)
+    // Failing to load prefs on startup is a bad thing(TM). See bug 38352 for
+    // an example problem that this can cause.
+    // Do some diagnosis and try to avoid losing data.
+    int message_id = 0;
+    if (error <= PersistentPrefStore::PREF_READ_ERROR_JSON_TYPE) {
+      message_id = IDS_PREFERENCES_CORRUPT_ERROR;
+    } else if (error != PersistentPrefStore::PREF_READ_ERROR_NO_FILE) {
+      message_id = IDS_PREFERENCES_UNREADABLE_ERROR;
+    }
+
+    if (message_id) {
+      // Note: ThreadTaskRunnerHandle() is usually BrowserThread::UI but during
+      // early startup it can be ChromeBrowserMainParts::DeferringTaskRunner
+      // which will forward to BrowserThread::UI when it's initialized.
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::BindOnce(&ShowProfileErrorDialog, ProfileErrorType::PREFERENCES,
+                         message_id,
+                         sql::GetCorruptFileDiagnosticsInfo(pref_filename)));
+    }
+#else
+    // On ChromeOS error screen with message about broken local state
+    // will be displayed.
+
+    // A supplementary error message about broken local state - is included
+    // in logs and user feedbacks.
+    if (error != PersistentPrefStore::PREF_READ_ERROR_NONE &&
+        error != PersistentPrefStore::PREF_READ_ERROR_NO_FILE) {
+      LOG(ERROR) << "An error happened during prefs loading: " << error;
+    }
+#endif
+  }
+}
+
 }  // namespace chrome_prefs

chrome/browser/prefs/chrome_pref_service_factory.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/memory/ref_counted.h"
+#include "components/prefs/persistent_pref_store.h"
 #include "components/prefs/pref_value_store.h"
 #include "services/preferences/public/mojom/tracked_preference_validation_delegate.mojom.h"
 
@@ -63,16 +64,12 @@ extern const char kSettingsEnforcementGroupEnforceAlwaysWithExtensionsAndDSE[];
 // the created PrefService or NULL if creation has failed. Note, it is
 // guaranteed that in asynchronous version initialization happens after this
 // function returned.
-
-// |user_pref_store| instance is an existing pref store that the local state
-// PrefService uses as its persistent user pref store.
 std::unique_ptr<PrefService> CreateLocalState(
     const base::FilePath& pref_filename,
     policy::PolicyService* policy_service,
     scoped_refptr<PrefRegistry> pref_registry,
     bool async,
-    std::unique_ptr<PrefValueStore::Delegate> delegate,
-    scoped_refptr<PersistentPrefStore> user_pref_store);
+    std::unique_ptr<PrefValueStore::Delegate> delegate);
 
 std::unique_ptr<sync_preferences::PrefServiceSyncable> CreateProfilePrefs(
     const base::FilePath& pref_filename,
@@ -85,6 +82,18 @@ std::unique_ptr<sync_preferences::PrefServiceSyncable> CreateProfilePrefs(
     scoped_refptr<base::SequencedTaskRunner> io_task_runner,
     std::unique_ptr<PrefValueStore::Delegate> delegate);
 
+// Installs policy related PrefStores on |preexisting_local_state|.
+// |preexisting_local_state| instance is a local state that has user PrefStore
+// and commandline PrefStore initialized. It is missing the mandatory and
+// recommended PrefStores and this method will add them to it.
+// |policy_service| is used as the source for mandatory or recommended
+// policies.
+// |delegate| is passed to listen to PrefStore initialization events.
+void InstallPoliciesOnLocalState(
+    PrefService* preexisting_local_state,
+    policy::PolicyService* policy_service,
+    std::unique_ptr<PrefValueStore::Delegate> delegate);
+
 // Call before startup tasks kick in to ignore the presence of a domain when
 // determining the active SettingsEnforcement group. For testing only.
 void DisableDomainCheckForTesting();
@@ -106,6 +115,11 @@ void ClearResetTime(Profile* profile);
 // Register user prefs used by chrome preference system.
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
+// Shows notifications which correspond to PersistentPrefStore's reading errors.
+void HandlePersistentPrefStoreReadError(
+    const base::FilePath& pref_filename,
+    PersistentPrefStore::PrefReadError error);
+
 }  // namespace chrome_prefs
 
 #endif  // CHROME_BROWSER_PREFS_CHROME_PREF_SERVICE_FACTORY_H_