Send compressed histograms with system logs when sending feedback.

chrome/browser/feedback/feedback_data.cc

Index: chrome/browser/feedback/feedback_data.cc
diff --git a/chrome/browser/feedback/feedback_data.cc b/chrome/browser/feedback/feedback_data.cc
index 95cb49fe9f9108ad113d1c7adf196273ee5e3cbf..c4dafbf0eb9539aeb3883918536c4f0d005b6632 100644
--- a/chrome/browser/feedback/feedback_data.cc
+++ b/chrome/browser/feedback/feedback_data.cc
@@ -39,6 +39,8 @@ const char kZipExt[] = ".zip";
 
 const base::FilePath::CharType kLogsFilename[] =
     FILE_PATH_LITERAL("system_logs.txt");
+const base::FilePath::CharType kHistogramsFilename[] =
+    FILE_PATH_LITERAL("histograms.txt");
 
 // Converts the system logs into a string that we can compress and send
 // with the report. This method only converts those logs that we want in
@@ -87,6 +89,18 @@ void ZipLogs(FeedbackData::SystemLogsMap* sys_info,
   }
 }
 
+void ZipHistograms(const std::string* histograms,
+                   std::string* compressed_histograms) {
+  DCHECK(compressed_histograms);
+  if (histograms->empty() ||
+      !feedback_util::ZipString(
+          base::FilePath(kHistogramsFilename),
+          *histograms,
+          compressed_histograms)) {
+    compressed_histograms->clear();
+  }
+}
+
 }  // namespace
 
 // static
@@ -103,6 +117,7 @@ FeedbackData::FeedbackData() : profile_(NULL),
                                trace_id_(0),
                                feedback_page_data_complete_(false),
                                syslogs_compression_complete_(false),
+                               histograms_compression_complete_(false),
                                attached_file_compression_complete_(false),
                                report_sent_(false) {
 }
@@ -144,6 +159,25 @@ void FeedbackData::SetAndCompressSystemInfo(
   }
 }
 
+void FeedbackData::SetAndCompressHistograms(
+    scoped_ptr<std::string> histograms) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  histograms_ = histograms.Pass();
+  if (histograms_.get()) {
+    std::string* compressed_histograms_ptr = new std::string;
+    scoped_ptr<std::string> compressed_histograms(compressed_histograms_ptr);

[Alexei Svitkine (slow), 2013/11/02 00:05:26]

Wouldn't |compressed_histograms_ptr| be freed as soon as |compressed_histograms|
goes out of scope? (i.e. right after the task is posted and the function ends)

The posted tasks will then get a dangling pointer...

Probably, you should just use a member variable to store this.

[michaelpg, 2013/11/02 17:39:09]

ln 177: base::Passed(&compressed_histograms) lets us pass the compressed
histograms to OnCompressedHistogramsComplete as a scoped_ptr, in doing so it
nullifies compressed_histograms.

That's why we create compressed_histograms_ptr for use at ln 174, because
compressed_histograms.get() wouldn't work if the args to the second base::Bind
is evaluated first.

The same pattern has been in use for SetAndCompressSystemInfo, above.

[Alexei Svitkine (slow), 2013/11/04 15:26:40]

I see - thanks for the explanation. This pattern definitely looks very confusing
- and I don't like it, but if it's correct, then I guess its fine.

Though, it would also be good to have some tests that exercise this codepath, so
that the non-trivial memory ownership gets coverage on the memory bots. (Even if
it's correct now, this will ensure that a subtle refactor won't break it.)

+    BrowserThread::PostBlockingPoolTaskAndReply(

[michaelpg, 2013/11/04 17:25:18]

I'll look into testing this.

+        FROM_HERE,
+        base::Bind(&ZipHistograms,
+                   histograms_.get(),
+                   compressed_histograms_ptr),
+        base::Bind(&FeedbackData::OnCompressHistogramsComplete,
+                   this,
+                   base::Passed(&compressed_histograms)));
+  }
+}
+
 void FeedbackData::AttachAndCompressFileData(
     scoped_ptr<std::string> attached_filedata) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -199,6 +233,16 @@ void FeedbackData::OnCompressLogsComplete(
   SendReport();
 }
 
+void FeedbackData::OnCompressHistogramsComplete(
+    scoped_ptr<std::string> compressed_histograms) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  compressed_histograms_ = compressed_histograms.Pass();
+  histograms_compression_complete_ = true;
+
+  SendReport();
+}
+
 void FeedbackData::OnCompressFileComplete(
     scoped_ptr<std::string> compressed_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -217,6 +261,7 @@ void FeedbackData::OnCompressFileComplete(
 
 bool FeedbackData::IsDataComplete() {
   return (!sys_info_.get() || syslogs_compression_complete_) &&
+      (!histograms_.get() || histograms_compression_complete_) &&
       (!attached_filedata_.get() || attached_file_compression_complete_) &&
       !trace_id_ &&
       feedback_page_data_complete_;