Parallel Download: Fix flaky test ParallelDownloadComplete on Android.

Refactor RunLoop::RunUntilIdle call in a while loop into an observer
class that only hold one RunLoop.

RunLoop::RunUntilIdle in a while loop may never return in multithread
context on Android platform.

Bug: 786626
Change-Id: Ic5deb53e892d9c6303346eb6a7f803c89eddd26d
Reviewed-on: https://chromium-review.googlesource.com/882183
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532011}

content/browser/download/download_browsertest.cc

@@ -778,11 +778,6 @@ class DownloadContentTest : public ContentBrowserTest {
     return CountingDownloadFile::GetNumberActiveFilesFromFileThread() == 0;
   }
 
-  void WaitForServerToFinishAllResponses(size_t request_num) {
-    while (test_response_handler_.completed_requests().size() != request_num)
-      base::RunLoop().RunUntilIdle();
-  }
-
   void SetupErrorInjectionDownloads() {
     auto factory = std::make_unique<ErrorInjectionDownloadFileFactory>();
     inject_error_callback_ = base::Bind(
@@ -1998,7 +1993,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveResumedDownload) {
   EXPECT_FALSE(PathExists(intermediate_path));
   EXPECT_FALSE(PathExists(target_path));
   EXPECT_TRUE(EnsureNoPendingDownloads());
-  WaitForServerToFinishAllResponses(2);
+  test_response_handler()->WaitUntilCompletion(2u);
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelResumedDownload) {
@@ -2033,7 +2028,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelResumedDownload) {
   EXPECT_FALSE(PathExists(intermediate_path));
   EXPECT_FALSE(PathExists(target_path));
   EXPECT_TRUE(EnsureNoPendingDownloads());
-  WaitForServerToFinishAllResponses(2);
+  test_response_handler()->WaitUntilCompletion(2u);
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeRestoredDownload_NoFile) {
@@ -2899,14 +2894,8 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest,
                download->GetTargetFilePath().BaseName().value().c_str());
 }
 
-#if defined(OS_ANDROID)
-// Flaky on android: https://crbug.com/786626
-#define MAYBE_ParallelDownloadComplete DISABLED_ParallelDownloadComplete
-#else
-#define MAYBE_ParallelDownloadComplete ParallelDownloadComplete
-#endif
 // Verify parallel download in normal case.
-IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, MAYBE_ParallelDownloadComplete) {
+IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, ParallelDownloadComplete) {
   EXPECT_TRUE(base::FeatureList::IsEnabled(features::kParallelDownloading));
 
   GURL url = TestDownloadHttpResponse::GetNextURLForDownload();
@@ -2928,9 +2917,8 @@ IN_PROC_BROWSER_TEST_F(ParallelDownloadTest, MAYBE_ParallelDownloadComplete) {
   DownloadItem* download = StartDownloadAndReturnItem(shell(), server_url);
   // Send some data for the first request and pause it so download won't
   // complete before other parallel requests are created.
-  request_pause_handler.WaitForCallback();
-  while (test_response_handler()->completed_requests().size() == 0)
-    base::RunLoop().RunUntilIdle();
+  test_response_handler()->WaitUntilCompletion(1u);
+
   // Now resume the first request.
   request_pause_handler.Resume();
   WaitForCompletion(download);

content/public/test/test_download_http_response.cc

@@ -461,10 +461,14 @@ TestDownloadResponseHandler::HandleTestDownloadRequest(
 }
 
 TestDownloadResponseHandler::TestDownloadResponseHandler() = default;
-TestDownloadResponseHandler::~TestDownloadResponseHandler() = default;
+
+TestDownloadResponseHandler::~TestDownloadResponseHandler() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 void TestDownloadResponseHandler::RegisterToTestServer(
     net::test_server::EmbeddedTestServer* server) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!server->Started())
       << "Register request handler before starting the server";
   server->RegisterRequestHandler(base::Bind(
@@ -475,7 +479,26 @@ void TestDownloadResponseHandler::RegisterToTestServer(
 
 void TestDownloadResponseHandler::OnRequestCompleted(
     std::unique_ptr<TestDownloadHttpResponse::CompletedRequest> request) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   completed_requests_.push_back(std::move(request));
+
+  if (run_loop_ && run_loop_->running() &&
+      completed_requests().size() >= request_count_) {
+    run_loop_->Quit();
+  }
+}
+
+void TestDownloadResponseHandler::WaitUntilCompletion(size_t request_count) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  request_count_ = request_count;
+
+  if ((run_loop_ && run_loop_->running()) ||
+      completed_requests().size() >= request_count_) {
+    return;
+  }
+
+  run_loop_ = std::make_unique<base::RunLoop>();
+  run_loop_->Run();
 }
 
 }  // namespace content

content/public/test/test_download_http_response.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/containers/queue.h"
+#include "base/sequence_checker.h"
 #include "net/http/http_response_info.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
@@ -266,12 +267,19 @@ class TestDownloadResponseHandler {
   void OnRequestCompleted(
       std::unique_ptr<TestDownloadHttpResponse::CompletedRequest> request);
 
+  // Wait for a certain number of requests to complete.
+  void WaitUntilCompletion(size_t request_count);
+
   using CompletedRequests =
       std::vector<std::unique_ptr<TestDownloadHttpResponse::CompletedRequest>>;
   CompletedRequests const& completed_requests() { return completed_requests_; }
 
  private:
   CompletedRequests completed_requests_;
+  size_t request_count_ = 0u;
+  std::unique_ptr<base::RunLoop> run_loop_;
+  SEQUENCE_CHECKER(sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(TestDownloadResponseHandler);
 };