Fix URLFetcherTest.SequencedTaskTest to run things on the Sequence.

This test previously used the WaitingURLFetcherDelegate helper's APIs
to run a RunLoop on the main thread, and wait for it to be quit by a
fetch completing on the Sequence. However, this violated the constraint
that the RunLoop may only be created, run and destroyed from the same
thread.

Bug: 729716
Change-Id: I4b699d17da7888527ef8d22f8e73a7f91236ef23
Reviewed-on: https://chromium-review.googlesource.com/1056265
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558344}

net/url_request/url_fetcher_impl_unittest.cc

@@ -21,13 +21,14 @@
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
-#include "base/single_thread_task_runner.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ -91,6 +92,10 @@ class WaitingURLFetcherDelegate : public URLFetcherDelegate {
       const GURL& url,
       URLFetcher::RequestType request_type,
       scoped_refptr<net::URLRequestContextGetter> context_getter) {
+    if (!on_complete_or_cancel_) {
+      run_loop_ = std::make_unique<base::RunLoop>();
+      on_complete_or_cancel_ = run_loop_->QuitClosure();
+    }
     fetcher_.reset(new URLFetcherImpl(url, request_type, this,
                                       TRAFFIC_ANNOTATION_FOR_TESTS));
     fetcher_->SetRequestContext(context_getter.get());
@@ -107,15 +112,15 @@ class WaitingURLFetcherDelegate : public URLFetcherDelegate {
   // Wait until the request has completed or been canceled. Does not start the
   // request.
   void WaitForComplete() {
-    EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
-    run_loop_.Run();
+    EXPECT_TRUE(task_runner_->RunsTasksInCurrentSequence());
+    run_loop_->Run();
   }
 
   // Cancels the fetch by deleting the fetcher.
   void CancelFetch() {
     EXPECT_TRUE(fetcher_);
     fetcher_.reset();
-    task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
+    std::move(on_complete_or_cancel_).Run();
   }
 
   // URLFetcherDelegate:
@@ -124,7 +129,7 @@ class WaitingURLFetcherDelegate : public URLFetcherDelegate {
     EXPECT_TRUE(fetcher_);
     EXPECT_EQ(fetcher_.get(), source);
     did_complete_ = true;
-    task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure());
+    std::move(on_complete_or_cancel_).Run();
   }
 
   void OnURLFetchDownloadProgress(const URLFetcher* source,
@@ -160,13 +165,18 @@ class WaitingURLFetcherDelegate : public URLFetcherDelegate {
 
   bool did_complete() const { return did_complete_; }
 
+  void set_on_complete_or_cancel_closure(base::OnceClosure closure) {
+    on_complete_or_cancel_ = std::move(closure);
+  }
+
  private:
   bool did_complete_;
 
   std::unique_ptr<URLFetcherImpl> fetcher_;
-  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_ =
-      base::ThreadTaskRunnerHandle::Get();
-  base::RunLoop run_loop_;
+  const scoped_refptr<base::SequencedTaskRunner> task_runner_ =
+      base::SequencedTaskRunnerHandle::Get();
+  std::unique_ptr<base::RunLoop> run_loop_;
+  base::OnceClosure on_complete_or_cancel_;
 
   DISALLOW_COPY_AND_ASSIGN(WaitingURLFetcherDelegate);
 };
@@ -210,9 +220,8 @@ class FetcherTestURLRequestContextGetter : public URLRequestContextGetter {
         shutting_down_(false) {}
 
   // Sets callback to be invoked when the getter is destroyed.
-  void set_on_destruction_callback(
-      const base::Closure& on_destruction_callback) {
-    on_destruction_callback_ = on_destruction_callback;
+  void set_on_destruction_callback(base::OnceClosure on_destruction_callback) {
+    on_destruction_callback_ = std::move(on_destruction_callback);
   }
 
   // URLRequestContextGetter:
@@ -310,7 +319,7 @@ class FetcherTestURLRequestContextGetter : public URLRequestContextGetter {
     // the parent class already ensures it's deleted on the network thread.
     DCHECK(network_task_runner_->BelongsToCurrentThread());
     if (!on_destruction_callback_.is_null())
-      on_destruction_callback_.Run();
+      std::move(on_destruction_callback_).Run();
   }
 
  private:
@@ -323,7 +332,7 @@ class FetcherTestURLRequestContextGetter : public URLRequestContextGetter {
   std::unique_ptr<FetcherTestURLRequestContext> context_;
   bool shutting_down_;
 
-  base::Closure on_destruction_callback_;
+  base::OnceClosure on_destruction_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FetcherTestURLRequestContextGetter);
 };
@@ -398,12 +407,12 @@ class URLFetcherTest : public TestWithScopedTaskEnvironment {
         URLFetcher::GET, CreateSameThreadContextGetter());
     if (save_to_temporary_file) {
       delegate->fetcher()->SaveResponseToTemporaryFile(
-          scoped_refptr<base::SingleThreadTaskRunner>(
-              base::ThreadTaskRunnerHandle::Get()));
+          scoped_refptr<base::SequencedTaskRunner>(
+              base::SequencedTaskRunnerHandle::Get()));
     } else {
       delegate->fetcher()->SaveResponseToFileAtPath(
-          requested_out_path, scoped_refptr<base::SingleThreadTaskRunner>(
-                                  base::ThreadTaskRunnerHandle::Get()));
+          requested_out_path, scoped_refptr<base::SequencedTaskRunner>(
+                                  base::SequencedTaskRunnerHandle::Get()));
     }
     delegate->StartFetcherAndWait();
 
@@ -559,26 +568,48 @@ TEST_F(URLFetcherTest, DifferentThreadsTest) {
   EXPECT_EQ(kDefaultResponseBody, data);
 }
 
-// Create the fetcher from a sequenced (not single-threaded) task. Verify that
-// the expected response is received.
+// Verifies that a URLFetcher works correctly on a TaskScheduler Sequence.
 TEST_F(URLFetcherTest, SequencedTaskTest) {
   auto sequenced_task_runner = base::CreateSequencedTaskRunnerWithTraits({});
-  auto delegate = std::make_unique<WaitingURLFetcherDelegate>();
-  sequenced_task_runner->PostTask(
-      FROM_HERE, base::Bind(&WaitingURLFetcherDelegate::CreateFetcher,
-                            base::Unretained(delegate.get()),
-                            test_server_->GetURL(kDefaultResponsePath),
-                            URLFetcher::GET, CreateCrossThreadContextGetter()));
-  RunUntilIdle();
-  delegate->StartFetcherAndWait();
 
-  EXPECT_TRUE(delegate->fetcher()->GetStatus().is_success());
-  EXPECT_EQ(200, delegate->fetcher()->GetResponseCode());
-  std::string data;
-  ASSERT_TRUE(delegate->fetcher()->GetResponseAsString(&data));
-  EXPECT_EQ(kDefaultResponseBody, data);
+  // Since we cannot use StartFetchAndWait(), which runs a nested RunLoop owned
+  // by the Delegate, on a SchedulerWorker Sequence, this test is split into
+  // two Callbacks, both run on |sequenced_task_runner_|. The test main thread
+  // then runs its own RunLoop, which the second of the Callbacks will quit.
+  base::RunLoop run_loop;
 
-  sequenced_task_runner->DeleteSoon(FROM_HERE, delegate.release());
+  // Actually start the test fetch, on the Sequence.
+  sequenced_task_runner->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](scoped_refptr<FetcherTestURLRequestContextGetter> context_getter,
+             const GURL& response_path, base::OnceClosure quit_closure) {
+            std::unique_ptr<WaitingURLFetcherDelegate> delegate =
+                std::make_unique<WaitingURLFetcherDelegate>();
+            WaitingURLFetcherDelegate* raw_delegate = delegate.get();
+
+            // Configure the delegate to run our |on_complete_closure_| rather
+            // than quitting its own |run_loop_|, on completion.
+            raw_delegate->set_on_complete_or_cancel_closure(base::BindOnce(
+                [](base::OnceClosure quit_closure,
+                   std::unique_ptr<WaitingURLFetcherDelegate> delegate) {
+                  EXPECT_TRUE(delegate->fetcher()->GetStatus().is_success());
+                  EXPECT_EQ(200, delegate->fetcher()->GetResponseCode());
+                  std::string data;
+                  ASSERT_TRUE(delegate->fetcher()->GetResponseAsString(&data));
+                  EXPECT_EQ(kDefaultResponseBody, data);
+                  std::move(quit_closure).Run();
+                },
+                base::Passed(&quit_closure), base::Passed(&delegate)));
+
+            raw_delegate->CreateFetcher(response_path, URLFetcher::GET,
+                                        context_getter);
+            raw_delegate->fetcher()->Start();
+          },
+          CreateCrossThreadContextGetter(),
+          test_server_->GetURL(kDefaultResponsePath), run_loop.QuitClosure()));
+
+  run_loop.Run();
   RunUntilIdle();
 }
 
@@ -780,7 +811,7 @@ TEST_F(URLFetcherTest, PostEntireFile) {
   delegate.fetcher()->SetUploadFilePath("application/x-www-form-urlencoded",
                                         upload_path, 0,
                                         std::numeric_limits<uint64_t>::max(),
-                                        base::ThreadTaskRunnerHandle::Get());
+                                        base::SequencedTaskRunnerHandle::Get());
   delegate.StartFetcherAndWait();
 
   EXPECT_TRUE(delegate.fetcher()->GetStatus().is_success());
@@ -803,7 +834,7 @@ TEST_F(URLFetcherTest, PostFileRange) {
                          CreateSameThreadContextGetter());
   delegate.fetcher()->SetUploadFilePath("application/x-www-form-urlencoded",
                                         upload_path, kRangeStart, kRangeLength,
-                                        base::ThreadTaskRunnerHandle::Get());
+                                        base::SequencedTaskRunnerHandle::Get());
   delegate.StartFetcherAndWait();
 
   EXPECT_TRUE(delegate.fetcher()->GetStatus().is_success());
@@ -1326,14 +1357,15 @@ TEST_F(URLFetcherTest, CancelSameThread) {
 // Make sure that the URLFetcher releases its context getter pointer on
 // cancellation, cross-thread case.
 TEST_F(URLFetcherTest, CancelDifferentThreads) {
-  base::RunLoop run_loop_;
+  base::RunLoop run_loop;
 
   WaitingURLFetcherDelegate delegate;
   scoped_refptr<FetcherTestURLRequestContextGetter> context_getter(
       CreateCrossThreadContextGetter());
-  context_getter->set_on_destruction_callback(base::Bind(
-      base::IgnoreResult(&base::SingleThreadTaskRunner::PostTask),
-      base::ThreadTaskRunnerHandle::Get(), FROM_HERE, run_loop_.QuitClosure()));
+  context_getter->set_on_destruction_callback(
+      base::BindOnce(base::IgnoreResult(&base::SequencedTaskRunner::PostTask),
+                     base::SequencedTaskRunnerHandle::Get(), FROM_HERE,
+                     run_loop.QuitClosure()));
   delegate.CreateFetcher(hanging_url(), URLFetcher::GET, context_getter);
 
   // The getter won't be destroyed if the test holds on to a reference to it.
@@ -1341,21 +1373,22 @@ TEST_F(URLFetcherTest, CancelDifferentThreads) {
 
   delegate.fetcher()->Start();
   delegate.CancelFetch();
-  run_loop_.Run();
+  run_loop.Run();
 
   EXPECT_FALSE(delegate.did_complete());
 }
 
 TEST_F(URLFetcherTest, CancelWhileDelayedByThrottleDifferentThreads) {
   GURL url = test_server_->GetURL(kDefaultResponsePath);
-  base::RunLoop run_loop_;
+  base::RunLoop run_loop;
 
   WaitingURLFetcherDelegate delegate;
   scoped_refptr<FetcherTestURLRequestContextGetter> context_getter(
       CreateCrossThreadContextGetter());
-  context_getter->set_on_destruction_callback(base::Bind(
-      base::IgnoreResult(&base::SingleThreadTaskRunner::PostTask),
-      base::ThreadTaskRunnerHandle::Get(), FROM_HERE, run_loop_.QuitClosure()));
+  context_getter->set_on_destruction_callback(
+      base::BindOnce(base::IgnoreResult(&base::SequencedTaskRunner::PostTask),
+                     base::SequencedTaskRunnerHandle::Get(), FROM_HERE,
+                     run_loop.QuitClosure()));
   delegate.CreateFetcher(url, URLFetcher::GET, context_getter);
 
   // Register an entry for test url using a sliding window of 400 seconds, and
@@ -1373,7 +1406,7 @@ TEST_F(URLFetcherTest, CancelWhileDelayedByThrottleDifferentThreads) {
 
   delegate.fetcher()->Start();
   delegate.CancelFetch();
-  run_loop_.Run();
+  run_loop.Run();
 
   EXPECT_FALSE(delegate.did_complete());
 }
@@ -1568,8 +1601,8 @@ TEST_F(URLFetcherTest, FileTestTryToOverwriteDirectory) {
       test_server_->GetURL(std::string(kTestServerFilePrefix) + kFileToFetch),
       URLFetcher::GET, CreateSameThreadContextGetter());
   delegate.fetcher()->SaveResponseToFileAtPath(
-      out_path, scoped_refptr<base::SingleThreadTaskRunner>(
-                    base::ThreadTaskRunnerHandle::Get()));
+      out_path, scoped_refptr<base::SequencedTaskRunner>(
+                    base::SequencedTaskRunnerHandle::Get()));
   delegate.StartFetcherAndWait();
 
   EXPECT_FALSE(delegate.fetcher()->GetStatus().is_success());