Wire in postmortem report collection

chrome/browser/chrome_browser_field_trials_desktop.cc

Index: chrome/browser/chrome_browser_field_trials_desktop.cc
diff --git a/chrome/browser/chrome_browser_field_trials_desktop.cc b/chrome/browser/chrome_browser_field_trials_desktop.cc
index b955ed8ac5150a4e67dea4c5f76f37e1490db510..b2fd0285da5806da96f9d392ac55b94a7150e589 100644
--- a/chrome/browser/chrome_browser_field_trials_desktop.cc
+++ b/chrome/browser/chrome_browser_field_trials_desktop.cc
@@ -4,27 +4,33 @@
 
 #include "chrome/browser/chrome_browser_field_trials_desktop.h"
 
+#include <map>
 #include <string>
 
 #include "base/command_line.h"
 #include "base/debug/activity_tracker.h"
 #include "base/feature_list.h"
+#include "base/files/file_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "chrome/browser/prerender/prerender_field_trial.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/variations/variations_util.h"
+#include "components/browser_watcher/features.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/common/content_switches.h"
 
+#if defined(OS_WIN)
+#include "components/browser_watcher/stability_debugging_win.h"
+#endif
+
 namespace chrome {
 
 namespace {
 
-const base::Feature kStabilityDebuggingFeature{
-    "StabilityDebugging", base::FEATURE_DISABLED_BY_DEFAULT
-};
+using browser_watcher::StabilityDebugging;
 
 void SetupStunProbeTrial() {
 #if defined(ENABLE_WEBRTC)
@@ -47,8 +53,26 @@ void SetupStunProbeTrial() {
 #endif
 }
 
+#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 LogStabilityDebuggingInitStatusWithoutStaticVar(
+    StabilityDebuggingInitializationStatus status) {
+  base::HistogramBase* init_status_hist = base::LinearHistogram::FactoryGet(

[Sigurður Ásgeirsson, 2016/09/22 15:46:14]

Ah - I see.
Since this'll at least instantiate only one instance of the code and static
variable, maybe a meet in the middle is to use the UMA macro once here?
This is fine too - whichever way your prefer.

[Alexei Svitkine (slow), 2016/09/23 14:13:53]

Yeah, I'd suggest using the macro. Not using the macro will save you a static
var that caches the pointer, but actually that part is not really much savings -
since the Histogram object itself is still cached.

And the macro is more readable. (Once we have the non-caching version of the
macro, it will also be easier to switch to that.)

Also, when you switch back to the macros you can rename the function to omit
"WithoutStaticVar" prefix which should make call sites fit on one line. :)

[manzagop (departed), 2016/09/23 21:57:01]

Done.

[manzagop (departed), 2016/09/23 21:57:01]

Done.

[Sigurður Ásgeirsson, 2016/09/26 15:22:25]

Each of those macro incantations instantiates a fair bit of code, performs at
least two atomic memory accesses, and adds at least one static variable to the
data section. It might be fun to try to fold these incantations into a templated
function or some such, see whether the compiler can scrunch them down. 
I count ~4800 UMA_HISTOGRAM* instances in Chrome, so it might be worth looking
at?

[Alexei Svitkine (slow), 2016/09/26 15:29:49]

Agreed.

We've previously done some exploration and improvements on this front here:

https://bugs.chromium.org/p/chromium/issues/detail?id=343946
And we still have a general top-level bug open here:
https://bugs.chromium.org/p/chromium/issues/detail?id=500942

If you have concrete ideas for improvements, feel free to post on later bug
(since it's still open) or file a more specific bug and make it blocked on that.
I definitely support anything we could do to make histograms have less overhead
than they do now. :)

+      "ActivityTracker.Record.InitStatus", 1, INIT_STATUS_MAX,
+      INIT_STATUS_MAX + 1, base::HistogramBase::kUmaTargetedHistogramFlag);
+  init_status_hist->Add(status);
+}
+
 void SetupStabilityDebugging() {
-  if (!base::FeatureList::IsEnabled(kStabilityDebuggingFeature))
+  if (!base::FeatureList::IsEnabled(
+          browser_watcher::kStabilityDebuggingFeature))

[Alexei Svitkine (slow), 2016/09/23 14:13:53]

Nit: {} now that if is multi-line

[manzagop (departed), 2016/09/23 21:57:01]

Done.

     return;
 
   // TODO(bcwhite): Adjust these numbers once there is real data to show
@@ -57,24 +81,42 @@ void SetupStabilityDebugging() {
   const int kStackDepth = 4;
   const uint64_t kAllocatorId = 0;
 
-  // Track code activities (such as posting task, blocking on locks, and
-  // joining threads) that can cause hanging threads and general instability.
+  // Ensure the stability directory exists and determine the stability file's
+  // path.
   base::FilePath user_data_dir;
-  bool success = base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  DCHECK(success);
+  if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) ||
+      !base::CreateDirectory(StabilityDebugging::GetDir(user_data_dir))) {
+    LOG(ERROR) << "Failed to create the stability directory.";
+    LogStabilityDebuggingInitStatusWithoutStaticVar(
+        CREATE_STABILITY_DIR_FAILED);
+    return;
+  }
+  base::FilePath stability_file;
+  if (!StabilityDebugging::GetFileForProcess(base::Process::Current(),
+                                             user_data_dir, &stability_file)) {
+    LOG(ERROR) << "Failed to obtain stability file's path.";
+    LogStabilityDebuggingInitStatusWithoutStaticVar(
+        GET_STABILITY_FILE_PATH_FAILED);
+    return;
+  }
+  LogStabilityDebuggingInitStatusWithoutStaticVar(INIT_SUCCESS);
+
+  // Track code activities (such as posting task, blocking on locks, and
+  // joining threads) that can cause hanging threads and general instability
   base::debug::GlobalActivityTracker::CreateWithFile(
-      user_data_dir
-          .AppendASCII("StabilityDebugInfo")
-          .AddExtension(base::PersistentMemoryAllocator::kFileExtension),
-      kMemorySize, kAllocatorId, kStabilityDebuggingFeature.name, kStackDepth);
+      stability_file, kMemorySize, kAllocatorId,
+      browser_watcher::kStabilityDebuggingFeature.name, kStackDepth);
 }
+#endif

[Alexei Svitkine (slow), 2016/09/23 14:13:53]

// defined(OS_WIN)

[manzagop (departed), 2016/09/23 21:57:01]

Done.

 
 }  // namespace
 
 void SetupDesktopFieldTrials(const base::CommandLine& parsed_command_line) {
   prerender::ConfigurePrerender(parsed_command_line);
   SetupStunProbeTrial();
+#if defined(OS_WIN)
   SetupStabilityDebugging();
+#endif
 }
 
 }  // namespace chrome