Fix data race in browsing_data_remover_test_util

If browser_tests is built with tsan enabled, Thread Sanitizer reports
data races in browsing_data_remover_test_util.

There are two threads to concurrently access the shared data, the
test’s main thread and a thread controlled by TaskScheduler singleton.
Both access browsing_data_remover_done_ and/or
flush_for_testing_complete_ fields to break run loop and exit test
after some cleanup occurs. In TaskScheduler’s thread, the write access
is done by the lambda function.

The proposed solution is to use the lambda call only to report the
asynchronous event to main thread, which becomes the only one to
access the fields implementing run loop break logic.

R=sky@chromium.org

Bug: 885188
Change-Id: I706edb24ed362444cf46e558b1d54389179884da
Reviewed-on: https://chromium-review.googlesource.com/1233613Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593987}

content/public/test/browsing_data_remover_test_util.cc

@@ -6,12 +6,14 @@
 
 #include "base/bind.h"
 #include "base/task/task_scheduler/task_scheduler.h"
+#include "base/threading/thread_task_runner_handle.h"
 
 namespace content {
 
 BrowsingDataRemoverCompletionObserver::BrowsingDataRemoverCompletionObserver(
     BrowsingDataRemover* remover)
-    : observer_(this) {
+    : observer_(this),
+      origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
   observer_.Add(remover);
 }
 
@@ -20,10 +22,7 @@ BrowsingDataRemoverCompletionObserver::
 
 void BrowsingDataRemoverCompletionObserver::BlockUntilCompletion() {
   base::TaskScheduler::GetInstance()->FlushAsyncForTesting(base::BindOnce(
-      [](BrowsingDataRemoverCompletionObserver* observer) {
-        observer->flush_for_testing_complete_ = true;
-        observer->QuitRunLoopWhenTasksComplete();
-      },
+      &BrowsingDataRemoverCompletionObserver::FlushForTestingComplete,
       base::Unretained(this)));
   run_loop_.Run();
 }
@@ -34,6 +33,19 @@ void BrowsingDataRemoverCompletionObserver::OnBrowsingDataRemoverDone() {
   QuitRunLoopWhenTasksComplete();
 }
 
+void BrowsingDataRemoverCompletionObserver::FlushForTestingComplete() {
+  if (origin_task_runner_->RunsTasksInCurrentSequence()) {
+    flush_for_testing_complete_ = true;
+    QuitRunLoopWhenTasksComplete();
+    return;
+  }
+  origin_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &BrowsingDataRemoverCompletionObserver::FlushForTestingComplete,
+          base::Unretained(this)));
+}
+
 void BrowsingDataRemoverCompletionObserver::QuitRunLoopWhenTasksComplete() {
   if (!flush_for_testing_complete_ || !browsing_data_remover_done_)
     return;
@@ -43,7 +55,9 @@ void BrowsingDataRemoverCompletionObserver::QuitRunLoopWhenTasksComplete() {
 
 BrowsingDataRemoverCompletionInhibitor::BrowsingDataRemoverCompletionInhibitor(
     BrowsingDataRemover* remover)
-    : remover_(remover), run_loop_(new base::RunLoop) {
+    : remover_(remover),
+      run_loop_(new base::RunLoop),
+      origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
   DCHECK(remover);
   remover_->SetWouldCompleteCallbackForTesting(
       base::Bind(&BrowsingDataRemoverCompletionInhibitor::
@@ -66,10 +80,7 @@ void BrowsingDataRemoverCompletionInhibitor::Reset() {
 
 void BrowsingDataRemoverCompletionInhibitor::BlockUntilNearCompletion() {
   base::TaskScheduler::GetInstance()->FlushAsyncForTesting(base::BindOnce(
-      [](BrowsingDataRemoverCompletionInhibitor* inhibitor) {
-        inhibitor->flush_for_testing_complete_ = true;
-        inhibitor->QuitRunLoopWhenTasksComplete();
-      },
+      &BrowsingDataRemoverCompletionInhibitor::FlushForTestingComplete,
       base::Unretained(this)));
   run_loop_->Run();
   run_loop_ = std::make_unique<base::RunLoop>();
@@ -91,6 +102,19 @@ void BrowsingDataRemoverCompletionInhibitor::OnBrowsingDataRemoverWouldComplete(
   QuitRunLoopWhenTasksComplete();
 }
 
+void BrowsingDataRemoverCompletionInhibitor::FlushForTestingComplete() {
+  if (origin_task_runner_->RunsTasksInCurrentSequence()) {
+    flush_for_testing_complete_ = true;
+    QuitRunLoopWhenTasksComplete();
+    return;
+  }
+  origin_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &BrowsingDataRemoverCompletionInhibitor::FlushForTestingComplete,
+          base::Unretained(this)));
+}
+
 void BrowsingDataRemoverCompletionInhibitor::QuitRunLoopWhenTasksComplete() {
   if (!flush_for_testing_complete_ ||
       !browsing_data_remover_would_complete_done_) {

content/public/test/browsing_data_remover_test_util.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/scoped_observer.h"
+#include "base/sequenced_task_runner.h"
 #include "content/public/browser/browsing_data_remover.h"
 
 namespace content {
@@ -29,6 +30,7 @@ class BrowsingDataRemoverCompletionObserver
   void OnBrowsingDataRemoverDone() override;
 
  private:
+  void FlushForTestingComplete();
   void QuitRunLoopWhenTasksComplete();
 
   // Tracks when the Task Scheduler task flushing is done.
@@ -40,6 +42,7 @@ class BrowsingDataRemoverCompletionObserver
 
   base::RunLoop run_loop_;
   ScopedObserver<BrowsingDataRemover, BrowsingDataRemover::Observer> observer_;
+  scoped_refptr<base::SequencedTaskRunner> origin_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataRemoverCompletionObserver);
 };
@@ -66,6 +69,7 @@ class BrowsingDataRemoverCompletionInhibitor {
       const base::Closure& continue_to_completion);
 
  private:
+  void FlushForTestingComplete();
   void QuitRunLoopWhenTasksComplete();
 
   // Tracks when the Task Scheduler task flushing is done.
@@ -80,6 +84,7 @@ class BrowsingDataRemoverCompletionInhibitor {
 
   std::unique_ptr<base::RunLoop> run_loop_;
   base::Closure continue_to_completion_callback_;
+  scoped_refptr<base::SequencedTaskRunner> origin_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataRemoverCompletionInhibitor);
 };