Add --force-first-run-dialog, regression tests for r511312.

Chrome has a modal first-run dialog displayed during browser startup
on Linux and Mac, before the main MessageLoop begins. It is supressed
under many scenarios, and completely disabled in Chromium builds,
making it very difficult to test (even manually).

Before r511312 Chrome would upload a crash dump if a SIGTERM or similar
was received while the dialog was showing on Mac.

This CL adds --force-first-run-dialog which works in official and non-
official builds for testing the dialog manually. Adds a regression
test for r511312 to test signal handling while this dialog is showing.
This also gives some bot coverage of the dialog code.

Consolidates repeated and redundant logic leading up to the dialog
invocation, allowing Linux/Mac to share the --force-first-run-dialog
logic as well.

The behaviour of the run loop used while the modal dialog runs also
changed in r511673, which could cause Chrome to ignore SIGTERM while
the dialog was showing. That's not ideal, so handle this as well.

Bug: 777660
Change-Id: I523966003eda01d43f15d0217e075e11838da38a
Reviewed-on: https://chromium-review.googlesource.com/734381Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517337}

chrome/browser/first_run/first_run_dialog.h

@@ -5,14 +5,27 @@
 #ifndef CHROME_BROWSER_FIRST_RUN_FIRST_RUN_DIALOG_H_
 #define CHROME_BROWSER_FIRST_RUN_FIRST_RUN_DIALOG_H_
 
+#include "base/callback_forward.h"
+#include "build/build_config.h"
+
+// Hide this function on platforms where the dialog does not exist.
+#if defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
+
 class Profile;
 
 namespace first_run {
 
-// Shows the first run dialog. Only called if IsOrganicFirstRun() is true.
-// Returns true if the dialog was shown.
-bool ShowFirstRunDialog(Profile* profile);
+// Shows the first run dialog. Only called for organic first runs on Mac and
+// desktop Linux official builds when metrics reporting is not already enabled.
+// Invokes ChangeMetricsReportingState() if consent is given to enable crash
+// reporting, and may initiate the flow to set the default browser.
+void ShowFirstRunDialog(Profile* profile);
+
+// Returns a Closure invoked before calling ShowFirstRunDialog(). For testing.
+base::OnceClosure& GetBeforeShowFirstRunDialogHookForTesting();
 
 }  // namespace first_run
 
+#endif  // OS_MACOSX || DESKTOP_LINUX
+
 #endif  // CHROME_BROWSER_FIRST_RUN_FIRST_RUN_DIALOG_H_

chrome/browser/first_run/first_run_internal_posix.cc

@@ -11,8 +11,10 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/first_run/first_run_dialog.h"
+#include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "chrome/installer/util/master_preferences.h"
 #include "components/metrics/metrics_pref_names.h"
@@ -21,26 +23,73 @@
 #include "components/startup_metric_utils/browser/startup_metric_utils.h"
 
 namespace first_run {
+
+#if !defined(OS_CHROMEOS)
+base::OnceClosure& GetBeforeShowFirstRunDialogHookForTesting() {
+  CR_DEFINE_STATIC_LOCAL(base::OnceClosure, closure, ());
+  return closure;
+}
+#endif  // OS_CHROMEOS
+
 namespace internal {
+namespace {
 
-void DoPostImportPlatformSpecificTasks(Profile* profile) {
 #if !defined(OS_CHROMEOS)
-  base::FilePath local_state_path;
-  PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
-  bool local_state_file_exists = base::PathExists(local_state_path);
-  // Launch the first run dialog only for certain builds, and only if the user
-  // has not already set preferences.
-  if (internal::IsOrganicFirstRun() && !local_state_file_exists) {
-    if (ShowFirstRunDialog(profile)) {
-      bool is_opt_in = first_run::IsMetricsReportingOptIn();
-      metrics::RecordMetricsReportingDefaultState(
-          g_browser_process->local_state(),
-          is_opt_in ? metrics::EnableMetricsDefault::OPT_IN
-                    : metrics::EnableMetricsDefault::OPT_OUT);
-      startup_metric_utils::SetNonBrowserUIDisplayed();
-    }
+// Returns whether the first run dialog should be shown. This is only true for
+// certain builds, and only if the user has not already set preferences. In a
+// real, official-build first run, initializes the default metrics reporting if
+// the dialog should be shown.
+bool ShouldShowFirstRunDialog() {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kForceFirstRunDialog)) {
+    return true;
   }
+
+#if !defined(GOOGLE_CHROME_BUILD)
+  // On non-official builds, only --force-first-run-dialog will show the dialog.
+  return false;
 #endif
+
+  base::FilePath local_state_path;
+  PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
+  if (base::PathExists(local_state_path))
+    return false;
+
+  if (!IsOrganicFirstRun())
+    return false;
+
+  // The purpose of the dialog is to ask the user to enable stats and crash
+  // reporting. This setting may be controlled through configuration management
+  // in enterprise scenarios. If that is the case, skip the dialog entirely, as
+  // it's not worth bothering the user for only the default browser question
+  // (which is likely to be forced in enterprise deployments anyway).
+  if (IsMetricsReportingPolicyManaged())
+    return false;
+
+  // For real first runs, Mac and Desktop Linux initialize the default metrics
+  // reporting state when the first run dialog is shown.
+  bool is_opt_in = first_run::IsMetricsReportingOptIn();
+  metrics::RecordMetricsReportingDefaultState(
+      g_browser_process->local_state(),
+      is_opt_in ? metrics::EnableMetricsDefault::OPT_IN
+                : metrics::EnableMetricsDefault::OPT_OUT);
+  return true;
+}
+#endif  // !OS_CHROMEOS
+
+}  // namespace
+
+void DoPostImportPlatformSpecificTasks(Profile* profile) {
+#if !defined(OS_CHROMEOS)
+  if (!ShouldShowFirstRunDialog())
+    return;
+
+  if (GetBeforeShowFirstRunDialogHookForTesting())
+    std::move(GetBeforeShowFirstRunDialogHookForTesting()).Run();
+
+  ShowFirstRunDialog(profile);
+  startup_metric_utils::SetNonBrowserUIDisplayed();
+#endif  // !OS_CHROMEOS
 }
 
 bool IsFirstRunSentinelPresent() {

chrome/browser/first_run/first_run_internal_posix_browsertest.cc

+// Copyright 2017 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.
+
+#include "chrome/test/base/in_process_browser_test.h"
+
+#include <signal.h>
+
+#include "base/command_line.h"
+#include "chrome/browser/first_run/first_run_dialog.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+
+namespace first_run {
+
+class FirstRunInternalPosixTest : public InProcessBrowserTest {
+ protected:
+  FirstRunInternalPosixTest() = default;
+
+  // InProcessBrowserTest:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kForceFirstRun);
+    command_line->AppendSwitch(switches::kForceFirstRunDialog);
+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
+    // The modal dialog will spawn and spin a nested RunLoop when
+    // content::BrowserTestBase::SetUp() invokes content::ContentMain().
+    // BrowserTestBase sets GetContentMainParams()->ui_task before this, but the
+    // ui_task isn't actually Run() until after the dialog is spawned in
+    // ChromeBrowserMainParts::PreMainMessageLoopRunImpl(). Instead, try to
+    // inspect state by posting a task to run in that nested RunLoop.
+    // There's no MessageLoop to enqueue a task on yet, there's also no
+    // content::NotificationService, or anything else sensible that would allow
+    // us to hook in a task. So use a testing-only Closure.
+    GetBeforeShowFirstRunDialogHookForTesting() = base::BindOnce(
+        &FirstRunInternalPosixTest::SetupNestedTask, base::Unretained(this));
+    EXPECT_FALSE(inspected_state_);
+  }
+
+  void TearDownInProcessBrowserTestFixture() override {
+    EXPECT_TRUE(inspected_state_);
+    // The test closure should have run. But clear the global in case it hasn't.
+    EXPECT_FALSE(GetBeforeShowFirstRunDialogHookForTesting());
+    GetBeforeShowFirstRunDialogHookForTesting().Reset();
+    InProcessBrowserTest::TearDownInProcessBrowserTestFixture();
+  }
+
+ private:
+  // A task run immediately before first_run::DoPostImportPlatformSpecificTasks
+  // shows the first-run dialog.
+  void SetupNestedTask() {
+    EXPECT_TRUE(base::SequencedTaskRunnerHandle::Get());
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&FirstRunInternalPosixTest::InspectState,
+                              base::Unretained(this)));
+  }
+
+  // A task queued up to run once the first-run dialog starts pumping messages
+  // in its modal loop.
+  void InspectState() {
+    // Send a signal to myself. This should post a task for the next run loop
+    // iteration to set browser_shutdown::IsTryingToQuit(), and interrupt the
+    // RunLoop.
+    raise(SIGTERM);
+    inspected_state_ = true;
+  }
+
+  bool inspected_state_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(FirstRunInternalPosixTest);
+};
+
+// Test the first run flow for showing the modal dialog that surfaces the first
+// run dialog. Ensure browser startup safely handles a signal while the modal
+// RunLoop is running.
+IN_PROC_BROWSER_TEST_F(FirstRunInternalPosixTest, HandleSigterm) {
+  // Never reached. PreMainMessageLoopRunImpl() should return before this task
+  // is run.
+  ADD_FAILURE() << "Should never be called";
+}
+
+}  // namespace first_run

chrome/browser/ui/cocoa/first_run_dialog.mm

@@ -17,12 +17,11 @@
 #include "chrome/browser/first_run/first_run_dialog.h"
 #include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/ui/cocoa/first_run_dialog_controller.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
-#include "components/search_engines/template_url_service.h"
+#include "content/public/common/content_switches.h"
 #import "third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 #include "url/gurl.h"
@@ -38,7 +37,7 @@ class FirstRunShowBridge : public base::RefCounted<FirstRunShowBridge> {
  public:
   FirstRunShowBridge(FirstRunDialogController* controller);
 
-  void ShowDialog();
+  void ShowDialog(const base::Closure& quit_closure);
 
  private:
   friend class base::RefCounted<FirstRunShowBridge>;
@@ -52,23 +51,18 @@ FirstRunShowBridge::FirstRunShowBridge(
     FirstRunDialogController* controller) : controller_(controller) {
 }
 
-void FirstRunShowBridge::ShowDialog() {
+void FirstRunShowBridge::ShowDialog(const base::Closure& quit_closure) {
+  // Proceeding past the modal dialog requires user interaction. Allow nested
+  // tasks to run so that signal handlers operate correctly.
+  base::MessageLoop::ScopedNestableTaskAllower allow_nested(
+      base::MessageLoop::current());
   [controller_ show];
-  base::RunLoop::QuitCurrentDeprecated();
+  quit_closure.Run();
 }
 
 FirstRunShowBridge::~FirstRunShowBridge() {}
 
-// Show the first run UI.
-// Returns true if the first run dialog was shown.
-bool ShowFirstRunModal(Profile* profile) {
-  // The purpose of the dialog is to ask the user to enable stats and crash
-  // reporting. This setting may be controlled through configuration management
-  // in enterprise scenarios. If that is the case, skip the dialog entirely, as
-  // it's not worth bothering the user for only the default browser question
-  // (which is likely to be forced in enterprise deployments anyway).
-  if (IsMetricsReportingPolicyManaged())
-    return false;
+void ShowFirstRunModal(Profile* profile) {
   base::scoped_nsobject<FirstRunDialogController> dialog(
       [[FirstRunDialogController alloc] init]);
 
@@ -79,14 +73,15 @@ bool ShowFirstRunModal(Profile* profile) {
   bool consent_given = [dialog.get() isStatsReportingEnabled];
   ChangeMetricsReportingState(consent_given);
 
-  // If selected set as default browser.
-  BOOL make_default_browser = [dialog.get() isMakeDefaultBrowserEnabled];
+  // If selected, set as default browser. Skip in automated tests so that an OS
+  // dialog confirming the default browser choice isn't left on screen.
+  BOOL make_default_browser =
+      [dialog.get() isMakeDefaultBrowserEnabled] &&
+      !base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType);
   if (make_default_browser) {
     bool success = shell_integration::SetAsDefaultBrowser();
     DCHECK(success);
   }
-
-  return true;
 }
 
 // True when the stats checkbox should be checked by default. This is only
@@ -96,34 +91,12 @@ bool StatsCheckboxDefault() {
   return !first_run::IsMetricsReportingOptIn();
 }
 
-bool IsFirstRunEnabledForBuildType() {
-#if defined(GOOGLE_CHROME_BUILD)
-  return true;
-#else
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kForceUnofficialFirstRun);
-#endif
-}
-
 }  // namespace
 
 namespace first_run {
 
-bool ShowFirstRunDialog(Profile* profile) {
-  bool dialog_shown = false;
-  if (IsFirstRunEnabledForBuildType())
-    dialog_shown = ShowFirstRunModal(profile);
-  // Set preference to show first run bubble and welcome page.
-  // Only display the bubble if there is a default search provider.
-  TemplateURLService* search_engines_model =
-      TemplateURLServiceFactory::GetForProfile(profile);
-  if (search_engines_model &&
-      search_engines_model->GetDefaultSearchProvider()) {
-    first_run::SetShowFirstRunBubblePref(first_run::FIRST_RUN_BUBBLE_SHOW);
-  }
-  first_run::SetShouldShowWelcomePage();
-
-  return dialog_shown;
+void ShowFirstRunDialog(Profile* profile) {
+  ShowFirstRunModal(profile);
 }
 
 }  // namespace first_run
@@ -164,9 +137,11 @@ bool ShowFirstRunDialog(Profile* profile) {
   // Therefore the main MessageLoop is run so things work.
 
   scoped_refptr<FirstRunShowBridge> bridge(new FirstRunShowBridge(self));
+  base::RunLoop run_loop;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&FirstRunShowBridge::ShowDialog, bridge.get()));
-  base::RunLoop().Run();
+      FROM_HERE, base::Bind(&FirstRunShowBridge::ShowDialog, bridge.get(),
+                            run_loop.QuitClosure()));
+  run_loop.Run();
 }
 
 - (void)show {

chrome/browser/ui/views/first_run_dialog.cc

@@ -48,32 +48,22 @@ void InitCrashReporterIfEnabled(bool enabled) {
 
 namespace first_run {
 
-bool ShowFirstRunDialog(Profile* profile) {
-  return FirstRunDialog::Show(profile);
+void ShowFirstRunDialog(Profile* profile) {
+  FirstRunDialog::Show(profile);
 }
 
 }  // namespace first_run
 
 // static
-bool FirstRunDialog::Show(Profile* profile) {
-  bool dialog_shown = false;
-
-#if defined(GOOGLE_CHROME_BUILD)
-  // If the metrics reporting is managed, we won't ask.
-  if (!IsMetricsReportingPolicyManaged()) {
-    FirstRunDialog* dialog = new FirstRunDialog(profile);
-    views::DialogDelegate::CreateDialogWidget(dialog, NULL, NULL)->Show();
-
-    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
-    base::MessageLoopForUI::ScopedNestableTaskAllower allow_nested(loop);
-    base::RunLoop run_loop;
-    dialog->quit_runloop_ = run_loop.QuitClosure();
-    run_loop.Run();
-    dialog_shown = true;
-  }
-#endif  // defined(GOOGLE_CHROME_BUILD)
-
-  return dialog_shown;
+void FirstRunDialog::Show(Profile* profile) {
+  FirstRunDialog* dialog = new FirstRunDialog(profile);
+  views::DialogDelegate::CreateDialogWidget(dialog, NULL, NULL)->Show();
+
+  base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+  base::MessageLoopForUI::ScopedNestableTaskAllower allow_nested(loop);
+  base::RunLoop run_loop;
+  dialog->quit_runloop_ = run_loop.QuitClosure();
+  run_loop.Run();
 }
 
 FirstRunDialog::FirstRunDialog(Profile* profile)

chrome/browser/ui/views/first_run_dialog.h

@@ -21,8 +21,7 @@ class FirstRunDialog : public views::DialogDelegateView,
                        public views::LinkListener {
  public:
   // Displays the first run UI for reporting opt-in, import data etc.
-  // Returns true if the dialog was shown.
-  static bool Show(Profile* profile);
+  static void Show(Profile* profile);
 
  private:
   explicit FirstRunDialog(Profile* profile);

chrome/common/chrome_switches.cc

@@ -436,6 +436,11 @@ const char kForceEnableMetricsReporting[] = "force-enable-metrics-reporting";
 // whether or not it's actually the First Run (this overrides kNoFirstRun).
 const char kForceFirstRun[]                 = "force-first-run";
 
+// Shows the modal first run dialog during browser startup. This is shown for
+// the "organic" first run experience (Chrome downloaded, empty user data dir).
+// This does nothing without --force-first-run also being set.
+const char kForceFirstRunDialog[] = "force-first-run-dialog";
+
 // Forces Chrome to use localNTP instead of server (GWS) NTP.
 const char kForceLocalNtp[]                 = "force-local-ntp";
 
@@ -906,10 +911,6 @@ const char kEnableMacViewsNativeAppWindows[] =
 // Enables Translate experimental new UX which replaces the infobar.
 const char kEnableTranslateNewUX[] = "enable-translate-new-ux";
 
-// Forces the first-run flow even on unofficial builds. Note that this still
-// requires a fresh user-data-dir.
-const char kForceUnofficialFirstRun[] = "force-unofficial-first-run";
-
 // Shows a notification when quitting Chrome with hosted apps running. Default
 // behavior is to also quit all hosted apps.
 const char kHostedAppQuitNotification[] = "enable-hosted-app-quit-notification";

chrome/common/chrome_switches.h

@@ -138,6 +138,7 @@ extern const char kForceDesktopIOSPromotion[];
 extern const char kForceEffectiveConnectionType[];
 extern const char kForceEnableMetricsReporting[];
 extern const char kForceFirstRun[];
+extern const char kForceFirstRunDialog[];
 extern const char kForceLocalNtp[];
 extern const char kHomePage[];
 extern const char kIgnoreUrlFetcherCertRequests[];
@@ -278,7 +279,6 @@ extern const char kEnableHostedAppsInWindows[];
 extern const char kEnableMacViewsNativeAppWindows[];
 extern const char kEnableTranslateNewUX[];
 extern const char kEnableUserMetrics[];
-extern const char kForceUnofficialFirstRun[];
 extern const char kHostedAppQuitNotification[];
 extern const char kMetricsClientID[];
 extern const char kRelauncherProcess[];

chrome/test/BUILD.gn

@@ -1834,6 +1834,10 @@ test("browser_tests") {
     if (is_mac || is_win || is_chromeos) {
       sources += [ "../browser/extensions/api/networking_cast_private/networking_cast_private_apitest.cc" ]
     }
+    if (is_desktop_linux || is_mac) {
+      sources +=
+          [ "../browser/first_run/first_run_internal_posix_browsertest.cc" ]
+    }
     if (enable_app_list) {
       sources += [
         "../browser/apps/drive/drive_app_converter_browsertest.cc",