Feedback: Fix regression in sending offline reports.

Don't delete feedback reports files on disk on the
report destruction. This leads to losing pending reports
on sign out or reboot. Instead explicitly delete the files
once successfully uploaded, or when a no-retry failure occurs.

BUG=851687
TEST=Added new test.

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

components/feedback/feedback_report.cc

@@ -82,8 +82,6 @@ void FeedbackReport::DeleteReportOnDisk() {
       base::Bind(base::IgnoreResult(&base::DeleteFile), file_, false));
 }
 
-FeedbackReport::~FeedbackReport() {
-  DeleteReportOnDisk();
-}
+FeedbackReport::~FeedbackReport() {}
 
 }  // namespace feedback

components/feedback/feedback_uploader.cc

@@ -84,6 +84,7 @@ void FeedbackUploader::OnReportUploadSuccess() {
   retry_delay_ = g_minimum_retry_delay;
   is_dispatching_ = false;
   // Explicitly release the successfully dispatched report.
+  report_being_dispatched_->DeleteReportOnDisk();
   report_being_dispatched_ = nullptr;
   UpdateUploadTimer();
 }
@@ -94,6 +95,9 @@ void FeedbackUploader::OnReportUploadFailure(bool should_retry) {
     retry_delay_ *= 2;
     report_being_dispatched_->set_upload_at(retry_delay_ + base::Time::Now());
     reports_queue_.emplace(report_being_dispatched_);
+  } else {
+    // The report won't be retried, hence explicitly delete its file on disk.
+    report_being_dispatched_->DeleteReportOnDisk();
   }
 
   // The report dispatching failed, and should either be retried or not. In all

components/feedback/feedback_uploader_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/stl_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/task_scheduler/task_traits.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "components/feedback/feedback_report.h"
 #include "components/feedback/feedback_uploader_factory.h"
 #include "content/public/test/test_browser_context.h"
@@ -46,6 +47,15 @@ class MockFeedbackUploader : public FeedbackUploader {
     run_loop_->Run();
   }
 
+  void SimulateLoadingOfflineReports() {
+    task_runner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &FeedbackReport::LoadReportsAndQueue, feedback_reports_path(),
+            base::Bind(&MockFeedbackUploader::QueueSingleReport,
+                       base::SequencedTaskRunnerHandle::Get(), this)));
+  }
+
   const std::map<std::string, unsigned int>& dispatched_reports() const {
     return dispatched_reports_;
   }
@@ -53,6 +63,15 @@ class MockFeedbackUploader : public FeedbackUploader {
   void set_simulate_failure(bool value) { simulate_failure_ = value; }
 
  private:
+  static void QueueSingleReport(
+      scoped_refptr<base::SequencedTaskRunner> main_task_runner,
+      MockFeedbackUploader* uploader,
+      const std::string& data) {
+    main_task_runner->PostTask(
+        FROM_HERE, base::BindOnce(&MockFeedbackUploader::QueueReport,
+                                  uploader->AsWeakPtr(), data));
+  }
+
   // FeedbackUploaderChrome:
   void StartDispatchingReport() override {
     if (base::ContainsKey(dispatched_reports_,
@@ -93,11 +112,15 @@ class FeedbackUploaderTest : public testing::Test {
  public:
   FeedbackUploaderTest() {
     FeedbackUploader::SetMinimumRetryDelayForTesting(kRetryDelayForTest);
-    uploader_ = std::make_unique<MockFeedbackUploader>(&context_);
+    RecreateUploader();
   }
 
   ~FeedbackUploaderTest() override = default;
 
+  void RecreateUploader() {
+    uploader_ = std::make_unique<MockFeedbackUploader>(&context_);
+  }
+
   void QueueReport(const std::string& data) {
     uploader_->QueueReport(data);
   }
@@ -156,4 +179,32 @@ TEST_F(FeedbackUploaderTest, QueueMultipleWithFailures) {
   EXPECT_EQ(uploader()->dispatched_reports().at(kReportFive), 1u);
 }
 
+TEST_F(FeedbackUploaderTest, SimulateOfflineReports) {
+  // Simulate offline reports by failing to upload three reports.
+  uploader()->set_simulate_failure(true);
+  QueueReport(kReportOne);
+  QueueReport(kReportTwo);
+  QueueReport(kReportThree);
+
+  // All three reports will be attempted to be uploaded, but the uploader queue
+  // will remain having three reports since they all failed.
+  uploader()->set_expected_reports(3);
+  uploader()->RunMessageLoop();
+  EXPECT_EQ(uploader()->dispatched_reports().size(), 3u);
+  EXPECT_FALSE(uploader()->QueueEmpty());
+
+  // Simulate a sign out / resign in by recreating the uploader. This should not
+  // clear any pending feedback report files on disk, and hence they can be
+  // reloaded.
+  RecreateUploader();
+  uploader()->SimulateLoadingOfflineReports();
+  uploader()->set_expected_reports(3);
+  uploader()->RunMessageLoop();
+
+  // The three reports were loaded, successfully uploaded, and the uploader
+  // queue is now empty.
+  EXPECT_EQ(uploader()->dispatched_reports().size(), 3u);
+  EXPECT_TRUE(uploader()->QueueEmpty());
+}
+
 }  // namespace feedback