Bound the impact of system instability on chrome instability.

components/browser_watcher/postmortem_report_collector.cc

 // Copyright 2016 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/postmortem_report_collector.h"
 
 #include <utility>
 
 #include "base/debug/activity_analyzer.h"
 #include "base/files/file_enumerator.h"
@@ skipping 18 matching lines @@
 using base::debug::ActivityUserData;
 using base::debug::GlobalActivityAnalyzer;
 using base::debug::GlobalActivityTracker;
 using base::debug::ThreadActivityAnalyzer;
 using crashpad::CrashReportDatabase;
 
 namespace {
 
 const char kFieldTrialKeyPrefix[] = "FieldTrial.";
 
+// DO NOT CHANGE VALUES. This is logged persistently in a histogram.
+enum SystemSessionAnalysisStatus {
+  SYSTEM_SESSION_ANALYSIS_SUCCESS = 0,
+  SYSTEM_SESSION_ANALYSIS_NO_TIMESTAMP = 1,
+  SYSTEM_SESSION_ANALYSIS_NO_ANALYZER = 2,
+  SYSTEM_SESSION_ANALYSIS_FAILED = 3,
+  SYSTEM_SESSION_ANALYSIS_OUTSIDE_RANGE = 4,
+  SYSTEM_SESSION_ANALYSIS_STATUS_MAX = 5
+};
+
 // Collects stability user data from the recorded format to the collected
 // format.
 void CollectUserData(
     const ActivityUserData::Snapshot& recorded_map,
     google::protobuf::Map<std::string, TypedValue>* collected_map,
     StabilityReport* report) {
   DCHECK(collected_map);
 
   for (const auto& name_and_value : recorded_map) {
     const std::string& key = name_and_value.first;
@@ skipping 59 matching lines @@
         break;
       case ActivityUserData::UNSIGNED_VALUE:
         collected_value.set_unsigned_value(recorded_value.GetUint());
         break;
     }
 
     (*collected_map)[key].Swap(&collected_value);
   }
 }
 
+bool GetStartTimestamp(
+    const google::protobuf::Map<std::string, TypedValue>& global_data,
+    base::Time* time) {
+  DCHECK(time);
+
+  const auto& it = global_data.find(kStabilityStartTimestamp);
+  if (it == global_data.end())
+    return false;
+
+  const TypedValue& value = it->second;
+  if (value.value_case() != TypedValue::kSignedValue)
+    return false;
+
+  *time = base::Time::FromInternalValue(value.signed_value());
+  return true;
+}
+
 void CollectModuleInformation(
     const std::vector<GlobalActivityTracker::ModuleInfo>& modules,
     ProcessState* process_state) {
   DCHECK(process_state);
 
   char code_identifier[17];
   char debug_identifier[41];
 
   for (const GlobalActivityTracker::ModuleInfo& recorded : modules) {
     CodeModule* collected = process_state->add_modules();
@@ skipping 18 matching lines @@
     collected->set_debug_identifier(debug_identifier);
     collected->set_is_unloaded(!recorded.is_loaded);
   }
 }
 
 }  // namespace
 
 PostmortemReportCollector::PostmortemReportCollector(
     const std::string& product_name,
     const std::string& version_number,
-    const std::string& channel_name)
+    const std::string& channel_name,
+    SystemSessionAnalyzer* analyzer)
     : product_name_(product_name),
       version_number_(version_number),
-      channel_name_(channel_name) {}
+      channel_name_(channel_name),
+      system_session_analyzer_(analyzer) {}
+
+PostmortemReportCollector::~PostmortemReportCollector() {}
 
 int PostmortemReportCollector::CollectAndSubmitForUpload(
     const base::FilePath& debug_info_dir,
     const base::FilePath::StringType& debug_file_pattern,
     const std::set<base::FilePath>& excluded_debug_files,
     crashpad::CrashReportDatabase* report_database) {
   DCHECK_NE(true, debug_info_dir.empty());
   DCHECK_NE(true, debug_file_pattern.empty());
   DCHECK_NE(nullptr, report_database);
 
@@ skipping 133 matching lines @@
 
   // Create the report, then flesh it out.
   report->reset(new StabilityReport());
 
   // Collect log messages.
   for (const std::string& message : log_messages) {
     (*report)->add_log_messages(message);
   }
 
   // Collect global user data.
-  google::protobuf::Map<std::string, TypedValue>& global_data =
-      *(*report)->mutable_global_data();
-  CollectUserData(global_data_snapshot, &global_data, report->get());
-
-  // Add the reporting Chrome's details to the report.
-  global_data[kStabilityReporterChannel].set_string_value(channel_name());
-#if defined(ARCH_CPU_X86)
-  global_data[kStabilityReporterPlatform].set_string_value(
-      std::string("Win32"));
-#elif defined(ARCH_CPU_X86_64)
-  global_data[kStabilityReporterPlatform].set_string_value(
-      std::string("Win64"));
-#endif
-  global_data[kStabilityReporterProduct].set_string_value(product_name());
-  global_data[kStabilityReporterVersion].set_string_value(version_number());
+  CollectUserData(global_data_snapshot, (*report)->mutable_global_data(),
+                  report->get());
+  SetReporterDetails(report->get());
+  DetermineSystemState(report->get());
 
   // Collect thread activity data.
   // Note: a single process is instrumented.

[Sigurður Ásgeirsson, 2017/03/02 21:52:58]

this comment is a little opaque - what does "instrumented" mean here?

[manzagop (departed), 2017/03/06 15:37:40]

Hopefully clearer. Done.

   ProcessState* process_state = (*report)->add_process_states();
   for (; thread_analyzer != nullptr;
        thread_analyzer = global_analyzer->GetNextAnalyzer()) {
     // Only valid analyzers are expected per contract of GetFirstAnalyzer /
     // GetNextAnalyzer.
     DCHECK(thread_analyzer->IsValid());
 
     if (!process_state->has_process_id()) {
       process_state->set_process_id(
           thread_analyzer->activity_snapshot().process_id);
     }
     DCHECK_EQ(thread_analyzer->activity_snapshot().process_id,
               process_state->process_id());
 
     ThreadState* thread_state = process_state->add_threads();
     CollectThread(thread_analyzer->activity_snapshot(), thread_state);
   }
 
   // Collect module information.
   CollectModuleInformation(global_analyzer->GetModules(), process_state);
 
   return SUCCESS;
 }
 
+void PostmortemReportCollector::SetReporterDetails(
+    StabilityReport* report) const {
+  DCHECK(report);
+
+  google::protobuf::Map<std::string, TypedValue>& global_data =
+      *(report->mutable_global_data());
+
+  // Reporter version details.
+  global_data[kStabilityReporterChannel].set_string_value(channel_name());
+#if defined(ARCH_CPU_X86)
+  global_data[kStabilityReporterPlatform].set_string_value(
+      std::string("Win32"));
+#elif defined(ARCH_CPU_X86_64)
+  global_data[kStabilityReporterPlatform].set_string_value(
+      std::string("Win64"));
+#endif
+  global_data[kStabilityReporterProduct].set_string_value(product_name());
+  global_data[kStabilityReporterVersion].set_string_value(version_number());
+}
+
+void PostmortemReportCollector::DetermineSystemState(

[Sigurður Ásgeirsson, 2017/03/02 21:52:58]

Record{Previous|System}Shutdown{State|Reason}?

[manzagop (departed), 2017/03/06 15:37:40]

Done.

+    StabilityReport* report) const {
+  DCHECK(report);
+
+  // A session state for the stability report: was the system session clean?
+  SystemState::SessionState session_state = SystemState::UNKNOWN;
+  // An analasis status for metrics: did the analysis succeed?

[Sigurður Ásgeirsson, 2017/03/02 21:52:57]

analasis->analysis?

[manzagop (departed), 2017/03/06 15:37:40]

Done.

+  SystemSessionAnalysisStatus status = SYSTEM_SESSION_ANALYSIS_SUCCESS;
+
+  base::Time time;
+  if (!GetStartTimestamp(report->global_data(), &time)) {
+    status = SYSTEM_SESSION_ANALYSIS_NO_TIMESTAMP;
+  } else if (!system_session_analyzer_) {
+    status = SYSTEM_SESSION_ANALYSIS_NO_ANALYZER;
+  } else {
+    SystemSessionAnalyzer::Status analyzer_status =
+        system_session_analyzer_->IsSessionUnclean(time);
+    switch (analyzer_status) {
+      case SystemSessionAnalyzer::FAILED:
+        status = SYSTEM_SESSION_ANALYSIS_FAILED;
+        break;
+      case SystemSessionAnalyzer::CLEAN:
+        session_state = SystemState::CLEAN;
+        break;
+      case SystemSessionAnalyzer::UNCLEAN:
+        session_state = SystemState::UNCLEAN;
+        break;
+      case SystemSessionAnalyzer::OUTSIDE_RANGE:
+        status = SYSTEM_SESSION_ANALYSIS_OUTSIDE_RANGE;
+        break;
+    }
+  }
+
+  report->mutable_system_state()->set_session_state(session_state);
+  UMA_HISTOGRAM_ENUMERATION(

[Sigurður Ásgeirsson, 2017/03/02 21:52:58]

isn't this the terrible caching version of the histograms?

[manzagop (departed), 2017/03/06 15:37:40]

FWIU this is the only version that has a shorthand (macro). 

Last time I had multiple uses of the macro for the same histogram. We ended up
creating a function so the macro would be only used once per metric.

Tell me what you think. If you feel strongly about it we should loop in metrics
team, as I think most people use this without second thought.

[Sigurður Ásgeirsson, 2017/03/06 19:00:09]

It irks me that this generates a whole lot of code for optimize for efficiency
on re-use. When a histogram is recorded only once in a process' lifetime, that's
the wrong optimization to make.

+      "ActivityTracker.Collect.SystemSessionAnalysisStatus", status,
+      SYSTEM_SESSION_ANALYSIS_STATUS_MAX);
+}
+
 void PostmortemReportCollector::CollectThread(
     const base::debug::ThreadActivityAnalyzer::Snapshot& snapshot,
     ThreadState* thread_state) {
   DCHECK(thread_state);
 
   thread_state->set_thread_name(snapshot.thread_name);
   thread_state->set_thread_id(snapshot.thread_id);
   thread_state->set_activity_count(snapshot.activity_stack_depth);
 
   for (size_t i = 0; i < snapshot.activity_stack.size(); ++i) {
@@ skipping 39 matching lines @@
     StabilityReport* report,
     const crashpad::UUID& client_id,
     const crashpad::UUID& report_id,
     base::PlatformFile minidump_file) {
   DCHECK(report);
 
   return WritePostmortemDump(minidump_file, client_id, report_id, report);
 }
 
 }  // namespace browser_watcher