[headless] Move PrefService to HeadlessBrowserMainParts

This allows us to CommitPendingWrite() in PostMainMessageLoopRun(),
while it is still safe to do so, and avoid crashes on shutdown.

Previously, PrefService was doing non-trivial work during
HeadlessContentMainDelegate::~HeadlessContentMainDelegate() that
ended up with SIGABRT on MacOS.

Drive-by: also SetEncryptionKey() in network service on Mac,
similar to Windows.

Bug: 1050905
TBR: gab@chromium.org
Change-Id: I70e632d5f3a03a68b9ef90458315e2c2a516e1f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2634681
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844378}

headless/BUILD.gn

@@ -250,6 +250,7 @@ source_set("headless_shared_sources") {
     "//headless:headless",
     "//headless:headless_non_renderer",
   ]
+  defines = []
 
   sources = [
     "app/headless_shell_switches.cc",
@@ -298,6 +299,10 @@ source_set("headless_shared_sources") {
   }
 
   configs += [ ":inside_headless_component" ]
+
+  if (headless_use_prefs) {
+    defines += [ "HEADLESS_USE_PREFS" ]
+  }
 }
 
 # Code that is needed in a renderer process.
@@ -595,6 +600,7 @@ test("headless_unittests") {
     "public/domains/types_unittest.cc",
     "public/util/error_reporter_unittest.cc",
   ]
+  defines = []
 
   deps = [
     ":headless_shell_lib",
@@ -612,6 +618,10 @@ test("headless_unittests") {
     deps += [ "//components/crash/core/app:crash_export_thunks" ]
   }
 
+  if (headless_use_prefs) {
+    defines += [ "HEADLESS_USE_PREFS" ]
+  }
+
   if (enable_basic_printing) {
     sources += [ "lib/browser/headless_printing_unittest.cc" ]
     deps += [
@@ -735,6 +745,7 @@ static_library("headless_shell_lib") {
     "app/headless_shell.h",
     "public/headless_shell.h",
   ]
+  defines = []
 
   if (!is_component_build) {
     sources += [
@@ -788,6 +799,10 @@ static_library("headless_shell_lib") {
   if (is_mac) {
     deps += [ "//components/os_crypt" ]
   }
+
+  if (headless_use_prefs) {
+    defines += [ "HEADLESS_USE_PREFS" ]
+  }
 }
 
 if (is_fuchsia) {
@@ -805,6 +820,7 @@ if (is_fuchsia) {
 
 executable("headless_shell") {
   sources = [ "app/headless_shell_main.cc" ]
+  defines = []
 
   deps = [ ":headless_shell_lib" ]
 
@@ -819,6 +835,10 @@ executable("headless_shell") {
   if (is_mac) {
     deps += [ "//sandbox/mac:seatbelt" ]
   }
+
+  if (headless_use_prefs) {
+    defines += [ "HEADLESS_USE_PREFS" ]
+  }
 }
 
 process_version("version_header") {
@@ -832,9 +852,14 @@ process_version("version_header") {
 
 executable("headless_example") {
   sources = [ "app/headless_example.cc" ]
+  defines = []
 
   deps = [
     ":headless_shell_lib",
     "//skia",  # we need this to override font render hinting in headless build
   ]
+
+  if (headless_use_prefs) {
+    defines += [ "HEADLESS_USE_PREFS" ]
+  }
 }

headless/lib/DEPS

@@ -5,12 +5,8 @@ specific_include_rules = {
   "headless_content_client.cc": [
       "+components/embedder_support/origin_trials",
   ],
-  "headless_content_main_delegate.h": [
-    "+components/prefs",
-  ],
   "headless_content_main_delegate.cc": [
     "+cc/base/switches.h",
-    "+components/prefs",
     "+components/viz/common/switches.h",
     "+gpu/config/gpu_switches.h",
     "+third_party/blink/public/common/switches.h",

headless/lib/browser/DEPS

@@ -18,3 +18,11 @@ include_rules = [
   "+ui/compositor",
   "+ui/events/keycodes/dom",
 ]
+specific_include_rules = {
+  "headless_browser_main_parts.h": [
+    "+components/prefs",
+  ],
+  "headless_browser_main_parts.cc": [
+    "+components/prefs",
+  ],
+}

headless/lib/browser/headless_browser_main_parts.cc

@@ -9,8 +9,23 @@
 #include "headless/lib/browser/headless_devtools.h"
 #include "headless/lib/browser/headless_screen.h"
 
+#if defined(HEADLESS_USE_PREFS)
+#include "components/os_crypt/os_crypt.h"
+#include "components/prefs/json_pref_store.h"
+#include "components/prefs/pref_service_factory.h"
+#endif
+
 namespace headless {
 
+namespace {
+
+#if defined(HEADLESS_USE_PREFS)
+const base::FilePath::CharType kLocalStateFilename[] =
+    FILE_PATH_LITERAL("Local State");
+#endif
+
+}  // namespace
+
 HeadlessBrowserMainParts::HeadlessBrowserMainParts(
     const content::MainFunctionParams& parameters,
     HeadlessBrowserImpl* browser)
@@ -19,6 +34,9 @@ HeadlessBrowserMainParts::HeadlessBrowserMainParts(
 HeadlessBrowserMainParts::~HeadlessBrowserMainParts() = default;
 
 void HeadlessBrowserMainParts::PreMainMessageLoopRun() {
+#if defined(HEADLESS_USE_PREFS)
+  CreatePrefService();
+#endif
   if (browser_->options()->DevtoolsServerEnabled()) {
     StartLocalDevToolsHttpHandler(browser_);
     devtools_http_handler_started_ = true;
@@ -47,6 +65,10 @@ void HeadlessBrowserMainParts::PostMainMessageLoopRun() {
     StopLocalDevToolsHttpHandler();
     devtools_http_handler_started_ = false;
   }
+#if defined(HEADLESS_USE_PREFS)
+  if (local_state_)
+    local_state_->CommitPendingWrite();
+#endif
 }
 
 void HeadlessBrowserMainParts::QuitMainMessageLoop() {
@@ -54,4 +76,33 @@ void HeadlessBrowserMainParts::QuitMainMessageLoop() {
     std::move(quit_main_message_loop_).Run();
 }
 
+#if defined(HEADLESS_USE_PREFS)
+void HeadlessBrowserMainParts::CreatePrefService() {
+  if (browser_->options()->user_data_dir.empty()) {
+    LOG(WARNING) << "Cannot create Pref Service with no user data dir.";
+    return;
+  }
+
+  base::FilePath local_state_file =
+      browser_->options()->user_data_dir.Append(kLocalStateFilename);
+  auto pref_store = base::MakeRefCounted<JsonPrefStore>(local_state_file);
+  auto result = pref_store->ReadPrefs();
+  CHECK(result == JsonPrefStore::PREF_READ_ERROR_NONE ||
+        result == JsonPrefStore::PREF_READ_ERROR_NO_FILE);
+
+  auto pref_registry = base::MakeRefCounted<PrefRegistrySimple>();
+#if defined(OS_WIN)
+  OSCrypt::RegisterLocalPrefs(pref_registry.get());
+#endif
+
+  PrefServiceFactory factory;
+  factory.set_user_prefs(pref_store);
+  local_state_ = factory.Create(std::move(pref_registry));
+
+#if defined(OS_WIN)
+  CHECK(OSCrypt::Init(local_state_.get()));
+#endif
+}
+#endif  // defined(HEADLESS_USE_PREFS)
+
 }  // namespace headless

headless/lib/browser/headless_browser_main_parts.h

@@ -13,6 +13,11 @@
 #include "content/public/common/main_function_params.h"
 #include "headless/public/headless_browser.h"
 
+#if defined(HEADLESS_USE_PREFS)
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#endif
+
 namespace headless {
 
 class HeadlessBrowserImpl;
@@ -38,9 +43,17 @@ class HeadlessBrowserMainParts : public content::BrowserMainParts {
   void QuitMainMessageLoop();
 
  private:
+#if defined(HEADLESS_USE_PREFS)
+  void CreatePrefService();
+#endif
+
   const content::MainFunctionParams parameters_;  // For running browser tests.
   HeadlessBrowserImpl* browser_;  // Not owned.
 
+#if defined(HEADLESS_USE_PREFS)
+  std::unique_ptr<PrefService> local_state_;
+#endif
+
   bool run_message_loop_ = true;
   bool devtools_http_handler_started_ = false;
   base::OnceClosure quit_main_message_loop_;

headless/lib/headless_content_main_delegate.cc

@@ -47,12 +47,6 @@
 #include "headless/embedded_resource_pak.h"
 #endif
 
-#if defined(HEADLESS_USE_PREFS)
-#include "components/os_crypt/os_crypt.h"
-#include "components/prefs/json_pref_store.h"
-#include "components/prefs/pref_service_factory.h"
-#endif
-
 #if defined(OS_MAC) || defined(OS_WIN)
 #include "components/crash/core/app/crashpad.h"
 #endif
@@ -75,11 +69,6 @@ const base::Feature kVirtualTime{"VirtualTime",
 const base::FilePath::CharType kDefaultProfileName[] =
     FILE_PATH_LITERAL("Default");
 
-#if defined(HEADLESS_USE_PREFS)
-const base::FilePath::CharType kLocalStateFilename[] =
-    FILE_PATH_LITERAL("Local State");
-#endif
-
 namespace {
 
 // Keep in sync with content/common/content_constants_internal.h.
@@ -495,9 +484,6 @@ HeadlessContentMainDelegate::CreateContentUtilityClient() {
 
 void HeadlessContentMainDelegate::PostEarlyInitialization(
     bool is_running_tests) {
-#if defined(HEADLESS_USE_PREFS)
-  CreatePrefService();
-#endif
   if (base::FeatureList::IsEnabled(features::kVirtualTime)) {
     // Only pass viz flags into the virtual time mode.
     const char* const switches[] = {
@@ -524,33 +510,4 @@ void HeadlessContentMainDelegate::PostEarlyInitialization(
   }
 }
 
-#if defined(HEADLESS_USE_PREFS)
-void HeadlessContentMainDelegate::CreatePrefService() {
-  if (options()->user_data_dir.empty()) {
-    LOG(WARNING) << "Cannot create Pref Service with no user data dir.";
-    return;
-  }
-
-  base::FilePath local_state_file =
-      options()->user_data_dir.Append(kLocalStateFilename);
-  auto pref_store = base::MakeRefCounted<JsonPrefStore>(local_state_file);
-  auto result = pref_store->ReadPrefs();
-  CHECK(result == JsonPrefStore::PREF_READ_ERROR_NONE ||
-        result == JsonPrefStore::PREF_READ_ERROR_NO_FILE);
-
-  auto pref_registry = base::MakeRefCounted<PrefRegistrySimple>();
-#if defined(OS_WIN)
-  OSCrypt::RegisterLocalPrefs(pref_registry.get());
-#endif
-
-  PrefServiceFactory factory;
-  factory.set_user_prefs(pref_store);
-  local_state_ = factory.Create(std::move(pref_registry));
-
-#if defined(OS_WIN)
-  CHECK(OSCrypt::Init(local_state_.get()));
-#endif
-}
-#endif  // defined(HEADLESS_USE_PREFS)
-
 }  // namespace headless

headless/lib/headless_content_main_delegate.h

@@ -19,11 +19,6 @@
 #include "headless/public/headless_browser.h"
 #include "headless/public/headless_export.h"
 
-#if defined(HEADLESS_USE_PREFS)
-#include "components/prefs/pref_registry_simple.h"
-#include "components/prefs/pref_service.h"
-#endif
-
 namespace base {
 namespace debug {
 struct CrashKeyString;
@@ -78,10 +73,6 @@ class HEADLESS_EXPORT HeadlessContentMainDelegate
   void InitLogging(const base::CommandLine& command_line);
   void InitCrashReporter(const base::CommandLine& command_line);
 
-#if defined(HEADLESS_USE_PREFS)
-  void CreatePrefService();
-#endif
-
   std::unique_ptr<content::ContentRendererClient> renderer_client_;
   std::unique_ptr<content::ContentBrowserClient> browser_client_;
   std::unique_ptr<content::ContentUtilityClient> utility_client_;
@@ -91,10 +82,6 @@ class HEADLESS_EXPORT HeadlessContentMainDelegate
   std::unique_ptr<HeadlessBrowserImpl> browser_;
   std::unique_ptr<HeadlessBrowser::Options> options_;
 
-#if defined(HEADLESS_USE_PREFS)
-  std::unique_ptr<PrefService> local_state_;
-#endif
-
   base::debug::CrashKeyString* headless_crash_key_;  // Note: never deallocated.
 
   DISALLOW_COPY_AND_ASSIGN(HeadlessContentMainDelegate);