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_chrome_unittest.cc b/components/feedback/feedback_uploader_chrome_unittest.cc
index 7db4687..0f631b1 100644
--- a/components/feedback/feedback_uploader_chrome_unittest.cc
+++ b/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 @@
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 @@
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
