Set `task_runner_' in WebRtcEventLogUploaderImpl

This makes it clearer that WebRtcEventLogUploaderImpl runs on the
same task runner as WebRtcRemoteEventLogManager, and makes following
the threading issues easier.

Bug: 1092071
Change-Id: I0a72973525a03632b423dc69afc495439dfdaed5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2260568
Commit-Queue: Elad Alon <eladalon@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781877}

chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc

@@ -203,7 +203,7 @@ WebRtcRemoteEventLogManager::WebRtcRemoteEventLogManager(
       uploading_supported_for_connection_type_(false),
       scheduled_upload_tasks_(0),
       uploader_factory_(
-          std::make_unique<WebRtcEventLogUploaderImpl::Factory>()),
+          std::make_unique<WebRtcEventLogUploaderImpl::Factory>(task_runner)),
       task_runner_(task_runner),
       weak_ptr_factory_(
           std::make_unique<base::WeakPtrFactory<WebRtcRemoteEventLogManager>>(

chrome/browser/media/webrtc/webrtc_event_log_uploader.cc

@@ -8,7 +8,6 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
-#include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "components/version_info/version_info.h"
@@ -124,11 +123,17 @@ void OnURLLoadUploadProgress(uint64_t current, uint64_t total) {
 const char WebRtcEventLogUploaderImpl::kUploadURL[] =
     "https://clients2.google.com/cr/report";
 
+WebRtcEventLogUploaderImpl::Factory::Factory(
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(std::move(task_runner)) {}
+
+WebRtcEventLogUploaderImpl::Factory::~Factory() = default;
+
 std::unique_ptr<WebRtcEventLogUploader>
 WebRtcEventLogUploaderImpl::Factory::Create(const WebRtcLogFileInfo& log_file,
                                             UploadResultCallback callback) {
   return std::make_unique<WebRtcEventLogUploaderImpl>(
-      log_file, std::move(callback), kMaxRemoteLogFileSizeBytes);
+      task_runner_, log_file, std::move(callback), kMaxRemoteLogFileSizeBytes);
 }
 
 std::unique_ptr<WebRtcEventLogUploader>
@@ -137,17 +142,18 @@ WebRtcEventLogUploaderImpl::Factory::CreateWithCustomMaxSizeForTesting(
     UploadResultCallback callback,
     size_t max_log_file_size_bytes) {
   return std::make_unique<WebRtcEventLogUploaderImpl>(
-      log_file, std::move(callback), max_log_file_size_bytes);
+      task_runner_, log_file, std::move(callback), max_log_file_size_bytes);
 }
 
 WebRtcEventLogUploaderImpl::WebRtcEventLogUploaderImpl(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
     const WebRtcLogFileInfo& log_file,
     UploadResultCallback callback,
     size_t max_log_file_size_bytes)
-    : log_file_(log_file),
+    : task_runner_(std::move(task_runner)),
+      log_file_(log_file),
       callback_(std::move(callback)),
-      max_log_file_size_bytes_(max_log_file_size_bytes),
-      io_task_runner_(base::SequencedTaskRunnerHandle::Get()) {
+      max_log_file_size_bytes_(max_log_file_size_bytes) {
   history_file_writer_ = WebRtcEventLogHistoryFileWriter::Create(
       GetWebRtcEventLogHistoryFilePath(log_file_.path));
   if (!history_file_writer_) {
@@ -189,12 +195,12 @@ WebRtcEventLogUploaderImpl::~WebRtcEventLogUploaderImpl() {
   // 3. The upload was never started, due to an early failure (e.g. file not
   //    found). In that case, |url_loader_| will not have been set.
   // 4. Chrome shutdown.
-  if (io_task_runner_->RunsTasksInCurrentSequence()) {  // Scenarios 1-3.
+  if (task_runner_->RunsTasksInCurrentSequence()) {  // Scenarios 1-3.
     DCHECK(!url_loader_);
   } else {  // # Scenario #4 - Chrome shutdown.
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     bool will_delete =
-        io_task_runner_->DeleteSoon(FROM_HERE, url_loader_.release());
+        task_runner_->DeleteSoon(FROM_HERE, url_loader_.release());
     DCHECK(!will_delete)
         << "Task runners must have been stopped by this stage of shutdown.";
   }
@@ -202,12 +208,12 @@ WebRtcEventLogUploaderImpl::~WebRtcEventLogUploaderImpl() {
 
 const WebRtcLogFileInfo& WebRtcEventLogUploaderImpl::GetWebRtcLogFileInfo()
     const {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
   return log_file_;
 }
 
 bool WebRtcEventLogUploaderImpl::Cancel() {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   // The upload could already have been completed, or maybe was never properly
   // started (due to a file read failure, etc.).
@@ -230,7 +236,7 @@ bool WebRtcEventLogUploaderImpl::Cancel() {
 }
 
 bool WebRtcEventLogUploaderImpl::PrepareUploadData(std::string* upload_data) {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   std::string log_file_contents;
   if (!base::ReadFileToStringWithMaxSize(log_file_.path, &log_file_contents,
@@ -270,7 +276,7 @@ bool WebRtcEventLogUploaderImpl::PrepareUploadData(std::string* upload_data) {
 }
 
 void WebRtcEventLogUploaderImpl::StartUpload(const std::string& upload_data) {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   auto resource_request = std::make_unique<network::ResourceRequest>();
   resource_request->url = GURL(kUploadURL);
@@ -303,7 +309,7 @@ void WebRtcEventLogUploaderImpl::StartUpload(const std::string& upload_data) {
 
 void WebRtcEventLogUploaderImpl::OnURLLoadComplete(
     std::unique_ptr<std::string> response_body) {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
   DCHECK(url_loader_);
 
   if (response_body.get() != nullptr && response_body->empty()) {
@@ -341,7 +347,7 @@ void WebRtcEventLogUploaderImpl::OnURLLoadComplete(
 }
 
 void WebRtcEventLogUploaderImpl::ReportResult(bool result) {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   // * If the upload was successful, the file is no longer needed.
   // * If the upload failed, we don't want to retry, because we run the risk of
@@ -355,12 +361,12 @@ void WebRtcEventLogUploaderImpl::ReportResult(bool result) {
   // Release hold of history file, allowing it to be read, moved or deleted.
   history_file_writer_.reset();
 
-  io_task_runner_->PostTask(
+  task_runner_->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback_), log_file_.path, result));
 }
 
 void WebRtcEventLogUploaderImpl::DeleteLogFile() {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
   const bool deletion_successful =
       base::DeleteFile(log_file_.path, /*recursive=*/false);
   if (!deletion_successful) {
@@ -371,7 +377,7 @@ void WebRtcEventLogUploaderImpl::DeleteLogFile() {
 }
 
 void WebRtcEventLogUploaderImpl::DeleteHistoryFile() {
-  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
   if (!history_file_writer_) {
     LOG(ERROR) << "Deletion of history file attempted after uploader "
                << "has relinquished ownership of it.";

chrome/browser/media/webrtc/webrtc_event_log_uploader.h

@@ -67,7 +67,9 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
  public:
   class Factory : public WebRtcEventLogUploader::Factory {
    public:
-    ~Factory() override = default;
+    explicit Factory(scoped_refptr<base::SequencedTaskRunner> task_runner);
+
+    ~Factory() override;
 
     std::unique_ptr<WebRtcEventLogUploader> Create(
         const WebRtcLogFileInfo& log_file,
@@ -80,9 +82,13 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
         const WebRtcLogFileInfo& log_file,
         UploadResultCallback callback,
         size_t max_remote_log_file_size_bytes);
+
+   private:
+    scoped_refptr<base::SequencedTaskRunner> task_runner_;
   };
 
   WebRtcEventLogUploaderImpl(
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
       const WebRtcLogFileInfo& log_file,
       UploadResultCallback callback,
       size_t max_remote_log_file_size_bytes);
@@ -121,6 +127,9 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
   // The URL used for uploading the logs.
   static const char kUploadURL[];
 
+  // The object lives on this IO-capable task runner.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   // Housekeeping information about the uploaded file (path, time of last
   // modification, associated BrowserContext).
   const WebRtcLogFileInfo log_file_;
@@ -138,9 +147,6 @@ class WebRtcEventLogUploaderImpl : public WebRtcEventLogUploader {
 
   // This object is in charge of the actual upload.
   std::unique_ptr<network::SimpleURLLoader> url_loader_;
-
-  // The object lives on this IO-capable task runner.
-  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
 };
 
 }  // namespace webrtc_event_logging

chrome/browser/media/webrtc/webrtc_event_log_uploader_impl_unittest.cc

@@ -81,7 +81,8 @@ class WebRtcEventLogUploaderImplTest : public ::testing::Test {
 
     EXPECT_TRUE(base::Time::FromString("30 Dec 1983", &kReasonableTime));
 
-    uploader_factory_ = std::make_unique<WebRtcEventLogUploaderImpl::Factory>();
+    uploader_factory_ = std::make_unique<WebRtcEventLogUploaderImpl::Factory>(
+        base::SequencedTaskRunnerHandle::Get());
   }
 
   ~WebRtcEventLogUploaderImplTest() override {