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_data_unittest.cc b/components/feedback/feedback_data_unittest.cc
index 443d3b5..5171b75 100644
--- a/components/feedback/feedback_data_unittest.cc
+++ b/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 @@
} // namespace
-namespace feedback {
-
class FeedbackDataTest : public testing::Test {
protected:
FeedbackDataTest()
diff --git a/components/feedback/feedback_report.cc b/components/feedback/feedback_report.cc
index bdbd469..48f825ec6 100644
--- a/components/feedback/feedback_report.cc
+++ b/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,
diff --git a/components/feedback/feedback_report.h b/components/feedback/feedback_report.h
index 11826c26..d3fdb8d 100644
--- a/components/feedback/feedback_report.h
+++ b/components/feedback/feedback_report.h
@@ -45,6 +45,7 @@
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:
diff --git a/components/feedback/feedback_uploader.cc b/components/feedback/feedback_uploader.cc
index 7f8b796..a17c9a7df 100644
--- a/components/feedback/feedback_uploader.cc
+++ b/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");
+// 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);
+}
+
} // namespace
+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() {}
+
+// 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();
}
-FeedbackUploader::FeedbackUploader(const base::FilePath& path)
- : report_path_(path.Append(kFeedbackReportPath)),
- retry_delay_(base::TimeDelta::FromMinutes(kRetryDelayMinutes)),
- url_(kFeedbackPostUrl) {
- Init();
-}
-
-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() {}
-
-void FeedbackUploader::Init() {
- dispatch_callback_ = base::Bind(&FeedbackUploader::DispatchReport,
- AsWeakPtr());
-}
-
-void FeedbackUploader::QueueReport(const std::string& data) {
- QueueReportWithDelay(data, base::TimeDelta());
-}
-
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
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);
};
diff --git a/components/feedback/feedback_uploader_chrome.cc b/components/feedback/feedback_uploader_chrome.cc
index 70c16c0..77a6af4 100644
--- a/components/feedback/feedback_uploader_chrome.cc
+++ b/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 @@
} // 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 @@
// 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 @@
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());
diff --git a/components/feedback/feedback_uploader_chrome.h b/components/feedback/feedback_uploader_chrome.h
index f2579b6..21cf138 100644
--- a/components/feedback/feedback_uploader_chrome.h
+++ b/components/feedback/feedback_uploader_chrome.h
@@ -18,11 +18,14 @@
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_;
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
diff --git a/components/feedback/feedback_uploader_delegate.cc b/components/feedback/feedback_uploader_delegate.cc
index 0519a8164..3f0c353 100644
--- a/components/feedback/feedback_uploader_delegate.cc
+++ b/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 @@
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 ("
diff --git a/components/feedback/feedback_uploader_delegate.h b/components/feedback/feedback_uploader_delegate.h
index b51de39..ca858ebb 100644
--- a/components/feedback/feedback_uploader_delegate.h
+++ b/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);
};
diff --git a/components/feedback/feedback_uploader_factory.cc b/components/feedback/feedback_uploader_factory.cc
index 111d893..59023cc 100644
--- a/components/feedback/feedback_uploader_factory.cc
+++ b/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 @@
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(
diff --git a/components/feedback/feedback_uploader_factory.h b/components/feedback/feedback_uploader_factory.h
index effad010..b04df4a 100644
--- a/components/feedback/feedback_uploader_factory.h
+++ b/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 @@
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 @@
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);
};
diff --git a/components/feedback/feedback_uploader_unittest.cc b/components/feedback/feedback_uploader_unittest.cc
index 76201f3b..5158478 100644
--- a/components/feedback/feedback_uploader_unittest.cc
+++ b/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 @@
uploader_->QueueReport(data);
}
- void ReportFailure(const std::string& data) {
- uploader_->RetryReport(data);
- }
+ MockFeedbackUploader* uploader() const { return uploader_; }
- 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();
- }
-
+ 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);
- QueueReport(kReportTwo);
- QueueReport(kReportThree);
- QueueReport(kReportFour);
- ReportFailure(kReportThree);
- ReportFailure(kReportTwo);
+ // 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);
+ EXPECT_EQ(kRetryDelayForTest * 4, uploader()->retry_delay());
+ uploader()->set_simulate_failure(false);
+
+ // 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
