Stability instrumentation: metrics for collection on crash

components/browser_watcher/stability_paths.cc

Index: components/browser_watcher/stability_paths.cc
diff --git a/components/browser_watcher/stability_paths.cc b/components/browser_watcher/stability_paths.cc
index 66805fa67d12381e81386de3e803adc59aa35f28..08d7e9650c7a349568febb58fd9a571381d67927 100644
--- a/components/browser_watcher/stability_paths.cc
+++ b/components/browser_watcher/stability_paths.cc
@@ -8,10 +8,15 @@
 #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/histogram_macros.h"
 #include "base/metrics/persistent_memory_allocator.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
@@ -22,6 +27,11 @@
 #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,22 @@ 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());

[Sigurður Ásgeirsson, 2017/05/31 17:19:05]

Does a MemoryMappedFile require that you allocate it from heap? If not, then why
not a local variable?

[bcwhite, 2017/05/31 18:19:26]

Owership of the object is passed via std::move to the
FilePersistentMemoryAllocator so has to be heap-based.

[manzagop (departed), 2017/05/31 19:57:23]

The allocator takes ownership of it at l.53. That said, the allocator doesn't
need to be on heap.

+  mmfile->Initialize(file_path, MemoryMappedFile::READ_WRITE);
+  if (!mmfile->IsValid())
+    return false;
+  if (!FilePersistentMemoryAllocator::IsFileAcceptable(*mmfile, true))
+    return false;
+  std::unique_ptr<FilePersistentMemoryAllocator> allocator(
+      new FilePersistentMemoryAllocator(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 +98,32 @@ 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();
+
+  // 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;
+    return;  // TODO(manzagop): add a metric for this.

[Sigurður Ásgeirsson, 2017/05/31 17:19:05]

is now an opportune moment for addressing this TODO?

[manzagop (departed), 2017/05/31 19:57:22]

Done.

   }
+  base::File deleter(stability_file, base::File::FLAG_OPEN |
+                                         base::File::FLAG_READ |
+                                         base::File::FLAG_DELETE_ON_CLOSE);
+}
 
-  // 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);
+bool MarkStabilityFileDeleted(const base::FilePath& file_path) {
+  bool success = SetPmaFileDeleted(file_path);
+  base::File deleter(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                                    base::File::FLAG_DELETE_ON_CLOSE);
+  return success;
 }
 
 }  // namespace browser_watcher