Feedback report improvements

- Avoid copying mega bytes of feedback data when
  sending reports.
- Explicitly use OnceCallback and RepeatingCallback
  when appropriate.

BUG=748742
TEST=manual + unittests.

Change-Id: I6fb76e50f41f01a6df57e401cbfe1a1033e4db82
Reviewed-on: https://chromium-review.googlesource.com/1121077Reviewed-by: Rahul Chaturvedi <rkc@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571947}

chrome/browser/feedback/feedback_profile_observer.cc

@@ -50,10 +50,11 @@ void FeedbackProfileObserver::Observe(
 
 void FeedbackProfileObserver::QueueSingleReport(
     feedback::FeedbackUploader* uploader,
-    const std::string& data) {
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+    std::unique_ptr<std::string> data) {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
       base::BindOnce(&FeedbackUploaderChrome::QueueReport,
-                                         uploader->AsWeakPtr(), data));
+                     uploader->AsWeakPtr(), std::move(data)));
 }
 
 void FeedbackProfileObserver::QueueUnsentReports(
@@ -61,11 +62,11 @@ void FeedbackProfileObserver::QueueUnsentReports(
   feedback::FeedbackUploaderChrome* uploader =
       feedback::FeedbackUploaderFactoryChrome::GetForBrowserContext(context);
   uploader->task_runner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &FeedbackReport::LoadReportsAndQueue,
+      FROM_HERE, base::BindOnce(&FeedbackReport::LoadReportsAndQueue,
                                 uploader->feedback_reports_path(),
-          base::Bind(&FeedbackProfileObserver::QueueSingleReport, uploader)));
+                                base::BindRepeating(
+                                    &FeedbackProfileObserver::QueueSingleReport,
+                                    uploader)));
 }
 
 }  // namespace feedback

chrome/browser/feedback/feedback_profile_observer.h

@@ -41,7 +41,7 @@ class FeedbackProfileObserver : public content::NotificationObserver {
   void QueueUnsentReports(content::BrowserContext* context);
 
   static void QueueSingleReport(feedback::FeedbackUploader* uploader,
-                                const std::string& data);
+                                std::unique_ptr<std::string> data);
 
   // Used to track creation of profiles so we can load any unsent reports
   // for that profile.

components/feedback/feedback_data.cc

@@ -72,8 +72,8 @@ void FeedbackData::SetAndCompressSystemInfo(
     AddLogs(std::move(sys_info));
     base::PostTaskWithTraitsAndReply(
         FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
-        base::Bind(&FeedbackData::CompressLogs, this),
-        base::Bind(&FeedbackData::OnCompressComplete, this));
+        base::BindOnce(&FeedbackData::CompressLogs, this),
+        base::BindOnce(&FeedbackData::OnCompressComplete, this));
   }
 }
 
@@ -143,9 +143,9 @@ void FeedbackData::SendReport() {
     report_sent_ = true;
     userfeedback::ExtensionSubmit feedback_data;
     PrepareReport(&feedback_data);
-    std::string post_body;
-    feedback_data.SerializeToString(&post_body);
-    uploader_->QueueReport(post_body);
+    auto post_body = std::make_unique<std::string>();
+    feedback_data.SerializeToString(post_body.get());
+    uploader_->QueueReport(std::move(post_body));
   }
 }
 

components/feedback/feedback_report.cc

@@ -12,6 +12,8 @@
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 
+namespace feedback {
+
 namespace {
 
 constexpr base::FilePath::CharType kFeedbackReportFilenameWildcard[] =
@@ -21,44 +23,45 @@ constexpr char kFeedbackReportFilenamePrefix[] = "Feedback Report.";
 
 void WriteReportOnBlockingPool(const base::FilePath reports_path,
                                const base::FilePath& file,
-                               const std::string& data) {
+                               scoped_refptr<FeedbackReport> report) {
   DCHECK(reports_path.IsParent(file));
   if (!base::DirectoryExists(reports_path)) {
     base::File::Error error;
     if (!base::CreateDirectoryAndGetError(reports_path, &error))
       return;
   }
-  base::ImportantFileWriter::WriteFileAtomically(file, data, "FeedbackReport");
+  base::ImportantFileWriter::WriteFileAtomically(file, report->data(),
+                                                 "FeedbackReport");
 }
 
 }  // namespace
 
-namespace feedback {
-
 FeedbackReport::FeedbackReport(
     const base::FilePath& path,
     const base::Time& upload_at,
-    const std::string& data,
+    std::unique_ptr<std::string> data,
     scoped_refptr<base::SequencedTaskRunner> task_runner)
     : reports_path_(path),
       upload_at_(upload_at),
-      data_(data),
+      data_(std::move(data)),
       reports_task_runner_(task_runner) {
   if (reports_path_.empty())
     return;
   file_ = reports_path_.AppendASCII(
       kFeedbackReportFilenamePrefix + base::GenerateGUID());
 
-  reports_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &WriteReportOnBlockingPool, reports_path_, file_, data_));
+  reports_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&WriteReportOnBlockingPool, reports_path_, file_,
+                     base::WrapRefCounted<FeedbackReport>(this)));
 }
 
 // static
 const char FeedbackReport::kCrashReportIdsKey[]  = "crash_report_ids";
 
 // static
-void FeedbackReport::LoadReportsAndQueue(
-    const base::FilePath& user_dir, QueueCallback callback) {
+void FeedbackReport::LoadReportsAndQueue(const base::FilePath& user_dir,
+                                         const QueueCallback& callback) {
   if (user_dir.empty())
     return;
 
@@ -69,9 +72,9 @@ void FeedbackReport::LoadReportsAndQueue(
   for (base::FilePath name = enumerator.Next();
        !name.empty();
        name = enumerator.Next()) {
-    std::string data;
-    if (ReadFileToString(name, &data))
-      callback.Run(data);
+    auto data = std::make_unique<std::string>();
+    if (ReadFileToString(name, data.get()))
+      callback.Run(std::move(data));
     base::DeleteFile(name, false);
   }
 }
@@ -79,7 +82,7 @@ void FeedbackReport::LoadReportsAndQueue(
 void FeedbackReport::DeleteReportOnDisk() {
   reports_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(base::IgnoreResult(&base::DeleteFile), file_, false));
+      base::BindOnce(base::IgnoreResult(&base::DeleteFile), file_, false));
 }
 
 FeedbackReport::~FeedbackReport() {}

components/feedback/feedback_report.h

@@ -19,16 +19,19 @@ class SequencedTaskRunner;
 
 namespace feedback {
 
-typedef base::Callback<void(const std::string&)> QueueCallback;
+// Repeating since for every feedback report file on disk, the callback to
+// queue it in the uploader needs to be invoked.
+using QueueCallback =
+    base::RepeatingCallback<void(std::unique_ptr<std::string>)>;
 
 // This class holds a feedback report. Once a report is created, a disk backup
 // for it is created automatically. This backup needs to explicitly be
 // deleted by calling DeleteReportOnDisk.
-class FeedbackReport : public base::RefCounted<FeedbackReport> {
+class FeedbackReport : public base::RefCountedThreadSafe<FeedbackReport> {
  public:
   FeedbackReport(const base::FilePath& path,
                  const base::Time& upload_at,
-                 const std::string& data,
+                 std::unique_ptr<std::string> data,
                  scoped_refptr<base::SequencedTaskRunner> task_runner);
 
   // The ID of the product specific data for the crash report IDs as stored by
@@ -38,7 +41,7 @@ class FeedbackReport : public base::RefCounted<FeedbackReport> {
   // Loads the reports still on disk and queues then using the given callback.
   // This call blocks on the file reads.
   static void LoadReportsAndQueue(const base::FilePath& user_dir,
-                                  QueueCallback callback);
+                                  const QueueCallback& callback);
 
   // Stops the disk write of the report and deletes the report file if already
   // written.
@@ -46,10 +49,10 @@ class FeedbackReport : public base::RefCounted<FeedbackReport> {
 
   const base::Time& upload_at() const { return upload_at_; }
   void set_upload_at(const base::Time& time) { upload_at_ = time; }
-  const std::string& data() const { return data_; }
+  const std::string& data() const { return *data_; }
 
  private:
-  friend class base::RefCounted<FeedbackReport>;
+  friend class base::RefCountedThreadSafe<FeedbackReport>;
   virtual ~FeedbackReport();
 
   // Name of the file corresponding to this report.
@@ -57,7 +60,7 @@ class FeedbackReport : public base::RefCounted<FeedbackReport> {
 
   base::FilePath reports_path_;
   base::Time upload_at_;  // Upload this report at or after this time.
-  std::string data_;
+  std::unique_ptr<std::string> data_;
 
   scoped_refptr<base::SequencedTaskRunner> reports_task_runner_;
 

components/feedback/feedback_uploader.cc

@@ -72,8 +72,8 @@ void FeedbackUploader::SetMinimumRetryDelayForTesting(base::TimeDelta delay) {
   g_minimum_retry_delay = delay;
 }
 
-void FeedbackUploader::QueueReport(const std::string& data) {
-  QueueReportWithDelay(data, base::TimeDelta());
+void FeedbackUploader::QueueReport(std::unique_ptr<std::string> data) {
+  QueueReportWithDelay(std::move(data), base::TimeDelta());
 }
 
 void FeedbackUploader::StartDispatchingReport() {
@@ -205,10 +205,11 @@ void FeedbackUploader::UpdateUploadTimer() {
   }
 }
 
-void FeedbackUploader::QueueReportWithDelay(const std::string& data,
+void FeedbackUploader::QueueReportWithDelay(std::unique_ptr<std::string> data,
                                             base::TimeDelta delay) {
   reports_queue_.emplace(base::MakeRefCounted<FeedbackReport>(
-      feedback_reports_path_, base::Time::Now() + delay, data, task_runner_));
+      feedback_reports_path_, base::Time::Now() + delay, std::move(data),
+      task_runner_));
   UpdateUploadTimer();
 }
 

components/feedback/feedback_uploader.h

@@ -43,7 +43,7 @@ class FeedbackUploader : public KeyedService,
   static void SetMinimumRetryDelayForTesting(base::TimeDelta delay);
 
   // Queues a report for uploading.
-  void QueueReport(const std::string& data);
+  void QueueReport(std::unique_ptr<std::string> data);
 
   bool QueueEmpty() const { return reports_queue_.empty(); }
 
@@ -101,7 +101,8 @@ class FeedbackUploader : public KeyedService,
   // Update our timer for uploading the next report.
   void UpdateUploadTimer();
 
-  void QueueReportWithDelay(const std::string& data, base::TimeDelta delay);
+  void QueueReportWithDelay(std::unique_ptr<std::string> data,
+                            base::TimeDelta delay);
 
   // Browser context this uploader was created for.
   content::BrowserContext* context_;

components/feedback/feedback_uploader_dispatch_unittest.cc

@@ -31,6 +31,10 @@ constexpr int kHttpPostSuccessNoContent = 204;
 constexpr int kHttpPostFailClientError = 400;
 constexpr int kHttpPostFailServerError = 500;
 
+void QueueReport(FeedbackUploader* uploader, const std::string& report_data) {
+  uploader->QueueReport(std::make_unique<std::string>(report_data));
+}
+
 }  // namespace
 
 class FeedbackUploaderDispatchTest : public ::testing::Test {
@@ -75,7 +79,7 @@ TEST_F(FeedbackUploaderDispatchTest, VariationHeaders) {
       context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
 
   net::TestURLFetcherFactory factory;
-  uploader.QueueReport("test");
+  QueueReport(&uploader, "test");
 
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   net::HttpRequestHeaders headers;
@@ -99,7 +103,7 @@ TEST_F(FeedbackUploaderDispatchTest, TestVariousServerResponses) {
   // increase the backoff delay.
   net::TestURLFetcherFactory factory;
   factory.set_remove_fetcher_on_delete(true);
-  uploader.QueueReport("Successful report");
+  QueueReport(&uploader, "Successful report");
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   fetcher->set_response_code(kHttpPostSuccessNoContent);
   fetcher->delegate()->OnURLFetchComplete(fetcher);
@@ -108,7 +112,7 @@ TEST_F(FeedbackUploaderDispatchTest, TestVariousServerResponses) {
 
   // Failed reports due to client errors are not retried. No backoff delay
   // should be doubled.
-  uploader.QueueReport("Client error failed report");
+  QueueReport(&uploader, "Client error failed report");
   fetcher = factory.GetFetcherByID(0);
   fetcher->set_response_code(kHttpPostFailClientError);
   fetcher->delegate()->OnURLFetchComplete(fetcher);
@@ -116,7 +120,7 @@ TEST_F(FeedbackUploaderDispatchTest, TestVariousServerResponses) {
   EXPECT_TRUE(uploader.QueueEmpty());
 
   // Failed reports due to server errors are retried.
-  uploader.QueueReport("Server error failed report");
+  QueueReport(&uploader, "Server error failed report");
   fetcher = factory.GetFetcherByID(0);
   fetcher->set_response_code(kHttpPostFailServerError);
   fetcher->delegate()->OnURLFetchComplete(fetcher);

components/feedback/feedback_uploader_unittest.cc

@@ -52,7 +52,7 @@ class MockFeedbackUploader : public FeedbackUploader {
         FROM_HERE,
         base::BindOnce(
             &FeedbackReport::LoadReportsAndQueue, feedback_reports_path(),
-            base::Bind(&MockFeedbackUploader::QueueSingleReport,
+            base::BindRepeating(&MockFeedbackUploader::QueueSingleReport,
                                 base::SequencedTaskRunnerHandle::Get(), this)));
   }
 
@@ -66,10 +66,10 @@ class MockFeedbackUploader : public FeedbackUploader {
   static void QueueSingleReport(
       scoped_refptr<base::SequencedTaskRunner> main_task_runner,
       MockFeedbackUploader* uploader,
-      const std::string& data) {
+      std::unique_ptr<std::string> data) {
     main_task_runner->PostTask(
         FROM_HERE, base::BindOnce(&MockFeedbackUploader::QueueReport,
-                                  uploader->AsWeakPtr(), data));
+                                  uploader->AsWeakPtr(), std::move(data)));
   }
 
   // FeedbackUploaderChrome:
@@ -122,7 +122,7 @@ class FeedbackUploaderTest : public testing::Test {
   }
 
   void QueueReport(const std::string& data) {
-    uploader_->QueueReport(data);
+    uploader_->QueueReport(std::make_unique<std::string>(data));
   }
 
   MockFeedbackUploader* uploader() const { return uploader_.get(); }