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/564304
Reviewed-by: Rahul Chaturvedi <[email protected]>
Commit-Queue: Ahmed Fakhry <[email protected]>
Cr-Commit-Position: refs/heads/master@{#488758}diff --git a/components/feedback/feedback_uploader.h b/components/feedback/feedback_uploader.h
index 1b057273..eaed704d 100644
--- a/components/feedback/feedback_uploader.h
+++ b/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 @@
// 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 @@
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);
};
