Collect log messages from stability files

components/browser_watcher/postmortem_report_collector.cc

Index: components/browser_watcher/postmortem_report_collector.cc
diff --git a/components/browser_watcher/postmortem_report_collector.cc b/components/browser_watcher/postmortem_report_collector.cc
index 24cb0b67c018d61b56fecc4ddeb07ba09e342ebf..e3030ba5d810f61e2c6612de9fb62340f699b5dc 100644
--- a/components/browser_watcher/postmortem_report_collector.cc
+++ b/components/browser_watcher/postmortem_report_collector.cc
@@ -167,17 +167,24 @@ PostmortemReportCollector::CollectionStatus PostmortemReportCollector::Collect(
     return ANALYZER_CREATION_FAILED;
 
   // Early exit if there is no data.
+  std::vector<std::string> log_messages = global_analyzer->GetLogMessages();
   ThreadActivityAnalyzer* thread_analyzer = global_analyzer->GetFirstAnalyzer();
-  if (!thread_analyzer) {
+  if (log_messages.empty() && !thread_analyzer) {

[bcwhite, 2016/12/09 00:34:22]

It's possible to have log messages even on a clean exit.  Right now it's limited
to FATAL messages but that's not part of the definition.

[manzagop (departed), 2016/12/09 22:41:09]

Right: if we stop deleting the file on clean exit, then we'll need to add some
logic to know if we should send a report (ie if the exit was clean or not).

Do you feel a change is needed?

[bcwhite, 2016/12/09 23:17:06]

log_messages being empty or non-empty doesn't consistently indicate crash-exit
or clean-exit.

I don't think you should check it at all when deciding whether it was a clean
exit.

[manzagop (departed), 2016/12/12 15:59:32]

I've removed the mention of clean exit: this should only be about whether the
file is empty. 

Our current decision of clean vs not is whether the file exists, since we open
it for deletion on clean exit. You're right that we *also* don't send a report
if it's fully empty.

I've also updated the comment where this function is called to state the current
behavior: existing file that is non-empty = send a report.

[bcwhite, 2016/12/13 13:22:55]

So to be clear...  You _do_ want to send a report if _any_ log messages were
collected, keeping in mind that these messages may not be "fatal" messages.

Though this is currently the case but may not be so in the future which would
mean, should it change, that a report would get sent every time.

[manzagop (departed), 2016/12/13 14:01:39]

That's right: short term we rely on MarkStabilityFileForDeletion to know if the
exit was unclean:
https://cs.chromium.org/chromium/src/components/browser_watcher/stability_deb...

Longer term we'll get rid of MarkStabilityFileForDeletion and cook up some kind
of analysis for which presence of logs won't be an indicator of unclean
shutdown.

As we found out, having thread activity data also isn't an indicator of unclean
shutdown.

     // No data. This case happens in the case of a clean exit.
     return DEBUG_FILE_NO_DATA;
   }
 
-  // Iterate through the thread analyzers, fleshing out the report.
+  // 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 thread activity data.
   // Note: a single process is instrumented.
   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 /