Clean up redundant ShutdownCleanliness-related code.

Also, move the CHECK into CleanExitBeacon.

There's no reason we can't use kStabilityExitedCleanly instead of
UmaMetricsProperlyShutdown() in ChromeBrowserMainParts
::PostDestroyThreads().

The CHECK does, however, need to be moved to before the destruction of
the browser process to access the kStabilityExitedCleanly pref.

We update clean_shutdown_status_ in two places:
* MetricsService::LogCleanShutdown(), in which kStabilityExitedCleanly
  is set to true and clean_shutdown_status_ to CLEANLY_SHUTDOWN
* MetricsService::LogNeedForCleanShutdown(), in which
  kStabilityExitedCleanly is set to false and clean_shutdown_status_ to
  NEED_TO_SHUTDOWN

We update kStabilityExitedCleanly in the two functions mentioned above
and in two mobile-only functions.

ShutdownCleanliness was not and is not used on iOS or Android, so it
does not matter that OnAppEnterForeground() and
MarkAppCleanShutdownAndCommit() update kStabilityExitedCleanly. The
CHECK is not performed on iOS or Android.

Bug: 1205645
Change-Id: Ica160674aabdeab20edc049eee8beeec7e8e1a7f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2872668
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880577}

chrome/browser/chrome_browser_main.cc

@@ -124,6 +124,7 @@
 #include "components/language/core/common/language_experiments.h"
 #include "components/metrics/call_stack_profile_metrics_provider.h"
 #include "components/metrics/call_stack_profile_params.h"
+#include "components/metrics/clean_exit_beacon.h"
 #include "components/metrics/expired_histogram_util.h"
 #include "components/metrics/metrics_reporting_default_state.h"
 #include "components/metrics/metrics_service.h"
@@ -1796,6 +1797,7 @@ void ChromeBrowserMainParts::PostDestroyThreads() {
   // not finish.
   NOTREACHED();
 #else
+
   browser_shutdown::RestartMode restart_mode =
       browser_shutdown::RestartMode::kNoRestart;
 
@@ -1809,8 +1811,21 @@ void ChromeBrowserMainParts::PostDestroyThreads() {
   }
 
   browser_process_->PostDestroyThreads();
-  // browser_shutdown takes care of deleting browser_process, so we need to
-  // release it.
+
+  // We need to do this check as late as possible, but due to modularity, this
+  // may be the last point in Chrome. This would be more effective if done at a
+  // higher level on the stack, so that it is impossible for an early return to
+  // bypass this code. Perhaps we need a *final* hook that is called on all
+  // paths from content/browser/browser_main.
+  //
+  // Since we use |browser_process_|'s local state for this CHECK, it must be
+  // done before |browser_process_| is released.
+  metrics::CleanExitBeacon::EnsureCleanShutdown(
+      browser_process_->local_state());
+
+  // The below call to browser_shutdown::ShutdownPostThreadsStop() deletes
+  // |browser_process_|. We release it so that we don't keep holding onto an
+  // invalid reference.
   ignore_result(browser_process_.release());
 
 #if BUILDFLAG(ENABLE_DOWNGRADE_PROCESSING)
@@ -1834,13 +1849,6 @@ void ChromeBrowserMainParts::PostDestroyThreads() {
   process_singleton_.reset();
   device_event_log::Shutdown();
 
-  // We need to do this check as late as possible, but due to modularity, this
-  // may be the last point in Chrome.  This would be more effective if done at
-  // a higher level on the stack, so that it is impossible for an early return
-  // to bypass this code.  Perhaps we need a *final* hook that is called on all
-  // paths from content/browser/browser_main.
-  CHECK(metrics::MetricsService::UmaMetricsProperlyShutdown());
-
 #if BUILDFLAG(IS_CHROMEOS_ASH)
   arc::StabilityMetricsManager::Shutdown();
   ash::StatsReportingController::Shutdown();

components/metrics/clean_exit_beacon.cc

@@ -66,14 +66,6 @@ CleanExitBeacon::CleanExitBeacon(const std::wstring& backup_registry_key,
 CleanExitBeacon::~CleanExitBeacon() {
 }
 
-// static
-void CleanExitBeacon::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterBooleanPref(prefs::kStabilityExitedCleanly, true);
-
-  registry->RegisterTimePref(prefs::kStabilityBrowserLastLiveTimeStamp,
-                             base::Time(), PrefRegistry::LOSSY_PREF);
-}
-
 void CleanExitBeacon::WriteBeaconValue(bool value) {
   UpdateLastLiveTimestamp();
   local_state_->SetBoolean(prefs::kStabilityExitedCleanly, value);
@@ -93,4 +85,17 @@ void CleanExitBeacon::UpdateLastLiveTimestamp() {
                         base::Time::Now());
 }
 
+// static
+void CleanExitBeacon::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(prefs::kStabilityExitedCleanly, true);
+
+  registry->RegisterTimePref(prefs::kStabilityBrowserLastLiveTimeStamp,
+                             base::Time(), PrefRegistry::LOSSY_PREF);
+}
+
+// static
+void CleanExitBeacon::EnsureCleanShutdown(PrefService* local_state) {
+  CHECK(local_state->GetBoolean(prefs::kStabilityExitedCleanly));
+}
+
 }  // namespace metrics

components/metrics/clean_exit_beacon.h

@@ -45,6 +45,9 @@ class CleanExitBeacon {
   // Registers local state prefs used by this class.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
+  // CHECKs that Chrome exited cleanly.
+  static void EnsureCleanShutdown(PrefService* local_state);
+
  private:
   PrefService* const local_state_;
   const bool initial_value_;

components/metrics/metrics_service.cc

@@ -186,10 +186,6 @@ void MarkAppCleanShutdownAndCommit(CleanExitBeacon* clean_exit_beacon,
 
 }  // namespace
 
-// static
-MetricsService::ShutdownCleanliness MetricsService::clean_shutdown_status_ =
-    MetricsService::CLEANLY_SHUTDOWN;
-
 // static
 void MetricsService::RegisterPrefs(PrefRegistrySimple* registry) {
   CleanExitBeacon::RegisterPrefs(registry);
@@ -437,8 +433,6 @@ void MetricsService::OnAppEnterForeground(bool force_open_new_log) {
 #else
 void MetricsService::LogNeedForCleanShutdown() {
   state_manager_->clean_exit_beacon()->WriteBeaconValue(false);
-  // Redundant setting to be sure we call for a clean shutdown.
-  clean_shutdown_status_ = NEED_TO_SHUTDOWN;
 }
 #endif  // defined(OS_ANDROID) || defined(OS_IOS)
 
@@ -762,12 +756,6 @@ bool MetricsService::PrepareInitialStabilityLog(
   return true;
 }
 
-bool MetricsService::UmaMetricsProperlyShutdown() {
-  CHECK(clean_shutdown_status_ == CLEANLY_SHUTDOWN ||
-        clean_shutdown_status_ == NEED_TO_SHUTDOWN);
-  return clean_shutdown_status_ == CLEANLY_SHUTDOWN;
-}
-
 void MetricsService::RegisterMetricsProvider(
     std::unique_ptr<MetricsProvider> provider) {
   DCHECK_EQ(CONSTRUCTED, state_);
@@ -911,9 +899,6 @@ void MetricsService::PrepareProviderMetricsTask() {
 
 void MetricsService::LogCleanShutdown(bool end_completed) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Redundant setting to assure that we always reset this value at shutdown
-  // (and that we don't use some alternate path, and not call LogCleanShutdown).
-  clean_shutdown_status_ = CLEANLY_SHUTDOWN;
   state_manager_->clean_exit_beacon()->WriteBeaconValue(true);
   StabilityMetricsProvider(local_state_).MarkSessionEndCompleted(end_completed);
 }

components/metrics/metrics_service.h

@@ -144,10 +144,6 @@ class MetricsService : public base::HistogramFlattener {
   bool reporting_active() const;
   bool has_unsent_logs() const;
 
-  // Redundant test to ensure that we are notified of a clean exit.
-  // This value should be true when process has completed shutdown.
-  static bool UmaMetricsProperlyShutdown();
-
   // Register the specified |provider| to provide additional metrics into the
   // UMA log. Should be called during MetricsService initialization only.
   void RegisterMetricsProvider(std::unique_ptr<MetricsProvider> provider);
@@ -208,11 +204,6 @@ class MetricsService : public base::HistogramFlattener {
   State state() const { return state_; }
 
  private:
-  enum ShutdownCleanliness {
-    CLEANLY_SHUTDOWN = 0xdeadbeef,
-    NEED_TO_SHUTDOWN = ~CLEANLY_SHUTDOWN
-  };
-
   // The current state of recording for the MetricsService. The state is UNSET
   // until set to something else, at which point it remains INACTIVE or ACTIVE
   // for the lifetime of the object.
@@ -389,10 +380,6 @@ class MetricsService : public base::HistogramFlattener {
   bool is_in_foreground_ = false;
 #endif
 
-  // Redundant marker to check that we completed our shutdown, and set the
-  // exited-cleanly bit in the prefs.
-  static ShutdownCleanliness clean_shutdown_status_;
-
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ActiveFieldTrialsReported);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, IsPluginProcess);
   FRIEND_TEST_ALL_PREFIXES(::ChromeMetricsServiceClientTest,