Browser shutdown cleanups (no functional changes).

- Make ShutdownType an enum class.
- Introduce browser_shutdown::HasShutdownStarted as a convenience.
- Move some Win-specific relaunch code from AttemptRestart, which runs
  fairly early in a restart that may never actually happen, into
  upgrade_util::RelaunchChromeBrowserImpl, which performs the actual
  restart.
- Switch from UMA macros to functions for one-shot metrics.

BUG=958865

Change-Id: I75cd08c712d8bb7c674b4b5e413d79abc874bc39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1910098Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Auto-Submit: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715730}

chrome/browser/background/background_mode_optimizer.cc

@@ -76,7 +76,7 @@ void BackgroundModeOptimizer::TryBrowserRestart() {
 
   // If the application is already shutting down, do not turn it into a restart.
   if (browser_shutdown::IsTryingToQuit() ||
-      browser_shutdown::GetShutdownType() != browser_shutdown::NOT_VALID) {
+      browser_shutdown::HasShutdownStarted()) {
     DVLOG(1) << "TryBrowserRestart: Cancelled because we are shutting down.";
     return;
   }

chrome/browser/chrome_content_browser_client.cc

@@ -1270,7 +1270,7 @@ void ChromeContentBrowserClient::SetBrowserStartupIsCompleteForTesting() {
 }
 
 bool ChromeContentBrowserClient::IsShuttingDown() {
-  return browser_shutdown::GetShutdownType() != browser_shutdown::NOT_VALID;
+  return browser_shutdown::HasShutdownStarted();
 }
 
 std::string ChromeContentBrowserClient::GetStoragePartitionIdForSite(

chrome/browser/chromeos/login/ui/login_display_host_webui.cc

@@ -714,7 +714,7 @@ void LoginDisplayHostWebUI::Observe(
 
 void LoginDisplayHostWebUI::RenderProcessGone(base::TerminationStatus status) {
   // Do not try to restore on shutdown
-  if (browser_shutdown::GetShutdownType() != browser_shutdown::NOT_VALID)
+  if (browser_shutdown::HasShutdownStarted())
     return;
 
   crash_count_++;

chrome/browser/first_run/upgrade_util_win.cc

@@ -32,7 +32,9 @@
 #include "build/branding_buildflags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/first_run/upgrade_util_win.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/shell_integration.h"
+#include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/install_static/install_util.h"
@@ -99,6 +101,21 @@ bool InvokeGoogleUpdateForRename() {
 namespace upgrade_util {
 
 bool RelaunchChromeBrowserImpl(const base::CommandLine& command_line) {
+  // Breakpad will upload crash reports if the breakpad pipe name environment
+  // variable is defined. So we undefine this environment variable before
+  // restarting, as the restarted processes will inherit their environment
+  // variables from ours, thus suppressing crash uploads.
+  if (!ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled()) {
+    HMODULE exe_module = GetModuleHandle(chrome::kBrowserProcessExecutableName);
+    if (exe_module) {
+      typedef void(__cdecl * ClearBreakpadPipeEnvVar)();
+      ClearBreakpadPipeEnvVar clear = reinterpret_cast<ClearBreakpadPipeEnvVar>(
+          GetProcAddress(exe_module, "ClearBreakpadPipeEnvironmentVariable"));
+      if (clear)
+        clear();
+    }
+  }
+
   base::FilePath chrome_exe;
   if (!base::PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED();

chrome/browser/lifetime/application_lifetime.cc

@@ -7,10 +7,6 @@
 #include <memory>
 #include <string>
 
-#if defined(OS_WIN)
-#include <windows.h>
-#endif
-
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/process/process.h"
@@ -27,7 +23,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/buildflags.h"
-#include "chrome/common/chrome_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/keep_alive_registry/keep_alive_registry.h"
 #include "components/language/core/browser/pref_names.h"
@@ -61,10 +56,10 @@
 
 #if defined(OS_WIN)
 #include "base/win/win_util.h"
-#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #endif
 
 namespace chrome {
+
 namespace {
 
 #if !defined(OS_ANDROID)
@@ -229,23 +224,6 @@ void AttemptUserExit() {
 // The Android implementation is in application_lifetime_android.cc
 #if !defined(OS_ANDROID)
 void AttemptRestart() {
-#if defined(OS_WIN)
-  // On Windows, Breakpad will upload crash reports if the breakpad pipe name
-  // environment variable is defined. So we undefine this environment variable
-  // before restarting, as the restarted processes will inherit their
-  // environment variables from ours, thus suppressing crash uploads.
-  if (!ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled()) {
-    HMODULE exe_module = GetModuleHandle(kBrowserProcessExecutableName);
-    if (exe_module) {
-      typedef void (__cdecl *ClearBreakpadPipeEnvVar)();
-      ClearBreakpadPipeEnvVar clear = reinterpret_cast<ClearBreakpadPipeEnvVar>(
-          GetProcAddress(exe_module, "ClearBreakpadPipeEnvironmentVariable"));
-      if (clear)
-        clear();
-    }
-  }
-#endif  // defined(OS_WIN)
-
   // TODO(beng): Can this use ProfileManager::GetLoadedProfiles instead?
   for (auto* browser : *BrowserList::GetInstance())
     content::BrowserContext::SaveSessionState(browser->profile());
@@ -307,10 +285,9 @@ void ExitIgnoreUnloadHandlers() {
   // In this case, AreAllBrowsersCloseable()
   // can be false in following cases. a) power-off b) signout from
   // screen locker.
-  if (!AreAllBrowsersCloseable())
-    browser_shutdown::OnShutdownStarting(browser_shutdown::END_SESSION);
-  else
-    browser_shutdown::OnShutdownStarting(browser_shutdown::BROWSER_EXIT);
+  browser_shutdown::OnShutdownStarting(
+      AreAllBrowsersCloseable() ? browser_shutdown::ShutdownType::kBrowserExit
+                                : browser_shutdown::ShutdownType::kEndSession);
 #endif
   AttemptExitInternal(true);
 }
@@ -346,7 +323,8 @@ void SessionEnding() {
   ShutdownWatcherHelper shutdown_watcher;
   shutdown_watcher.Arm(base::TimeDelta::FromSeconds(90));
 
-  browser_shutdown::OnShutdownStarting(browser_shutdown::END_SESSION);
+  browser_shutdown::OnShutdownStarting(
+      browser_shutdown::ShutdownType::kEndSession);
 
   // In a clean shutdown, browser_shutdown::OnShutdownStarting sets
   // g_shutdown_type, and browser_shutdown::ShutdownPreThreadsStop calls
@@ -366,6 +344,7 @@ void SessionEnding() {
 #if defined(OS_WIN)
   base::win::SetShouldCrashOnProcessDetach(false);
 #endif
+
   // On Windows 7 and later, the system will consider the process ripe for
   // termination as soon as it hides or destroys its windows. Since any
   // execution past that point will be non-deterministically cut short, we

chrome/browser/lifetime/browser_close_manager.cc

@@ -51,7 +51,8 @@ BrowserCloseManager::~BrowserCloseManager() {
 void BrowserCloseManager::StartClosingBrowsers() {
   // If the session is ending, skip straight to closing the browsers. There's no
   // time to wait for beforeunload dialogs.
-  if (browser_shutdown::GetShutdownType() == browser_shutdown::END_SESSION) {
+  if (browser_shutdown::GetShutdownType() ==
+      browser_shutdown::ShutdownType::kEndSession) {
     // Tell everyone that we are shutting down.
     browser_shutdown::SetTryingToQuit(true);
     CloseBrowsers();
@@ -162,8 +163,8 @@ void BrowserCloseManager::CloseBrowsers() {
             BrowserList::GetInstance()->end(),
             std::back_inserter(browser_list_copy));
 
-  bool session_ending =
-      browser_shutdown::GetShutdownType() == browser_shutdown::END_SESSION;
+  bool session_ending = browser_shutdown::GetShutdownType() ==
+                        browser_shutdown::ShutdownType::kEndSession;
 
   for (auto* browser : browser_list_copy) {
     browser->window()->Close();

chrome/browser/lifetime/browser_shutdown.cc

@@ -14,7 +14,7 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task/post_task.h"
@@ -76,7 +76,7 @@ namespace {
 bool g_trying_to_quit = false;
 
 base::Time* g_shutdown_started = nullptr;
-ShutdownType g_shutdown_type = NOT_VALID;
+ShutdownType g_shutdown_type = ShutdownType::kNotValid;
 int g_shutdown_num_processes;
 int g_shutdown_num_processes_slow;
 
@@ -90,14 +90,14 @@ base::FilePath GetShutdownMsPath() {
 
 const char* ToShutdownTypeString(ShutdownType type) {
   switch (type) {
-    case NOT_VALID:
+    case ShutdownType::kNotValid:
       NOTREACHED();
-      return "";
-    case WINDOW_CLOSE:
+      break;
+    case ShutdownType::kWindowClose:
       return "close";
-    case BROWSER_EXIT:
+    case ShutdownType::kBrowserExit:
       return "exit";
-    case END_SESSION:
+    case ShutdownType::kEndSession:
       return "end";
   }
   return "";
@@ -106,18 +106,15 @@ const char* ToShutdownTypeString(ShutdownType type) {
 }  // namespace
 
 void RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterIntegerPref(prefs::kShutdownType, NOT_VALID);
+  registry->RegisterIntegerPref(prefs::kShutdownType,
+                                static_cast<int>(ShutdownType::kNotValid));
   registry->RegisterIntegerPref(prefs::kShutdownNumProcesses, 0);
   registry->RegisterIntegerPref(prefs::kShutdownNumProcessesSlow, 0);
   registry->RegisterBooleanPref(prefs::kRestartLastSessionOnShutdown, false);
 }
 
-ShutdownType GetShutdownType() {
-  return g_shutdown_type;
-}
-
 void OnShutdownStarting(ShutdownType type) {
-  if (g_shutdown_type != NOT_VALID)
+  if (g_shutdown_type != ShutdownType::kNotValid)
     return;
 
   static crash_reporter::CrashKeyString<8> shutdown_type_key("shutdown-type");
@@ -145,6 +142,14 @@ void OnShutdownStarting(ShutdownType type) {
   }
 }
 
+bool HasShutdownStarted() {
+  return g_shutdown_type != ShutdownType::kNotValid;
+}
+
+ShutdownType GetShutdownType() {
+  return g_shutdown_type;
+}
+
 #if !defined(OS_ANDROID)
 bool ShutdownPreThreadsStop() {
 #if defined(OS_CHROMEOS)
@@ -183,10 +188,11 @@ bool ShutdownPreThreadsStop() {
 
 bool RecordShutdownInfoPrefs() {
   PrefService* prefs = g_browser_process->local_state();
-  if (g_shutdown_type > NOT_VALID && g_shutdown_num_processes > 0) {
+  if (g_shutdown_type != ShutdownType::kNotValid &&
+      g_shutdown_num_processes > 0) {
     // Record the shutdown info so that we can put it into a histogram at next
     // startup.
-    prefs->SetInteger(prefs::kShutdownType, g_shutdown_type);
+    prefs->SetInteger(prefs::kShutdownType, static_cast<int>(g_shutdown_type));
     prefs->SetInteger(prefs::kShutdownNumProcesses, g_shutdown_num_processes);
     prefs->SetInteger(prefs::kShutdownNumProcessesSlow,
                       g_shutdown_num_processes_slow);
@@ -217,7 +223,7 @@ void ShutdownPostThreadsStop(RestartMode restart_mode) {
 
 #if defined(OS_WIN)
   if (!browser_util::IsBrowserAlreadyRunning() &&
-      g_shutdown_type != END_SESSION) {
+      g_shutdown_type != ShutdownType::kEndSession) {
     upgrade_util::SwapNewChromeExeIfPresent();
   }
 #endif
@@ -263,7 +269,8 @@ void ShutdownPostThreadsStop(RestartMode restart_mode) {
 #endif  // defined(OS_CHROMEOS)
   }
 
-  if (g_shutdown_type > NOT_VALID && g_shutdown_num_processes > 0) {
+  if (g_shutdown_type != ShutdownType::kNotValid &&
+      g_shutdown_num_processes > 0) {
     // Measure total shutdown time as late in the process as possible
     // and then write it to a file to be read at startup.
     // We can't use prefs since all services are shutdown at this point.
@@ -298,29 +305,40 @@ void ReadLastShutdownFile(ShutdownType type,
     base::StringToInt64(shutdown_ms_str, &shutdown_ms);
   base::DeleteFile(shutdown_ms_file, false);
 
-  if (type == NOT_VALID || shutdown_ms == 0 || num_procs == 0)
+  if (shutdown_ms == 0 || num_procs == 0)
     return;
 
-  if (type == WINDOW_CLOSE) {
-    UMA_HISTOGRAM_MEDIUM_TIMES("Shutdown.window_close.time2",
-                               TimeDelta::FromMilliseconds(shutdown_ms));
-    UMA_HISTOGRAM_TIMES("Shutdown.window_close.time_per_process",
-                        TimeDelta::FromMilliseconds(shutdown_ms / num_procs));
-  } else if (type == BROWSER_EXIT) {
-    UMA_HISTOGRAM_MEDIUM_TIMES("Shutdown.browser_exit.time2",
-                               TimeDelta::FromMilliseconds(shutdown_ms));
-    UMA_HISTOGRAM_TIMES("Shutdown.browser_exit.time_per_process",
-                        TimeDelta::FromMilliseconds(shutdown_ms / num_procs));
-  } else if (type == END_SESSION) {
-    UMA_HISTOGRAM_MEDIUM_TIMES("Shutdown.end_session.time2",
-                               TimeDelta::FromMilliseconds(shutdown_ms));
-    UMA_HISTOGRAM_TIMES("Shutdown.end_session.time_per_process",
-                        TimeDelta::FromMilliseconds(shutdown_ms / num_procs));
-  } else {
-    NOTREACHED();
+  const char* time2_metric_name = nullptr;
+  const char* per_proc_metric_name = nullptr;
+
+  switch (type) {
+    case ShutdownType::kNotValid:
+      break;
+
+    case ShutdownType::kWindowClose:
+      time2_metric_name = "Shutdown.window_close.time2";
+      per_proc_metric_name = "Shutdown.window_close.time_per_process";
+      break;
+
+    case ShutdownType::kBrowserExit:
+      time2_metric_name = "Shutdown.browser_exit.time2";
+      per_proc_metric_name = "Shutdown.browser_exit.time_per_process";
+      break;
+
+    case ShutdownType::kEndSession:
+      time2_metric_name = "Shutdown.end_session.time2";
+      per_proc_metric_name = "Shutdown.end_session.time_per_process";
+      break;
   }
-  UMA_HISTOGRAM_COUNTS_100("Shutdown.renderers.total", num_procs);
-  UMA_HISTOGRAM_COUNTS_100("Shutdown.renderers.slow", num_procs_slow);
+  if (!time2_metric_name)
+    return;
+
+  base::UmaHistogramMediumTimes(time2_metric_name,
+                                TimeDelta::FromMilliseconds(shutdown_ms));
+  base::UmaHistogramTimes(per_proc_metric_name,
+                          TimeDelta::FromMilliseconds(shutdown_ms / num_procs));
+  base::UmaHistogramCounts100("Shutdown.renderers.total", num_procs);
+  base::UmaHistogramCounts100("Shutdown.renderers.slow", num_procs_slow);
 }
 
 void ReadLastShutdownInfo() {
@@ -330,11 +348,12 @@ void ReadLastShutdownInfo() {
   int num_procs = prefs->GetInteger(prefs::kShutdownNumProcesses);
   int num_procs_slow = prefs->GetInteger(prefs::kShutdownNumProcessesSlow);
   // clear the prefs immediately so we don't pick them up on a future run
-  prefs->SetInteger(prefs::kShutdownType, NOT_VALID);
+  prefs->SetInteger(prefs::kShutdownType,
+                    static_cast<int>(ShutdownType::kNotValid));
   prefs->SetInteger(prefs::kShutdownNumProcesses, 0);
   prefs->SetInteger(prefs::kShutdownNumProcessesSlow, 0);
 
-  UMA_HISTOGRAM_ENUMERATION("Shutdown.ShutdownType", type, kNumShutdownTypes);
+  base::UmaHistogramEnumeration("Shutdown.ShutdownType", type);
 
   base::PostTask(
       FROM_HERE,

chrome/browser/lifetime/browser_shutdown.h

@@ -41,25 +41,30 @@ enum class RestartMode {
 
 #endif  // !defined(OS_ANDROID)
 
-enum ShutdownType {
-  // an uninitialized value
-  NOT_VALID = 0,
-  // the last browser window was closed
-  WINDOW_CLOSE,
-  // user clicked on the Exit menu item
-  BROWSER_EXIT,
-  // windows is logging off or shutting down
-  END_SESSION
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class ShutdownType {
+  // An uninitialized value.
+  kNotValid = 0,
+  // The last browser window was closed.
+  kWindowClose = 1,
+  // The user clicked on the Exit menu item.
+  kBrowserExit = 2,
+  // User logoff or system shutdown.
+  kEndSession = 3,
+  kMaxValue = kEndSession
 };
 
-constexpr int kNumShutdownTypes = END_SESSION + 1;
-
 void RegisterPrefs(PrefRegistrySimple* registry);
 
 // Called when the browser starts shutting down so that we can measure shutdown
 // time.
 void OnShutdownStarting(ShutdownType type);
 
+// Returns true if OnShutdownStarting has been called to note that shutdown has
+// started.
+bool HasShutdownStarted();
+
 // Get the current shutdown type.
 ShutdownType GetShutdownType();
 

chrome/browser/lifetime/browser_shutdown_browsertest.cc

@@ -70,14 +70,15 @@ IN_PROC_BROWSER_TEST_F(BrowserShutdownBrowserTest,
   EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
   EXPECT_TRUE(BrowserList::GetInstance()->empty());
   EXPECT_EQ(browser_shutdown::GetShutdownType(),
-            browser_shutdown::WINDOW_CLOSE);
+            browser_shutdown::ShutdownType::kWindowClose);
   BrowserList::RemoveObserver(&closing_observer);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserShutdownBrowserTest,
                        TwoBrowsersClosingShutdownHistograms) {
-  histogram_tester_.ExpectUniqueSample("Shutdown.ShutdownType",
-                                       browser_shutdown::WINDOW_CLOSE, 1);
+  histogram_tester_.ExpectUniqueSample(
+      "Shutdown.ShutdownType",
+      static_cast<int>(browser_shutdown::ShutdownType::kWindowClose), 1);
   histogram_tester_.ExpectTotalCount("Shutdown.renderers.total", 1);
   histogram_tester_.ExpectTotalCount("Shutdown.window_close.time2", 1);
   histogram_tester_.ExpectTotalCount("Shutdown.window_close.time_per_process",

chrome/browser/metrics/chrome_metrics_service_accessor.h

@@ -26,8 +26,8 @@ class CrashesDOMHandler;
 class FlashDOMHandler;
 }
 
-namespace chrome {
-void AttemptRestart();
+namespace base {
+class CommandLine;
 }
 
 namespace domain_reliability {
@@ -69,6 +69,10 @@ namespace settings {
 class MetricsReportingHandler;
 }
 
+namespace upgrade_util {
+bool RelaunchChromeBrowserImpl(const base::CommandLine&);
+}
+
 // This class limits and documents access to metrics service helper methods.
 // Since these methods are private, each user has to be explicitly declared
 // as a 'friend' below.
@@ -84,7 +88,6 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
  private:
   friend class ::CrashesDOMHandler;
   friend class ::FlashDOMHandler;
-  friend void chrome::AttemptRestart();
   friend class ChromeBrowserFieldTrials;
   // For StackSamplingConfiguration.
   friend class ChromeBrowserMainParts;
@@ -112,6 +115,7 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
   friend class ChromePasswordManagerClient;
   friend void welcome::JoinOnboardingGroup(Profile* profile);
   friend class NavigationMetricsRecorder;
+  friend bool upgrade_util::RelaunchChromeBrowserImpl(const base::CommandLine&);
 
   // Testing related friends.
   friend class first_run::FirstRunMasterPrefsVariationsSeedTest;

chrome/browser/ui/browser.cc

@@ -803,8 +803,10 @@ void Browser::OnWindowClosing() {
       browser_shutdown::IsTryingToQuit() ||
       KeepAliveRegistry::GetInstance()->IsKeepingAliveOnlyByBrowserOrigin();
 
-  if (should_quit_if_last_browser && ShouldStartShutdown())
-    browser_shutdown::OnShutdownStarting(browser_shutdown::WINDOW_CLOSE);
+  if (should_quit_if_last_browser && ShouldStartShutdown()) {
+    browser_shutdown::OnShutdownStarting(
+        browser_shutdown::ShutdownType::kWindowClose);
+  }
 
   // Don't use GetForProfileIfExisting here, we want to force creation of the
   // session service so that user can restore what was open.

chrome/browser/ui/sad_tab_helper.cc

@@ -48,7 +48,7 @@ void SadTabHelper::RenderProcessGone(base::TerminationStatus status) {
   // Only show the sad tab if we're not in browser shutdown, so that WebContents
   // objects that are not in a browser (e.g., HTML dialogs) and thus are
   // visible do not flash a sad tab page.
-  if (browser_shutdown::GetShutdownType() != browser_shutdown::NOT_VALID)
+  if (browser_shutdown::HasShutdownStarted())
     return;
 
   if (sad_tab_)

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -1637,7 +1637,7 @@ bool TabStripModel::CloseWebContentses(
   // We only try the fast shutdown path if the whole browser process is *not*
   // shutting down. Fast shutdown during browser termination is handled in
   // browser_shutdown::OnShutdownStarting.
-  if (browser_shutdown::GetShutdownType() == browser_shutdown::NOT_VALID) {
+  if (!browser_shutdown::HasShutdownStarted()) {
     // Construct a map of processes to the number of associated tabs that are
     // closing.
     base::flat_map<content::RenderProcessHost*, size_t> processes;