Send compressed histograms with system logs when sending feedback.

chrome/browser/feedback/feedback_util.cc

 // Copyright (c) 2012 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/feedback/feedback_util.h"
 
 #include <sstream>
 #include <string>
 #include <vector>
 
@@ skipping 74 matching lines @@
 const int kHttpPostSuccessNoContent = 204;
 const int kHttpPostFailNoConnection = -1;
 const int kHttpPostFailClientError = 400;
 const int kHttpPostFailServerError = 500;
 
 const int64 kInitialRetryDelay = 900000;  // 15 minutes
 const int64 kRetryDelayIncreaseFactor = 2;
 const int64 kRetryDelayLimit = 14400000;  // 4 hours
 
 const char kArbitraryMimeType[] = "application/octet-stream";
+const char kHistogramsAttachmentName[] = "histograms.zip";
 const char kLogsAttachmentName[] = "system_logs.zip";
 
 #if defined(OS_CHROMEOS)
 const int kChromeOSProductId = 208;
 #else
 const int kChromeBrowserProductId = 237;
 #endif
 
 // Simple net::URLFetcherDelegate to clean up URLFetcher on completion.
 class PostCleanup : public net::URLFetcherDelegate {
@@ skipping 105 matching lines @@
   // Don't bother with empty keys or values
   if (key == "" || value == "") return;
   // Create log_value object and add it to the web_data object
   userfeedback::ProductSpecificData log_value;
   log_value.set_key(key);
   log_value.set_value(value);
   userfeedback::WebData* web_data = feedback_data->mutable_web_data();
   *(web_data->add_product_specific_data()) = log_value;
 }
 
+// Adds data as an attachment to feedback_data if the data is non-empty.
+void AddAttachment(userfeedback::ExtensionSubmit* feedback_data,
+                   const char* name,
+                   std::string* data) {
+  if (data == NULL || data->empty())
+    return;
+
+  userfeedback::ProductSpecificBinaryData attachment;
+  attachment.set_mime_type(kArbitraryMimeType);
+  attachment.set_name(name);
+  attachment.set_data(*data);
+  *(feedback_data->add_product_specific_binary_data()) = attachment;

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

Nit: Why not:

userfeedback::ProductSpecificBinaryData* attachment =
    feedback_data->add_product_specific_binary_data();
attachment->set_mime_type(kArbitraryMimeType);
attachment->set_name(name);
attachment->set_data(*data);

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

I'm not familiar with protobuf. It looks like RepeatedField is basically a
vector and doesn't pre-emptively allocate new memory except when adding data, so
your version would be safe provided the attachment pointer isn't used after the
next call to add_product_specific_binary_data(). Which is true here, since
AddAttachment is the only one using that function. But is it risky to rely on
those assumptions or is that okay?

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

I don't think your statement is correct (that it's unsafe to use the pointer
after another add_* call). Per the docs, its managed as a vector of pointers:
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...

... which means that a grow of the array will not touch where the allocated
sub-elements are and thus shouldn't invalidate the pointers.

Also, the way I suggest is how its done elsewhere in Chrome (e.g. UMA logs
uploads) and is more efficient (avoids a copy of the element).

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

You're right, I was thinking a RepeatedField rather than a RepeatedPtrField and
that the pointer pointed into the RepeatedField's allocated space somehow which
doesn't actually happen. Thanks.

On 2013/11/04 15:26:40, Alexei Svitkine wrote:

+}
+
 }  // namespace
 
 namespace chrome {
 
 const char kAppLauncherCategoryTag[] = "AppLauncher";
 
 void ShowFeedbackPage(Browser* browser,
                       const std::string& description_template,
                       const std::string& category_tag) {
   GURL page_url;
@@ skipping 51 matching lines @@
   web_data->mutable_navigator()->set_user_agent(content::GetUserAgent(GURL()));
 
   gfx::Rect screen_size;
   if (data->sys_info()) {
     for (FeedbackData::SystemLogsMap::const_iterator i =
         data->sys_info()->begin(); i != data->sys_info()->end(); ++i) {
       if (FeedbackData::BelowCompressionThreshold(i->second))
         AddFeedbackData(&feedback_data, i->first, i->second);
     }
 
-    if (data->compressed_logs() && data->compressed_logs()->size()) {
-      userfeedback::ProductSpecificBinaryData attachment;
-      attachment.set_mime_type(kArbitraryMimeType);
-      attachment.set_name(kLogsAttachmentName);
-      attachment.set_data(*(data->compressed_logs()));
-      *(feedback_data.add_product_specific_binary_data()) = attachment;
-    }
+    AddAttachment(&feedback_data, kLogsAttachmentName, data->compressed_logs());
   }
 
-  if (!data->attached_filename().empty() &&
-      data->attached_filedata() &&
-      !data->attached_filedata()->empty()) {
-    userfeedback::ProductSpecificBinaryData attached_file;
-    attached_file.set_mime_type(kArbitraryMimeType);
+  if (data->histograms()) {
+    AddAttachment(&feedback_data,
+                  kHistogramsAttachmentName,
+                  data->compressed_histograms());
+  }
+
+  if (!data->attached_filename().empty()) {
     // We need to use the UTF8Unsafe methods here to accomodate Windows, which
     // uses wide strings to store filepaths.
     std::string name = base::FilePath::FromUTF8Unsafe(
         data->attached_filename()).BaseName().AsUTF8Unsafe();
-    attached_file.set_name(name);
-    attached_file.set_data(*data->attached_filedata());
-    *(feedback_data.add_product_specific_binary_data()) = attached_file;
+    AddAttachment(&feedback_data, name.c_str(), data->attached_filedata());
   }
 
   // NOTE: Screenshot needs to be processed after system info since we'll get
   // the screenshot dimensions from system info.
   if (data->image() && data->image()->size()) {
     userfeedback::PostedScreenshot screenshot;
     screenshot.set_mime_type(kPngMimeType);
 
     // Set that we 'have' dimensions of the screenshot. These dimensions are
     // ignored by the server but are a 'required' field in the protobuf.
@@ skipping 58 matching lines @@
   if (!zip::Zip(temp_path, zip_file, false))
     return false;
 
   if (!base::ReadFileToString(zip_file, compressed_logs))
     return false;
 
   return true;
 }
 
 }  // namespace feedback_util