Fix early shutdown crash due to out-of-order destruction.

BrowserProcessImpl must take ownership of both
ChromeBrowserPolicyConnector and the Local State PrefService at the same
time. Failure to do so can result in the policy connector being
destroyed before the pref service, resulting in crashes.

This CL also cleans up some dead code left behind when creation of these
things was moved earlier in startup.

BUG=948265

Change-Id: Ia9bf5b9860381d8f865d1447c16d1674d22c4e4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362800Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800053}

chrome/browser/browser_process_impl.cc

@@ -226,18 +226,16 @@ rappor::RapporService* GetBrowserRapporService() {
   return nullptr;
 }
 
-BrowserProcessImpl::BrowserProcessImpl(StartupData* startup_data) {
+BrowserProcessImpl::BrowserProcessImpl(StartupData* startup_data)
+    : startup_data_(startup_data),
+      browser_policy_connector_(startup_data->chrome_feature_list_creator()
+                                    ->TakeChromeBrowserPolicyConnector()),
+      local_state_(
+          startup_data->chrome_feature_list_creator()->TakePrefService()),
+      platform_part_(std::make_unique<BrowserProcessPlatformPart>()) {
   g_browser_process = this;
 
   DCHECK(startup_data);
-  startup_data_ = startup_data;
-
-  chrome_feature_list_creator_ = startup_data->chrome_feature_list_creator();
-  browser_policy_connector_ =
-      chrome_feature_list_creator_->TakeChromeBrowserPolicyConnector();
-  created_browser_policy_connector_ = true;
-
-  platform_part_ = std::make_unique<BrowserProcessPlatformPart>();
   // Most work should be done in Init().
 }
 
@@ -700,8 +698,6 @@ ProfileManager* BrowserProcessImpl::profile_manager() {
 
 PrefService* BrowserProcessImpl::local_state() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!local_state_)
-    CreateLocalState();
   return local_state_.get();
 }
 
@@ -749,11 +745,6 @@ NotificationPlatformBridge* BrowserProcessImpl::notification_platform_bridge() {
 policy::ChromeBrowserPolicyConnector*
 BrowserProcessImpl::browser_policy_connector() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!created_browser_policy_connector_) {
-    DCHECK(!browser_policy_connector_);
-    browser_policy_connector_ = platform_part_->CreateBrowserPolicyConnector();
-    created_browser_policy_connector_ = true;
-  }
   return browser_policy_connector_.get();
 }
 
@@ -1115,13 +1106,6 @@ void BrowserProcessImpl::CreateProfileManager() {
   profile_manager_ = std::make_unique<ProfileManager>(user_data_dir);
 }
 
-void BrowserProcessImpl::CreateLocalState() {
-  DCHECK(!local_state_);
-
-  local_state_ = chrome_feature_list_creator_->TakePrefService();
-  DCHECK(local_state_);
-}
-
 void BrowserProcessImpl::PreCreateThreads(
     const base::CommandLine& command_line) {
 #if BUILDFLAG(ENABLE_EXTENSIONS)

chrome/browser/browser_process_impl.h

@@ -39,7 +39,6 @@
 #endif
 
 class BatteryMetrics;
-class ChromeFeatureListCreator;
 class ChromeMetricsServicesManagerClient;
 class DevToolsAutoOpener;
 class RemoteDebuggingServer;
@@ -211,7 +210,6 @@ class BrowserProcessImpl : public BrowserProcess,
 
   void CreateWatchdogThread();
   void CreateProfileManager();
-  void CreateLocalState();
   void CreateIconManager();
   void CreateIntranetRedirectDetector();
   void CreateNotificationPlatformBridge();
@@ -233,10 +231,11 @@ class BrowserProcessImpl : public BrowserProcess,
   void Pin();
   void Unpin();
 
+  StartupData* const startup_data_;
+
   bool created_watchdog_thread_ = false;
   std::unique_ptr<WatchDogThread> watchdog_thread_;
 
-  bool created_browser_policy_connector_ = false;
   // Must be destroyed after |local_state_|.
   std::unique_ptr<policy::ChromeBrowserPolicyConnector>
       browser_policy_connector_;
@@ -343,12 +342,6 @@ class BrowserProcessImpl : public BrowserProcess,
 
   scoped_refptr<DownloadRequestLimiter> download_request_limiter_;
 
-  // 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_;
-
-  StartupData* startup_data_;
-
   // Ensures that the observers of plugin/print disable/enable state
   // notifications are properly added and removed.
   PrefChangeRegistrar pref_change_registrar_;

chrome/browser/browser_process_platform_part_base.cc

@@ -7,8 +7,6 @@
 #include "base/notreached.h"
 #include "build/build_config.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
-#include "chrome/browser/policy/chrome_browser_policy_connector.h"
-#include "components/policy/core/browser/browser_policy_connector.h"
 
 BrowserProcessPlatformPartBase::BrowserProcessPlatformPartBase() {
 }
@@ -36,8 +34,3 @@ void BrowserProcessPlatformPartBase::AttemptExit(bool try_to_quit_application) {
 
 void BrowserProcessPlatformPartBase::PreMainMessageLoopRun() {
 }
-
-std::unique_ptr<policy::ChromeBrowserPolicyConnector>
-BrowserProcessPlatformPartBase::CreateBrowserPolicyConnector() {
-  return std::make_unique<policy::ChromeBrowserPolicyConnector>();
-}

chrome/browser/browser_process_platform_part_base.h

@@ -14,10 +14,6 @@ namespace base {
 class CommandLine;
 }
 
-namespace policy {
-class ChromeBrowserPolicyConnector;
-}
-
 // A base class for platform-specific BrowserProcessPlatformPart
 // implementations. This class itself should never be used verbatim.
 class BrowserProcessPlatformPartBase {
@@ -39,9 +35,6 @@ class BrowserProcessPlatformPartBase {
   // Called at the end of BrowserProcessImpl::PreMainMessageLoopRun().
   virtual void PreMainMessageLoopRun();
 
-  virtual std::unique_ptr<policy::ChromeBrowserPolicyConnector>
-  CreateBrowserPolicyConnector();
-
  private:
   DISALLOW_COPY_AND_ASSIGN(BrowserProcessPlatformPartBase);
 };

chrome/browser/browser_process_platform_part_chromeos.cc

@@ -242,12 +242,6 @@ void BrowserProcessPlatformPart::StartTearDown() {
   profile_helper_.reset();
 }
 
-std::unique_ptr<policy::ChromeBrowserPolicyConnector>
-BrowserProcessPlatformPart::CreateBrowserPolicyConnector() {
-  return std::unique_ptr<policy::ChromeBrowserPolicyConnector>(
-      new policy::BrowserPolicyConnectorChromeOS());
-}
-
 chromeos::system::SystemClock* BrowserProcessPlatformPart::GetSystemClock() {
   if (!system_clock_.get())
     system_clock_.reset(new chromeos::system::SystemClock());

chrome/browser/browser_process_platform_part_chromeos.h

@@ -117,8 +117,6 @@ class BrowserProcessPlatformPart : public BrowserProcessPlatformPartBase {
 
   // Overridden from BrowserProcessPlatformPartBase:
   void StartTearDown() override;
-  std::unique_ptr<policy::ChromeBrowserPolicyConnector>
-  CreateBrowserPolicyConnector() override;
 
   chromeos::system::SystemClock* GetSystemClock();
   void DestroySystemClock();

chrome/test/base/testing_browser_process.cc

@@ -65,6 +65,10 @@
 #include "components/keep_alive_registry/keep_alive_registry.h"
 #endif
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#endif
+
 // static
 TestingBrowserProcess* TestingBrowserProcess::GetGlobal() {
   return static_cast<TestingBrowserProcess*>(g_browser_process);
@@ -223,7 +227,13 @@ TestingBrowserProcess::browser_policy_connector() {
         chrome::DIR_POLICY_FILES, local_policy_path, true, false));
 #endif
 
-    browser_policy_connector_ = platform_part_->CreateBrowserPolicyConnector();
+#if defined(OS_CHROMEOS)
+    browser_policy_connector_ =
+        std::make_unique<policy::BrowserPolicyConnectorChromeOS>();
+#else
+    browser_policy_connector_ =
+        std::make_unique<policy::ChromeBrowserPolicyConnector>();
+#endif  // defined(OS_CHROMEOS)
 
     // Note: creating the ChromeBrowserPolicyConnector invokes BrowserThread::
     // GetTaskRunnerForThread(), which initializes a base::LazyInstance of