Fix bugs leading to spamming the feedback server with unsent corrupt reports

The feedback uploader can spam the server with tons of instances of the exact
same report due to the following:

1) Corrupted reports will be retied once every 60 minutes and on every login.
   Each retry will fail with the exact same ClientError, so there's no point
   in retrying.
2) FeedbackProfileObserver and FeedbackUploader were using two different types
   of task runners with different threads from their blocking pools. This can
   lead to a race condition (when corrupted reports are being resent on login)
   between the thread that reads the unsent file, and the thread that rewrites
   the file before it's attempted to be resent.

This CL:

- Fixes (1) by introducing an exponential backoff delay on failures other
  than ClientErrors, and not retry on Client Errors.
- Fixes (2) by using the same type of SingleThreadTaskRunner for all feedback
  background tasks to ensure sequencing and thread affinity.
- Adds some tests.

BUG=739885
TEST=adds new tests.

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

chrome/browser/feedback/feedback_profile_observer.cc

@@ -60,11 +60,11 @@ void FeedbackProfileObserver::QueueUnsentReports(
     content::BrowserContext* context) {
   feedback::FeedbackUploader* uploader =
       feedback::FeedbackUploaderFactory::GetForBrowserContext(context);
-  base::PostTaskWithTraits(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
+  uploader->task_runner()->PostTask(
+      FROM_HERE,
       base::BindOnce(
           &FeedbackReport::LoadReportsAndQueue,
-          uploader->GetFeedbackReportsPath(),
+          uploader->feedback_reports_path(),
           base::Bind(&FeedbackProfileObserver::QueueSingleReport, uploader)));
 }
 

components/feedback/feedback_data_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
+#include "components/feedback/feedback_report.h"
 #include "components/feedback/feedback_uploader.h"
 #include "components/feedback/feedback_uploader_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -20,23 +21,28 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace feedback {
+
 namespace {
 
-const char kHistograms[] = "";
-const char kImageData[] = "";
-const char kFileData[] = "";
+constexpr char kHistograms[] = "";
+constexpr char kImageData[] = "";
+constexpr char kFileData[] = "";
 
 class MockUploader : public feedback::FeedbackUploader, public KeyedService {
  public:
-  MockUploader(content::BrowserContext* context)
-      : FeedbackUploader(context ? context->GetPath() : base::FilePath()) {}
+  MockUploader(content::BrowserContext* context,
+               scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : FeedbackUploader(context ? context->GetPath() : base::FilePath(),
+                         task_runner) {}
 
-  MOCK_METHOD1(DispatchReport, void(const std::string&));
+  MOCK_METHOD1(DispatchReport, void(scoped_refptr<FeedbackReport>));
 };
 
 std::unique_ptr<KeyedService> CreateFeedbackUploaderService(
     content::BrowserContext* context) {
-  std::unique_ptr<MockUploader> uploader(new MockUploader(context));
+  auto uploader = base::MakeUnique<MockUploader>(
+      context, FeedbackUploaderFactory::CreateUploaderTaskRunner());
   EXPECT_CALL(*uploader, DispatchReport(testing::_)).Times(1);
   return std::move(uploader);
 }
@@ -47,8 +53,6 @@ std::unique_ptr<std::string> MakeScoped(const char* str) {
 
 }  // namespace
 
-namespace feedback {
-
 class FeedbackDataTest : public testing::Test {
  protected:
   FeedbackDataTest()

components/feedback/feedback_report.cc

@@ -15,10 +15,10 @@
 
 namespace {
 
-const base::FilePath::CharType kFeedbackReportFilenameWildcard[] =
+constexpr base::FilePath::CharType kFeedbackReportFilenameWildcard[] =
     FILE_PATH_LITERAL("Feedback Report.*");
 
-const char kFeedbackReportFilenamePrefix[] = "Feedback Report.";
+constexpr char kFeedbackReportFilenamePrefix[] = "Feedback Report.";
 
 void WriteReportOnBlockingPool(const base::FilePath reports_path,
                                const base::FilePath& file,

components/feedback/feedback_report.h

@@ -45,6 +45,7 @@ class FeedbackReport : public base::RefCounted<FeedbackReport> {
   void DeleteReportOnDisk();
 
   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_; }
 
  private:

components/feedback/feedback_uploader.cc

@@ -8,102 +8,100 @@
 
 #include "base/callback.h"
 #include "base/command_line.h"
-#include "base/files/file_path.h"
-#include "base/sequenced_task_runner.h"
-#include "base/task_runner_util.h"
-#include "base/task_scheduler/post_task.h"
 #include "components/feedback/feedback_report.h"
+#include "components/feedback/feedback_switches.h"
 
 namespace feedback {
+
 namespace {
 
-const char kFeedbackPostUrl[] =
+constexpr char kFeedbackPostUrl[] =
     "https://www.google.com/tools/feedback/chrome/__submit";
 
-const int64_t kRetryDelayMinutes = 60;
-
-const base::FilePath::CharType kFeedbackReportPath[] =
+constexpr base::FilePath::CharType kFeedbackReportPath[] =
     FILE_PATH_LITERAL("Feedback Reports");
 
-}  // namespace
-
-bool FeedbackUploader::ReportsUploadTimeComparator::operator()(
-    const scoped_refptr<FeedbackReport>& a,
-    const scoped_refptr<FeedbackReport>& b) const {
-  return a->upload_at() > b->upload_at();
+// The minimum time to wait before uploading reports are retried. Exponential
+// backoff delay is applied on successive failures.
+// This value can be overriden by tests by calling
+// FeedbackUploader::SetMinimumRetryDelayForTesting().
+base::TimeDelta g_minimum_retry_delay = base::TimeDelta::FromMinutes(60);
+
+GURL GetFeedbackPostGURL() {
+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+  return GURL(command_line.HasSwitch(switches::kFeedbackServer)
+                  ? command_line.GetSwitchValueASCII(switches::kFeedbackServer)
+                  : kFeedbackPostUrl);
 }
 
-FeedbackUploader::FeedbackUploader(const base::FilePath& path)
-    : report_path_(path.Append(kFeedbackReportPath)),
-      retry_delay_(base::TimeDelta::FromMinutes(kRetryDelayMinutes)),
-      url_(kFeedbackPostUrl) {
-  Init();
-}
+}  // namespace
 
-FeedbackUploader::FeedbackUploader(const base::FilePath& path,
-                                   const std::string& url)
-    : report_path_(path.Append(kFeedbackReportPath)),
-      retry_delay_(base::TimeDelta::FromMinutes(kRetryDelayMinutes)),
-      url_(url) {
-  Init();
+FeedbackUploader::FeedbackUploader(
+    const base::FilePath& path,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+    : feedback_reports_path_(path.Append(kFeedbackReportPath)),
+      task_runner_(task_runner),
+      retry_delay_(g_minimum_retry_delay),
+      feedback_post_url_(GetFeedbackPostGURL()) {
+  DCHECK(task_runner_);
 }
 
 FeedbackUploader::~FeedbackUploader() {}
 
-void FeedbackUploader::Init() {
-  dispatch_callback_ = base::Bind(&FeedbackUploader::DispatchReport,
-                                  AsWeakPtr());
+// static
+void FeedbackUploader::SetMinimumRetryDelayForTesting(base::TimeDelta delay) {
+  g_minimum_retry_delay = delay;
 }
 
 void FeedbackUploader::QueueReport(const std::string& data) {
   QueueReportWithDelay(data, base::TimeDelta());
 }
 
+void FeedbackUploader::OnReportUploadSuccess() {
+  retry_delay_ = g_minimum_retry_delay;
+  UpdateUploadTimer();
+}
+
+void FeedbackUploader::OnReportUploadFailure(
+    scoped_refptr<FeedbackReport> report) {
+  // Implement a backoff delay by doubling the retry delay on each failure.
+  retry_delay_ *= 2;
+  report->set_upload_at(retry_delay_ + base::Time::Now());
+  reports_queue_.emplace(report);
+  UpdateUploadTimer();
+}
+
+bool FeedbackUploader::ReportsUploadTimeComparator::operator()(
+    const scoped_refptr<FeedbackReport>& a,
+    const scoped_refptr<FeedbackReport>& b) const {
+  return a->upload_at() > b->upload_at();
+}
+
 void FeedbackUploader::UpdateUploadTimer() {
   if (reports_queue_.empty())
     return;
 
   scoped_refptr<FeedbackReport> report = reports_queue_.top();
-  base::Time now = base::Time::Now();
+  const base::Time now = base::Time::Now();
   if (report->upload_at() <= now) {
     reports_queue_.pop();
-    dispatch_callback_.Run(report->data());
+    DispatchReport(report);
     report->DeleteReportOnDisk();
   } else {
     // Stop the old timer and start an updated one.
-    if (upload_timer_.IsRunning())
-      upload_timer_.Stop();
+    upload_timer_.Stop();
     upload_timer_.Start(
         FROM_HERE, report->upload_at() - now, this,
         &FeedbackUploader::UpdateUploadTimer);
   }
 }
 
-void FeedbackUploader::RetryReport(const std::string& data) {
-  QueueReportWithDelay(data, retry_delay_);
-}
-
 void FeedbackUploader::QueueReportWithDelay(const std::string& data,
                                             base::TimeDelta delay) {
-  // Uses a BLOCK_SHUTDOWN file task runner because we really don't want to
-  // lose reports.
-  scoped_refptr<base::SequencedTaskRunner> task_runner =
-      base::CreateSequencedTaskRunnerWithTraits(
-          {base::MayBlock(), base::TaskPriority::BACKGROUND,
-           base::TaskShutdownBehavior::BLOCK_SHUTDOWN});
-
-  reports_queue_.push(new FeedbackReport(report_path_,
-                                         base::Time::Now() + delay,
-                                         data,
-                                         task_runner));
+  reports_queue_.emplace(base::MakeRefCounted<FeedbackReport>(
+      feedback_reports_path_, base::Time::Now() + delay, data, task_runner_));
   UpdateUploadTimer();
 }
 
-void FeedbackUploader::setup_for_test(
-    const ReportDataCallback& dispatch_callback,
-    const base::TimeDelta& retry_delay) {
-  dispatch_callback_ = dispatch_callback;
-  retry_delay_ = retry_delay;
-}
-
 }  // namespace feedback

components/feedback/feedback_uploader.h

@@ -6,18 +6,19 @@
 #define COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_H_
 
 #include <queue>
-#include <string>
+#include <vector>
 
-#include "base/files/file_util.h"
+#include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "url/gurl.h"
 
 namespace feedback {
 
-typedef base::Callback<void(const std::string&)> ReportDataCallback;
-
 class FeedbackReport;
 
 // FeedbackUploader is used to add a feedback report to the queue of reports
@@ -25,19 +26,41 @@ class FeedbackReport;
 // tried again when it's turn comes up next in the queue.
 class FeedbackUploader : public base::SupportsWeakPtr<FeedbackUploader> {
  public:
-  FeedbackUploader(const base::FilePath& path);
   FeedbackUploader(const base::FilePath& path,
-                   const std::string& url);
+                   scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   virtual ~FeedbackUploader();
 
-  // Queues a report for uploading.
-  virtual void QueueReport(const std::string& data);
+  static void SetMinimumRetryDelayForTesting(base::TimeDelta delay);
 
-  base::FilePath GetFeedbackReportsPath() { return report_path_; }
+  // Queues a report for uploading.
+  void QueueReport(const std::string& data);
 
   bool QueueEmpty() const { return reports_queue_.empty(); }
 
+  const base::FilePath& feedback_reports_path() const {
+    return feedback_reports_path_;
+  }
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner() const {
+    return task_runner_;
+  }
+
+  base::TimeDelta retry_delay() const { return retry_delay_; }
+
+  const GURL& feedback_post_url() const { return feedback_post_url_; }
+
  protected:
+  // Invoked when a feedback report upload succeeds. It will reset the
+  // |retry_delay_| to its minimum value and schedules the next report upload if
+  // any.
+  void OnReportUploadSuccess();
+
+  // Invoked when |report| fails to upload. It will double the |retry_delay_|
+  // and reenqueue |report| with the new delay. All subsequent retries will keep
+  // increasing the delay until a successful upload is encountered.
+  void OnReportUploadFailure(scoped_refptr<FeedbackReport> report);
+
+ private:
   friend class FeedbackUploaderTest;
 
   struct ReportsUploadTimeComparator {
@@ -45,34 +68,31 @@ class FeedbackUploader : public base::SupportsWeakPtr<FeedbackUploader> {
                     const scoped_refptr<FeedbackReport>& b) const;
   };
 
-  void Init();
-
   // Dispatches the report to be uploaded.
-  virtual void DispatchReport(const std::string& data) = 0;
+  virtual void DispatchReport(scoped_refptr<FeedbackReport> report) = 0;
 
   // Update our timer for uploading the next report.
   void UpdateUploadTimer();
 
-  // Requeue this report with a delay.
-  void RetryReport(const std::string& data);
-
   void QueueReportWithDelay(const std::string& data, base::TimeDelta delay);
 
-  void setup_for_test(const ReportDataCallback& dispatch_callback,
-                      const base::TimeDelta& retry_delay);
+  const base::FilePath feedback_reports_path_;
 
-  base::FilePath report_path_;
   // Timer to upload the next report at.
   base::OneShotTimer upload_timer_;
+
+  // See comment of |FeedbackUploaderFactory::task_runner_|.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
   // Priority queue of reports prioritized by the time the report is supposed
   // to be uploaded at.
   std::priority_queue<scoped_refptr<FeedbackReport>,
-                      std::vector<scoped_refptr<FeedbackReport> >,
-                      ReportsUploadTimeComparator> reports_queue_;
+                      std::vector<scoped_refptr<FeedbackReport>>,
+                      ReportsUploadTimeComparator>
+      reports_queue_;
 
-  ReportDataCallback dispatch_callback_;
   base::TimeDelta retry_delay_;
-  std::string url_;
+  const GURL feedback_post_url_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedbackUploader);
 };

components/feedback/feedback_uploader_chrome.cc

@@ -4,15 +4,10 @@
 
 #include "components/feedback/feedback_uploader_chrome.h"
 
-#include <string>
-
 #include "base/callback.h"
-#include "base/command_line.h"
 #include "base/files/file_path.h"
-#include "base/task_runner_util.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/feedback/feedback_report.h"
-#include "components/feedback/feedback_switches.h"
 #include "components/feedback/feedback_uploader_delegate.h"
 #include "components/variations/net/variations_http_headers.h"
 #include "content/public/browser/browser_context.h"
@@ -29,19 +24,17 @@ const char kProtoBufMimeType[] = "application/x-protobuf";
 
 }  // namespace
 
-FeedbackUploaderChrome::FeedbackUploaderChrome(content::BrowserContext* context)
-    : FeedbackUploader(context ? context->GetPath() : base::FilePath()),
+FeedbackUploaderChrome::FeedbackUploaderChrome(
+    content::BrowserContext* context,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+    : FeedbackUploader(context ? context->GetPath() : base::FilePath(),
+                       task_runner),
       context_(context) {
   CHECK(context_);
-  const base::CommandLine& command_line =
-      *base::CommandLine::ForCurrentProcess();
-  if (command_line.HasSwitch(switches::kFeedbackServer))
-    url_ = command_line.GetSwitchValueASCII(switches::kFeedbackServer);
 }
 
-void FeedbackUploaderChrome::DispatchReport(const std::string& data) {
-  GURL post_url(url_);
-
+void FeedbackUploaderChrome::DispatchReport(
+    scoped_refptr<FeedbackReport> report) {
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("chrome_feedback_report_app", R"(
         semantics {
@@ -74,12 +67,13 @@ void FeedbackUploaderChrome::DispatchReport(const std::string& data) {
   // Note: FeedbackUploaderDelegate deletes itself and the fetcher.
   net::URLFetcher* fetcher =
       net::URLFetcher::Create(
-          post_url, net::URLFetcher::POST,
+          feedback_post_url(), net::URLFetcher::POST,
           new FeedbackUploaderDelegate(
-              data,
-              base::Bind(&FeedbackUploaderChrome::UpdateUploadTimer,
+              report,
+              base::Bind(&FeedbackUploaderChrome::OnReportUploadSuccess,
                          AsWeakPtr()),
-              base::Bind(&FeedbackUploaderChrome::RetryReport, AsWeakPtr())),
+              base::Bind(&FeedbackUploaderChrome::OnReportUploadFailure,
+                         AsWeakPtr())),
           traffic_annotation)
           .release();
   data_use_measurement::DataUseUserData::AttachToFetcher(
@@ -88,13 +82,13 @@ void FeedbackUploaderChrome::DispatchReport(const std::string& data) {
   net::HttpRequestHeaders headers;
   // Note: It's OK to pass |is_signed_in| false if it's unknown, as it does
   // not affect transmission of experiments coming from the variations server.
-  bool is_signed_in = false;
+  const bool is_signed_in = false;
   variations::AppendVariationHeaders(fetcher->GetOriginalURL(),
                                      context_->IsOffTheRecord(), false,
                                      is_signed_in, &headers);
   fetcher->SetExtraRequestHeaders(headers.ToString());
 
-  fetcher->SetUploadData(kProtoBufMimeType, data);
+  fetcher->SetUploadData(kProtoBufMimeType, report->data());
   fetcher->SetRequestContext(
       content::BrowserContext::GetDefaultStoragePartition(context_)->
           GetURLRequestContext());

components/feedback/feedback_uploader_chrome.h

@@ -18,11 +18,14 @@ namespace feedback {
 class FeedbackUploaderChrome : public FeedbackUploader,
                                public KeyedService {
  public:
-  explicit FeedbackUploaderChrome(content::BrowserContext* context);
-
-  void DispatchReport(const std::string& data) override;
+  FeedbackUploaderChrome(
+      content::BrowserContext* context,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
  private:
+  // feedback::FeedbackUploader:
+  void DispatchReport(scoped_refptr<FeedbackReport> report) override;
+
   // Browser context this uploader was created for.
   content::BrowserContext* context_;
 

components/feedback/feedback_uploader_chrome_unittest.cc

@@ -8,6 +8,10 @@
 
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
+#include "base/run_loop.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/task_scheduler/task_traits.h"
+#include "components/feedback/feedback_uploader_factory.h"
 #include "components/variations/variations_associated_data.h"
 #include "components/variations/variations_http_header_provider.h"
 #include "content/public/test/test_browser_context.h"
@@ -18,6 +22,17 @@
 
 namespace feedback {
 
+namespace {
+
+constexpr base::TimeDelta kTestRetryDelay =
+    base::TimeDelta::FromMilliseconds(1);
+
+constexpr int kHttpPostSuccessNoContent = 204;
+constexpr int kHttpPostFailClientError = 400;
+constexpr int kHttpPostFailServerError = 500;
+
+}  // namespace
+
 class FeedbackUploaderChromeTest : public ::testing::Test {
  protected:
   FeedbackUploaderChromeTest()
@@ -54,10 +69,11 @@ TEST_F(FeedbackUploaderChromeTest, VariationHeaders) {
   CreateFieldTrialWithId("Test", "Group1", 123);
 
   content::TestBrowserContext context;
-  FeedbackUploaderChrome uploader(&context);
+  FeedbackUploaderChrome uploader(
+      &context, FeedbackUploaderFactory::CreateUploaderTaskRunner());
 
   net::TestURLFetcherFactory factory;
-  uploader.DispatchReport("test");
+  uploader.QueueReport("test");
 
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   net::HttpRequestHeaders headers;
@@ -71,4 +87,40 @@ TEST_F(FeedbackUploaderChromeTest, VariationHeaders) {
   variations::VariationsHttpHeaderProvider::GetInstance()->ResetForTesting();
 }
 
+TEST_F(FeedbackUploaderChromeTest, TestVariousServerResponses) {
+  content::TestBrowserContext context;
+  FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
+  FeedbackUploaderChrome uploader(
+      &context, FeedbackUploaderFactory::CreateUploaderTaskRunner());
+
+  EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
+  // Successful reports should not introduce any retries, and should not
+  // increase the backoff delay.
+  net::TestURLFetcherFactory factory;
+  factory.set_remove_fetcher_on_delete(true);
+  uploader.QueueReport("Successful report");
+  net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
+  fetcher->set_response_code(kHttpPostSuccessNoContent);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
+  EXPECT_TRUE(uploader.QueueEmpty());
+
+  // Failed reports due to client errors are not retried. No backoff delay
+  // should be doubled.
+  uploader.QueueReport("Client error failed report");
+  fetcher = factory.GetFetcherByID(0);
+  fetcher->set_response_code(kHttpPostFailClientError);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
+  EXPECT_TRUE(uploader.QueueEmpty());
+
+  // Failed reports due to server errors are retried.
+  uploader.QueueReport("Server error failed report");
+  fetcher = factory.GetFetcherByID(0);
+  fetcher->set_response_code(kHttpPostFailServerError);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  EXPECT_EQ(kTestRetryDelay * 2, uploader.retry_delay());
+  EXPECT_FALSE(uploader.QueueEmpty());
+}
+
 }  // namespace feedback

components/feedback/feedback_uploader_delegate.cc

@@ -8,26 +8,27 @@
 #include <sstream>
 
 #include "base/logging.h"
+#include "components/feedback/feedback_report.h"
 #include "net/url_request/url_fetcher.h"
 
 namespace feedback {
+
 namespace {
 
-const int kHttpPostSuccessNoContent = 204;
-const int kHttpPostFailNoConnection = -1;
-const int kHttpPostFailClientError = 400;
-const int kHttpPostFailServerError = 500;
+constexpr int kHttpPostSuccessNoContent = 204;
+constexpr int kHttpPostFailNoConnection = -1;
+constexpr int kHttpPostFailClientError = 400;
+constexpr int kHttpPostFailServerError = 500;
 
 }  // namespace
 
 FeedbackUploaderDelegate::FeedbackUploaderDelegate(
-    const std::string& post_body,
+    scoped_refptr<FeedbackReport> pending_report,
     const base::Closure& success_callback,
-    const ReportDataCallback& error_callback)
-        : post_body_(post_body),
-          success_callback_(success_callback),
-          error_callback_(error_callback) {
-}
+    const ReportFailureCallback& error_callback)
+    : pending_report_(pending_report),
+      success_callback_(success_callback),
+      error_callback_(error_callback) {}
 
 FeedbackUploaderDelegate::~FeedbackUploaderDelegate() {}
 
@@ -41,18 +42,27 @@ void FeedbackUploaderDelegate::OnURLFetchComplete(
     error_stream << "Success";
     success_callback_.Run();
   } else {
+    bool should_retry = true;
     // Process the error for debug output
     if (response_code == kHttpPostFailNoConnection) {
       error_stream << "No connection to server.";
-    } else if ((response_code > kHttpPostFailClientError) &&
+    } else if ((response_code >= kHttpPostFailClientError) &&
                (response_code < kHttpPostFailServerError)) {
+      // Client errors mean that the server failed to parse the proto that was
+      // sent, or that some requirements weren't met by the server side
+      // validation, and hence we should NOT retry sending this corrupt report
+      // and give up.
+      should_retry = false;
+
       error_stream << "Client error: HTTP response code " << response_code;
-    } else if (response_code > kHttpPostFailServerError) {
+    } else if (response_code >= kHttpPostFailServerError) {
       error_stream << "Server error: HTTP response code " << response_code;
     } else {
       error_stream << "Unknown error: HTTP response code " << response_code;
     }
-    error_callback_.Run(post_body_);
+
+    if (should_retry)
+      error_callback_.Run(pending_report_);
   }
 
   LOG(WARNING) << "FEEDBACK: Submission to feedback server ("

components/feedback/feedback_uploader_delegate.h

@@ -5,32 +5,34 @@
 #ifndef COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_DELEGATE_H_
 #define COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_DELEGATE_H_
 
-#include <string>
-
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "components/feedback/feedback_uploader.h"
 #include "net/url_request/url_fetcher_delegate.h"
 
 namespace feedback {
 
+using ReportFailureCallback =
+    base::Callback<void(scoped_refptr<FeedbackReport>)>;
+
 // FeedbackUploaderDelegate is a simple HTTP uploader for a feedback report.
 // When finished, it runs the appropriate callback passed in via the
 // constructor, and then deletes itself.
 class FeedbackUploaderDelegate : public net::URLFetcherDelegate {
  public:
-  FeedbackUploaderDelegate(const std::string& post_body,
+  FeedbackUploaderDelegate(scoped_refptr<FeedbackReport> pending_report,
                            const base::Closure& success_callback,
-                           const ReportDataCallback& error_callback);
+                           const ReportFailureCallback& error_callback);
   ~FeedbackUploaderDelegate() override;
 
  private:
   // Overridden from net::URLFetcherDelegate.
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
-  std::string post_body_;
+  scoped_refptr<FeedbackReport> pending_report_;
   base::Closure success_callback_;
-  ReportDataCallback error_callback_;
+  ReportFailureCallback error_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedbackUploaderDelegate);
 };

components/feedback/feedback_uploader_factory.cc

@@ -5,6 +5,9 @@
 #include "components/feedback/feedback_uploader_factory.h"
 
 #include "base/memory/singleton.h"
+#include "base/single_thread_task_runner.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/task_scheduler/task_traits.h"
 #include "components/feedback/feedback_uploader.h"
 #include "components/feedback/feedback_uploader_chrome.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -23,16 +26,27 @@ FeedbackUploader* FeedbackUploaderFactory::GetForBrowserContext(
       GetInstance()->GetServiceForBrowserContext(context, true));
 }
 
+// static
+scoped_refptr<base::SingleThreadTaskRunner>
+FeedbackUploaderFactory::CreateUploaderTaskRunner() {
+  // Uses a BLOCK_SHUTDOWN file task runner because we really don't want to
+  // lose reports or corrupt their files.
+  return base::CreateSingleThreadTaskRunnerWithTraits(
+      {base::MayBlock(), base::TaskPriority::BACKGROUND,
+       base::TaskShutdownBehavior::BLOCK_SHUTDOWN});
+}
+
 FeedbackUploaderFactory::FeedbackUploaderFactory()
     : BrowserContextKeyedServiceFactory(
           "feedback::FeedbackUploader",
-          BrowserContextDependencyManager::GetInstance()) {}
+          BrowserContextDependencyManager::GetInstance()),
+      task_runner_(CreateUploaderTaskRunner()) {}
 
 FeedbackUploaderFactory::~FeedbackUploaderFactory() {}
 
 KeyedService* FeedbackUploaderFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new FeedbackUploaderChrome(context);
+  return new FeedbackUploaderChrome(context, task_runner_);
 }
 
 content::BrowserContext* FeedbackUploaderFactory::GetBrowserContextToUse(

components/feedback/feedback_uploader_factory.h

@@ -6,10 +6,12 @@
 #define COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_FACTORY_H_
 
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 
 namespace base {
 template<typename T> struct DefaultSingletonTraits;
+class SingleThreadTaskRunner;
 }
 
 namespace content {
@@ -30,6 +32,11 @@ class FeedbackUploaderFactory : public BrowserContextKeyedServiceFactory {
   static FeedbackUploader* GetForBrowserContext(
       content::BrowserContext* context);
 
+  // Creates a new SingleThreadTaskRunner that is used to run feedback blocking
+  // background work. Tests can use this to create the exact same type of runner
+  // that's actually used in production code to simulate the same behavior.
+  static scoped_refptr<base::SingleThreadTaskRunner> CreateUploaderTaskRunner();
+
  private:
   friend struct base::DefaultSingletonTraits<FeedbackUploaderFactory>;
 
@@ -42,6 +49,11 @@ class FeedbackUploaderFactory : public BrowserContextKeyedServiceFactory {
   content::BrowserContext* GetBrowserContextToUse(
       content::BrowserContext* context) const override;
 
+  // The task runner used to handle all blocking background feedback-reports
+  // work. It involves reading / writing reports from / to disk. Those
+  // operations must not interleave and thread affinity is required.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(FeedbackUploaderFactory);
 };
 

components/feedback/feedback_uploader_unittest.cc

@@ -14,51 +14,103 @@
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "build/build_config.h"
+#include "components/feedback/feedback_report.h"
 #include "components/feedback/feedback_uploader_chrome.h"
 #include "components/feedback/feedback_uploader_factory.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/user_prefs/user_prefs.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace feedback {
+
 namespace {
 
-const char kReportOne[] = "one";
-const char kReportTwo[] = "two";
-const char kReportThree[] = "three";
-const char kReportFour[] = "four";
-const char kReportFive[] = "five";
+constexpr char kReportOne[] = "one";
+constexpr char kReportTwo[] = "two";
+constexpr char kReportThree[] = "three";
+constexpr char kReportFour[] = "four";
+constexpr char kReportFive[] = "five";
 
-const base::TimeDelta kRetryDelayForTest =
+constexpr base::TimeDelta kRetryDelayForTest =
     base::TimeDelta::FromMilliseconds(100);
 
+class MockFeedbackUploader : public FeedbackUploaderChrome {
+ public:
+  MockFeedbackUploader(content::BrowserContext* context,
+                       scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : FeedbackUploaderChrome(context, task_runner) {}
+  ~MockFeedbackUploader() override {}
+
+  void RunMessageLoop() {
+    if (ProcessingComplete())
+      return;
+    run_loop_ = base::MakeUnique<base::RunLoop>();
+    run_loop_->Run();
+  }
+
+  const std::map<std::string, unsigned int>& dispatched_reports() const {
+    return dispatched_reports_;
+  }
+  void set_expected_reports(size_t value) { expected_reports_ = value; }
+  void set_simulate_failure(bool value) { simulate_failure_ = value; }
+
+ private:
+  // FeedbackUploaderChrome:
+  void DispatchReport(scoped_refptr<FeedbackReport> report) override {
+    if (base::ContainsKey(dispatched_reports_, report->data()))
+      dispatched_reports_[report->data()]++;
+    else
+      dispatched_reports_[report->data()] = 1;
+
+    dispatched_reports_count_++;
+
+    if (simulate_failure_)
+      OnReportUploadFailure(report);
+    else
+      OnReportUploadSuccess();
+
+    if (ProcessingComplete()) {
+      if (run_loop_)
+        run_loop_->Quit();
+    }
+  }
+
+  bool ProcessingComplete() {
+    return (dispatched_reports_count_ >= expected_reports_);
+  }
+
+  std::unique_ptr<base::RunLoop> run_loop_;
+  std::map<std::string, unsigned int> dispatched_reports_;
+  size_t dispatched_reports_count_ = 0;
+  size_t expected_reports_ = 0;
+  bool simulate_failure_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(MockFeedbackUploader);
+};
+
 std::unique_ptr<KeyedService> CreateFeedbackUploaderService(
     content::BrowserContext* context) {
-  return base::MakeUnique<feedback::FeedbackUploaderChrome>(context);
+  return base::MakeUnique<MockFeedbackUploader>(
+      context, FeedbackUploaderFactory::CreateUploaderTaskRunner());
 }
 
 }  // namespace
 
-namespace feedback {
-
 class FeedbackUploaderTest : public testing::Test {
- protected:
+ public:
   FeedbackUploaderTest()
-      : context_(new content::TestBrowserContext()),
-        prefs_(new sync_preferences::TestingPrefServiceSyncable()),
-        dispatched_reports_count_(0),
-        expected_reports_(0) {
+      : context_(base::MakeUnique<content::TestBrowserContext>()),
+        prefs_(
+            base::MakeUnique<sync_preferences::TestingPrefServiceSyncable>()) {
     user_prefs::UserPrefs::Set(context_.get(), prefs_.get());
     FeedbackUploaderFactory::GetInstance()->SetTestingFactory(
         context_.get(), &CreateFeedbackUploaderService);
 
-    uploader_ = FeedbackUploaderFactory::GetForBrowserContext(context_.get());
-    uploader_->setup_for_test(
-        base::Bind(&FeedbackUploaderTest::MockDispatchReport,
-                   base::Unretained(this)),
-        kRetryDelayForTest);
+    FeedbackUploader::SetMinimumRetryDelayForTesting(kRetryDelayForTest);
+    uploader_ = static_cast<MockFeedbackUploader*>(
+        FeedbackUploaderFactory::GetForBrowserContext(context_.get()));
   }
 
   ~FeedbackUploaderTest() override {
@@ -70,98 +122,60 @@ class FeedbackUploaderTest : public testing::Test {
     uploader_->QueueReport(data);
   }
 
-  void ReportFailure(const std::string& data) {
-    uploader_->RetryReport(data);
-  }
-
-  void MockDispatchReport(const std::string& report_data) {
-    if (base::ContainsKey(dispatched_reports_, report_data)) {
-      dispatched_reports_[report_data]++;
-    } else {
-      dispatched_reports_[report_data] = 1;
-    }
-    dispatched_reports_count_++;
-
-    // Dispatch will always update the timer, whether successful or not,
-    // simulate the same behavior.
-    uploader_->UpdateUploadTimer();
-
-    if (ProcessingComplete()) {
-      if (run_loop_.get())
-        run_loop_->Quit();
-    }
-  }
-
-  bool ProcessingComplete() {
-    return (dispatched_reports_count_ >= expected_reports_);
-  }
-
-  void RunMessageLoop() {
-    if (ProcessingComplete())
-      return;
-    run_loop_.reset(new base::RunLoop());
-    run_loop_->Run();
-  }
+  MockFeedbackUploader* uploader() const { return uploader_; }
 
+ private:
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
-  std::unique_ptr<base::RunLoop> run_loop_;
   std::unique_ptr<content::TestBrowserContext> context_;
   std::unique_ptr<PrefService> prefs_;
 
-  FeedbackUploader* uploader_;
+  MockFeedbackUploader* uploader_;
 
-  std::map<std::string, unsigned int> dispatched_reports_;
-  size_t dispatched_reports_count_;
-  size_t expected_reports_;
+  DISALLOW_COPY_AND_ASSIGN(FeedbackUploaderTest);
 };
 
-#if defined(OS_LINUX) || defined(OS_MACOSX)
-#define MAYBE_QueueMultiple QueueMultiple
-#else
-// crbug.com/330547
-#define MAYBE_QueueMultiple DISABLED_QueueMultiple
-#endif
-TEST_F(FeedbackUploaderTest, MAYBE_QueueMultiple) {
-  dispatched_reports_.clear();
+TEST_F(FeedbackUploaderTest, QueueMultiple) {
   QueueReport(kReportOne);
   QueueReport(kReportTwo);
   QueueReport(kReportThree);
   QueueReport(kReportFour);
 
-  EXPECT_EQ(dispatched_reports_.size(), 4u);
-  EXPECT_EQ(dispatched_reports_[kReportOne], 1u);
-  EXPECT_EQ(dispatched_reports_[kReportTwo], 1u);
-  EXPECT_EQ(dispatched_reports_[kReportThree], 1u);
-  EXPECT_EQ(dispatched_reports_[kReportFour], 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().size(), 4u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportOne), 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportTwo), 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportThree), 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportFour), 1u);
 }
 
-#if defined(OS_WIN) || defined(OS_ANDROID)
-// crbug.com/330547
-#define MAYBE_QueueMultipleWithFailures DISABLED_QueueMultipleWithFailures
-#else
-#define MAYBE_QueueMultipleWithFailures QueueMultipleWithFailures
-#endif
-TEST_F(FeedbackUploaderTest, MAYBE_QueueMultipleWithFailures) {
-  dispatched_reports_.clear();
-
+TEST_F(FeedbackUploaderTest, QueueMultipleWithFailures) {
+  EXPECT_EQ(kRetryDelayForTest, uploader()->retry_delay());
   QueueReport(kReportOne);
+
+  // Simulate a failure in reports two and three. Make sure the backoff delay
+  // will be applied twice, and the reports will eventually be sent.
+  uploader()->set_simulate_failure(true);
   QueueReport(kReportTwo);
+  EXPECT_EQ(kRetryDelayForTest * 2, uploader()->retry_delay());
   QueueReport(kReportThree);
-  QueueReport(kReportFour);
+  EXPECT_EQ(kRetryDelayForTest * 4, uploader()->retry_delay());
+  uploader()->set_simulate_failure(false);
 
-  ReportFailure(kReportThree);
-  ReportFailure(kReportTwo);
+  // Once a successful report is sent, the backoff delay is reset back to its
+  // minimum value.
+  QueueReport(kReportFour);
+  EXPECT_EQ(kRetryDelayForTest, uploader()->retry_delay());
   QueueReport(kReportFive);
 
-  expected_reports_ = 7;
-  RunMessageLoop();
+  // Wait for the pending two failed reports to be sent.
+  uploader()->set_expected_reports(7);
+  uploader()->RunMessageLoop();
 
-  EXPECT_EQ(dispatched_reports_.size(), 5u);
-  EXPECT_EQ(dispatched_reports_[kReportOne], 1u);
-  EXPECT_EQ(dispatched_reports_[kReportTwo], 2u);
-  EXPECT_EQ(dispatched_reports_[kReportThree], 2u);
-  EXPECT_EQ(dispatched_reports_[kReportFour], 1u);
-  EXPECT_EQ(dispatched_reports_[kReportFive], 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().size(), 5u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportOne), 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportTwo), 2u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportThree), 2u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportFour), 1u);
+  EXPECT_EQ(uploader()->dispatched_reports().at(kReportFive), 1u);
 }
 
 }  // namespace feedback