Stability instrumentation: metrics for collection on crash

This CL revises adds metrics for stability instrumentation
collection on crash. The metrics can be used to estimate unclean
shutdown coverage in combination with postmortem collection (see
https://crrev.com/2883103002).

Additionally addresses a todo: sets the persitent allocator state to
deleted on crash.

BUG=620813

Review-Url: https://codereview.chromium.org/2910003002
Cr-Commit-Position: refs/heads/master@{#476319}

chrome/browser/chrome_browser_field_trials_desktop.cc

@@ -24,8 +24,6 @@
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
-#include "components/browser_watcher/features.h"
-#include "components/browser_watcher/stability_paths.h"
 #include "components/metrics/persistent_system_profile.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/common/content_switches.h"
@@ -34,7 +32,10 @@
 #if defined(OS_WIN)
 #include "base/win/pe_image.h"
 #include "chrome/install_static/install_util.h"
+#include "components/browser_watcher/features.h"
 #include "components/browser_watcher/stability_data_names.h"
+#include "components/browser_watcher/stability_metrics.h"
+#include "components/browser_watcher/stability_paths.h"
 #endif
 
 #if defined(OS_WIN)
@@ -68,19 +69,6 @@ void SetupStunProbeTrial() {
 }
 
 #if defined(OS_WIN)
-// DO NOT CHANGE VALUES. This is logged persistently in a histogram.
-enum StabilityDebuggingInitializationStatus {
-  INIT_SUCCESS = 0,
-  CREATE_STABILITY_DIR_FAILED = 1,
-  GET_STABILITY_FILE_PATH_FAILED = 2,
-  INIT_STATUS_MAX = 3
-};
-
-void LogStabilityDebuggingInitStatus(
-    StabilityDebuggingInitializationStatus status) {
-  UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Record.InitStatus", status,
-                            INIT_STATUS_MAX);
-}
 
 // Record information about the chrome module.
 void RecordChromeModuleInfo(
@@ -130,18 +118,18 @@ void SetupStabilityDebugging() {
   base::FilePath user_data_dir;
   if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) ||
       !base::CreateDirectory(browser_watcher::GetStabilityDir(user_data_dir))) {
-    LOG(ERROR) << "Failed to create the stability directory.";
-    LogStabilityDebuggingInitStatus(CREATE_STABILITY_DIR_FAILED);
     return;
   }
+  browser_watcher::LogStabilityRecordEvent(
+      browser_watcher::StabilityRecordEvent::kStabilityDirectoryExists);
+
   base::FilePath stability_file;
   if (!browser_watcher::GetStabilityFileForProcess(
           base::Process::Current(), user_data_dir, &stability_file)) {
-    LOG(ERROR) << "Failed to obtain stability file's path.";
-    LogStabilityDebuggingInitStatus(GET_STABILITY_FILE_PATH_FAILED);
     return;
   }
-  LogStabilityDebuggingInitStatus(INIT_SUCCESS);
+  browser_watcher::LogStabilityRecordEvent(
+      browser_watcher::StabilityRecordEvent::kGotStabilityPath);
 
   // Track code activities (such as posting task, blocking on locks, and
   // joining threads) that can cause hanging threads and general instability
@@ -153,6 +141,8 @@ void SetupStabilityDebugging() {
   base::debug::GlobalActivityTracker* global_tracker =
       base::debug::GlobalActivityTracker::Get();
   if (global_tracker) {
+    browser_watcher::LogStabilityRecordEvent(
+        browser_watcher::StabilityRecordEvent::kGotTracker);
     // Record product, version, channel, special build and platform.
     wchar_t exe_file[MAX_PATH] = {};
     CHECK(::GetModuleFileName(nullptr, exe_file, arraysize(exe_file)));

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -13,7 +13,6 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
-#include "base/debug/activity_tracker.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
@@ -423,17 +422,9 @@ void ChromeMetricsServiceClient::OnEnvironmentUpdate(std::string* environment) {
 
 void ChromeMetricsServiceClient::OnLogCleanShutdown() {
 #if defined(OS_WIN)
-  base::debug::GlobalActivityTracker* global_tracker =
-      base::debug::GlobalActivityTracker::Get();
-  if (global_tracker)
-    global_tracker->MarkDeleted();
-
   base::FilePath user_data_dir;
-  if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) {
-    // TODO(manzagop): add a metric.
-    return;
-  }
-  browser_watcher::MarkStabilityFileForDeletion(user_data_dir);
+  if (base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
+    browser_watcher::MarkOwnStabilityFileDeleted(user_data_dir);
 #endif  // OS_WIN
 }
 

components/browser_watcher/BUILD.gn

@@ -107,6 +107,8 @@ if (is_win) {
       "stability_data_names.h",
       "stability_debugging.cc",
       "stability_debugging.h",
+      "stability_metrics.cc",
+      "stability_metrics.h",
       "stability_paths.cc",
       "stability_paths.h",
     ]

components/browser_watcher/postmortem_report_collector.cc

@@ -214,10 +214,9 @@ bool PostmortemReportCollector::CollectAndSubmitOneReport(
     return false;
   }
 
-  // Report collection is successful. We may increment |unclean_system_cnt|.
+  LogCollectionStatus(SUCCESS);
   if (report_proto.system_state().session_state() == SystemState::UNCLEAN)
     *system_unclean = true;
-
   return true;
 }
 

components/browser_watcher/stability_metrics.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 "components/browser_watcher/stability_metrics.h"
+
+#include "base/metrics/histogram_macros.h"
+
+namespace browser_watcher {
+
+void LogCollectOnCrashEvent(CollectOnCrashEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("ActivityTracker.CollectCrash.Event", event,
+                            CollectOnCrashEvent::kCollectOnCrashEventMax);
+}
+
+void LogStabilityRecordEvent(StabilityRecordEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("ActivityTracker.Record.Event", event,
+                            StabilityRecordEvent::kStabilityRecordEventMax);
+}
+
+}  // namespace browser_watcher

components/browser_watcher/stability_metrics.h

+// 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.
+
+#ifndef COMPONENTS_BROWSER_WATCHER_STABILITY_METRICS_H_
+#define COMPONENTS_BROWSER_WATCHER_STABILITY_METRICS_H_
+
+namespace browser_watcher {
+
+// DO NOT REMOVE OR REORDER VALUES. This is logged persistently in a histogram.
+enum class CollectOnCrashEvent {
+  kCollectAttempt,
+  kUserDataDirNotEmpty,
+  kPathExists,
+  kReportExtractionSuccess,
+  kPmaSetDeletedFailed,
+  kOpenForDeleteFailed,
+  kSuccess,
+  // New values go here.
+  kCollectOnCrashEventMax
+};
+
+// DO NOT REMOVE OR REORDER VALUES. This is logged persistently in a histogram.
+enum class StabilityRecordEvent {
+  kRecordAttempt,
+  kStabilityDirectoryExists,
+  kGotStabilityPath,
+  kGotTracker,
+  kMarkDeleted,
+  kMarkDeletedGotFile,
+  kOpenForDeleteFailed,
+  // New values go here.
+  kStabilityRecordEventMax
+};
+
+void LogCollectOnCrashEvent(CollectOnCrashEvent event);
+void LogStabilityRecordEvent(StabilityRecordEvent event);
+
+}  // namespace browser_watcher
+
+#endif  // COMPONENTS_BROWSER_WATCHER_STABILITY_METRICS_H_

components/browser_watcher/stability_paths.cc

@@ -8,20 +8,30 @@
 #include <windows.h>
 #endif  // defined(OS_WIN)
 
+#include <memory>
 #include <string>
+#include <utility>
 
+#include "base/debug/activity_tracker.h"
 #include "base/feature_list.h"
 #include "base/files/file.h"
+#include "base/files/memory_mapped_file.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "components/browser_watcher/features.h"
+#include "components/browser_watcher/stability_metrics.h"
 
 #if defined(OS_WIN)
 
 #include "third_party/crashpad/crashpad/util/win/time.h"
 
 namespace browser_watcher {
+
+using base::FilePersistentMemoryAllocator;
+using base::MemoryMappedFile;
+using base::PersistentMemoryAllocator;
+
 namespace {
 
 bool GetCreationTime(const base::Process& process, FILETIME* creation_time) {
@@ -30,6 +40,21 @@ bool GetCreationTime(const base::Process& process, FILETIME* creation_time) {
                            &ignore) != 0;
 }
 
+bool SetPmaFileDeleted(const base::FilePath& file_path) {
+  // Map the file read-write so it can guarantee consistency between
+  // the analyzer and any trackers that may still be active.
+  std::unique_ptr<MemoryMappedFile> mmfile(new MemoryMappedFile());
+  mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE);
+  if (!mmfile->IsValid())
+    return false;
+  if (!FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, true))
+    return false;
+  FilePersistentMemoryAllocator allocator(std::move(mmfile), 0, 0,
+                                          base::StringPiece(), true);
+  allocator.SetMemoryState(PersistentMemoryAllocator::MEMORY_DELETED);
+  return true;
+}
+
 }  // namespace
 
 base::FilePath GetStabilityDir(const base::FilePath& user_data_dir) {
@@ -72,25 +97,40 @@ base::FilePath::StringType GetStabilityFilePattern() {
          base::PersistentMemoryAllocator::kFileExtension;
 }
 
-void MarkStabilityFileForDeletion(const base::FilePath& user_data_dir) {
-  if (!base::FeatureList::IsEnabled(
-          browser_watcher::kStabilityDebuggingFeature)) {
-    return;
-  }
+void MarkOwnStabilityFileDeleted(const base::FilePath& user_data_dir) {
+  base::debug::GlobalActivityTracker* global_tracker =
+      base::debug::GlobalActivityTracker::Get();
+  if (!global_tracker)
+    return;  // No stability instrumentation.
+
+  global_tracker->MarkDeleted();
+  LogStabilityRecordEvent(StabilityRecordEvent::kMarkDeleted);
 
+  // Open (with delete) and then immediately close the file by going out of
+  // scope. This should cause the stability debugging file to be deleted prior
+  // to the next execution.
   base::FilePath stability_file;
   if (!GetStabilityFileForProcess(base::Process::Current(), user_data_dir,
                                   &stability_file)) {
-    // TODO(manzagop): add a metric for this.
     return;
   }
+  LogStabilityRecordEvent(StabilityRecordEvent::kMarkDeletedGotFile);
 
-  // Open (with delete) and then immediately close the file by going out of
-  // scope. This should cause the stability debugging file to be deleted prior
-  // to the next execution.
-  base::File file(stability_file, base::File::FLAG_OPEN |
-                                      base::File::FLAG_READ |
-                                      base::File::FLAG_DELETE_ON_CLOSE);
+  base::File deleter(stability_file, base::File::FLAG_OPEN |
+                                         base::File::FLAG_READ |
+                                         base::File::FLAG_DELETE_ON_CLOSE);
+  if (!deleter.IsValid())
+    LogStabilityRecordEvent(StabilityRecordEvent::kOpenForDeleteFailed);
+}
+
+void MarkStabilityFileDeletedOnCrash(const base::FilePath& file_path) {
+  if (!SetPmaFileDeleted(file_path))
+    LogCollectOnCrashEvent(CollectOnCrashEvent::kPmaSetDeletedFailed);
+
+  base::File deleter(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                                    base::File::FLAG_DELETE_ON_CLOSE);
+  if (!deleter.IsValid())
+    LogCollectOnCrashEvent(CollectOnCrashEvent::kOpenForDeleteFailed);
 }
 
 }  // namespace browser_watcher

components/browser_watcher/stability_paths.h

@@ -35,8 +35,15 @@ bool GetStabilityFileForProcess(const base::Process& process,
 // Returns a pattern that matches file names returned by GetFileForProcess.
 base::FilePath::StringType GetStabilityFilePattern();
 
-// Marks the stability file for deletion.
-void MarkStabilityFileForDeletion(const base::FilePath& user_data_dir);
+// Sets the current process's stability file's state to deleted (via the
+// GlobalActivityTracker) and opens the file for deletion. Metrics pertaining to
+// stability recording are logged.
+void MarkOwnStabilityFileDeleted(const base::FilePath& user_data_dir);
+
+// Sets another process's stability file's state to deleted, then opens it for
+// deletion. This function is meant for use by the crashpad handler; it logs
+// metrics labelled as in the context of crash collection.
+void MarkStabilityFileDeletedOnCrash(const base::FilePath& file_path);
 
 #endif  // defined(OS_WIN)
 

components/browser_watcher/stability_report_user_stream_data_source.cc

@@ -14,6 +14,7 @@
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "components/browser_watcher/minidump_user_streams.h"
+#include "components/browser_watcher/stability_metrics.h"
 #include "components/browser_watcher/stability_paths.h"
 #include "components/browser_watcher/stability_report_extractor.h"
 #include "third_party/crashpad/crashpad/minidump/minidump_user_extension_stream_data_source.h"
@@ -73,6 +74,8 @@ bool BufferExtensionStreamDataSource::ReadStreamData(Delegate* delegate) {
       data_.size() ? data_.data() : nullptr, data_.size());
 }
 
+// TODO(manzagop): Collection should factor in whether this is a true crash or
+// dump without crashing.
 std::unique_ptr<BufferExtensionStreamDataSource> CollectReport(
     const base::FilePath& path) {
   StabilityReport report;
@@ -81,20 +84,17 @@ std::unique_ptr<BufferExtensionStreamDataSource> CollectReport(
                             COLLECTION_STATUS_MAX);
   if (status != SUCCESS)
     return nullptr;
+  LogCollectOnCrashEvent(CollectOnCrashEvent::kReportExtractionSuccess);
 
-  // Open (with delete) and then immediately close the file by going out of
-  // scope. This should cause the stability debugging file to be deleted prior
-  // to the next execution.
-  // TODO(manzagop): set the persistent allocator file's state to deleted in
-  //     case the file can't be deleted.
-  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
-                            base::File::FLAG_DELETE_ON_CLOSE);
-  UMA_HISTOGRAM_BOOLEAN("ActivityTracker.CollectCrash.OpenForDeleteSuccess",
-                        file.IsValid());
+  MarkStabilityFileDeletedOnCrash(path);
 
   std::unique_ptr<BufferExtensionStreamDataSource> source(
       new BufferExtensionStreamDataSource(kStabilityReportStreamType));
-  return source->Init(report) ? std::move(source) : nullptr;
+  if (!source->Init(report))
+    return nullptr;
+
+  LogCollectOnCrashEvent(CollectOnCrashEvent::kSuccess);
+  return source;
 }
 
 }  // namespace
@@ -107,9 +107,11 @@ std::unique_ptr<crashpad::MinidumpUserExtensionStreamDataSource>
 StabilityReportUserStreamDataSource::ProduceStreamData(
     crashpad::ProcessSnapshot* process_snapshot) {
   DCHECK(process_snapshot);
+  LogCollectOnCrashEvent(CollectOnCrashEvent::kCollectAttempt);
 
   if (user_data_dir_.empty())
     return nullptr;
+  LogCollectOnCrashEvent(CollectOnCrashEvent::kUserDataDirNotEmpty);
 
   base::FilePath stability_file =
       GetStabilityFileName(user_data_dir_, process_snapshot);
@@ -118,6 +120,7 @@ StabilityReportUserStreamDataSource::ProduceStreamData(
     // processes can be instrumented), or the stability file cannot be found.
     return nullptr;
   }
+  LogCollectOnCrashEvent(CollectOnCrashEvent::kPathExists);
 
   return CollectReport(stability_file);
 }

tools/metrics/histograms/enums.xml

@@ -359,6 +359,16 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="3" label="Crashpad database init failed"/>
 </enum>
 
+<enum name="ActivityTrackerCollectOnCrashEvent" type="int">
+  <int value="0" label="Collection attempt"/>
+  <int value="1" label="User data directory not empty"/>
+  <int value="2" label="Path exists"/>
+  <int value="3" label="Report extraction success"/>
+  <int value="4" label="PMA set deleted failed"/>
+  <int value="5" label="Open for delete failed"/>
+  <int value="6" label="Success"/>
+</enum>
+
 <enum name="ActivityTrackerCollectStatus" type="int">
   <int value="0" label="None"/>
   <int value="1" label="Sucess"/>
@@ -370,7 +380,20 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="7" label="Finished writing crash report failed"/>
 </enum>
 
+<enum name="ActivityTrackerRecordEvent" type="int">
+  <int value="0" label="Recording attempt"/>
+  <int value="1" label="Stability directory exists"/>
+  <int value="2" label="Got stability file path"/>
+  <int value="3" label="Got tracker"/>
+  <int value="4" label="Mark deleted"/>
+  <int value="5" label="Mark deleted got file"/>
+  <int value="6" label="Open for delete failed"/>
+</enum>
+
 <enum name="ActivityTrackerRecordInitStatus" type="int">
+  <obsolete>
+    Removed from code May 2017.
+  </obsolete>
   <int value="0" label="Success"/>
   <int value="1" label="Create stability dir failed"/>
   <int value="2" label="Get stability file path failed"/>

tools/metrics/histograms/histograms.xml

@@ -449,8 +449,20 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="ActivityTracker.CollectCrash.Event"
+    enum="ActivityTrackerCollectOnCrashEvent">
+  <owner>manzagop@chromium.org</owner>
+  <summary>
+    Events that occur during crash collection of the debug file. Logged each
+    time an event of interest occurs during crash debug file collection.
+  </summary>
+</histogram>
+
 <histogram name="ActivityTracker.CollectCrash.OpenForDeleteSuccess"
     enum="BooleanSuccess">
+  <obsolete>
+    Deprecated 05/2017 in favor of ActivityTracker.CollectCrash.Event.
+  </obsolete>
   <owner>manzagop@chromium.org</owner>
   <summary>
     Success of the attempt to open the debug file for deletion. Logged each time
@@ -468,8 +480,20 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="ActivityTracker.Record.Event"
+    enum="ActivityTrackerRecordEvent">
+  <owner>manzagop@chromium.org</owner>
+  <summary>
+    Events pertaining to recording to the debug file. Logged each time an event
+    of interest occurs wrt debug file recording.
+  </summary>
+</histogram>
+
 <histogram name="ActivityTracker.Record.InitStatus"
     enum="ActivityTrackerRecordInitStatus">
+  <obsolete>
+    Deprecated 05/2017 in favor of ActivityTracker.Record.Event.
+  </obsolete>
   <owner>manzagop@chromium.org</owner>
   <summary>
     Status of internal activity tracking initialization. Logged once, during the