Wire in postmortem report collection

chrome/browser/chrome_browser_field_trials_desktop.cc

 // Copyright 2013 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 "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)
   std::map<std::string, std::string> params;
   if (!variations::GetVariationParams("StunProbeTrial2", &params))
     return;
 
   // The parameter, used by StartStunFieldTrial, should have the following
   // format: "request_per_ip/interval/sharedsocket/batch_size/total_batches/
   // server1:port/server2:port/server3:port/"
   std::string cmd_param = params["request_per_ip"] + "/" + params["interval"] +
                           "/" + params["sharedsocket"] + "/" +
                           params["batch_size"] + "/" + params["total_batches"] +
                           "/" + params["server1"] + "/" + params["server2"] +
                           "/" + params["server3"] + "/" + params["server4"] +
                           "/" + params["server5"] + "/" + params["server6"];
 
   base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       switches::kWebRtcStunProbeTrialParameter, cmd_param);
 #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
   // just how much of an arena is necessary.
   const size_t kMemorySize = 1 << 20;  // 1 MiB
   const int kStackDepth = 4;
   const uint64_t kAllocatorId = 0;
 
+  // Ensure the stability directory exists and determine the stability file's
+  // path.
+  base::FilePath user_data_dir;
+  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::FilePath user_data_dir;
-  bool success = base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  DCHECK(success);
+  // 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