Fix flaky NetworkServiceRestartBrowserTest.BrowserIOSharedFactoryAfterStoragePartitionGone

This test is flaky on Linux ASAN and Android, sample failure from Android:
```
logging::LogMessage::~LogMessage()
base::internal::WeakReference::Flag::Invalidate()
base::internal::WeakReferenceOwner::~WeakReferenceOwner()
content::SimpleURLLoaderTestHelper::~SimpleURLLoaderTestHelper()
content::(anonymous namespace)::LoadBasicRequestOnIOThread(network::mojom::URLLoaderFactory*, GURL const&)
content::NetworkServiceRestartBrowserTest_BrowserIOSharedFactoryAfterStoragePartitionGone_Test::RunTestOnMainThread()
```

This CL makes sure that |simple_loader_helper.GetCallback()| was
only accessed on UI thread (where |simple_loader_helper| lives).

(I cannot reproduce the flakiness locally, will see if this fixes
the issue.)

Bug: 822585
Change-Id: I74de7dd4708f20dcb2f0145228d6de19ac7f15b3
Reviewed-on: https://chromium-review.googlesource.com/966985
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543859}

content/browser/network_service_restart_browsertest.cc

@@ -42,6 +42,19 @@ network::mojom::NetworkContextPtr CreateNetworkContext() {
   return network_context;
 }
 
+network::SimpleURLLoader::BodyAsStringCallback RunOnUIThread(
+    network::SimpleURLLoader::BodyAsStringCallback ui_callback) {
+  return base::BindOnce(
+      [](network::SimpleURLLoader::BodyAsStringCallback callback,
+         std::unique_ptr<std::string> response_body) {
+        DCHECK_CURRENTLY_ON(BrowserThread::IO);
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::BindOnce(std::move(callback), std::move(response_body)));
+      },
+      std::move(ui_callback));
+}
+
 int LoadBasicRequestOnIOThread(
     network::mojom::URLLoaderFactory* url_loader_factory,
     const GURL& url) {
@@ -49,9 +62,9 @@ int LoadBasicRequestOnIOThread(
   auto request = std::make_unique<network::ResourceRequest>();
   request->url = url;
 
+  // |simple_loader_helper| lives on UI thread and shouldn't be accessed on
+  // other threads.
   SimpleURLLoaderTestHelper simple_loader_helper;
-  // Wait for callback on UI thread to avoid nesting IO message loops.
-  simple_loader_helper.SetRunLoopQuitThread(BrowserThread::UI);
 
   std::unique_ptr<network::SimpleURLLoader> simple_loader =
       network::SimpleURLLoader::Create(std::move(request),
@@ -69,7 +82,7 @@ int LoadBasicRequestOnIOThread(
           },
           base::Unretained(simple_loader.get()),
           base::Unretained(url_loader_factory),
-          simple_loader_helper.GetCallback()));
+          RunOnUIThread(simple_loader_helper.GetCallback())));
 
   simple_loader_helper.WaitForCallback();
   return simple_loader->NetError();
@@ -398,11 +411,8 @@ IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
 // Make sure the factory returned from
 // |URLLoaderFactoryGetter::GetNetworkFactory()| doesn't crash if
 // it's called after the StoragePartition is deleted.
-// TODO(crbug.com/822585): Disabled since flaky on at least Linux ASAN and
-// Android.
-IN_PROC_BROWSER_TEST_F(
-    NetworkServiceRestartBrowserTest,
-    DISABLED_BrowserIOSharedFactoryAfterStoragePartitionGone) {
+IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
+                       BrowserIOSharedFactoryAfterStoragePartitionGone) {
   base::ScopedAllowBlockingForTesting allow_blocking;
   std::unique_ptr<ShellBrowserContext> browser_context =
       std::make_unique<ShellBrowserContext>(true, nullptr);

content/public/test/simple_url_loader_test_helper.cc

@@ -30,24 +30,12 @@ void SimpleURLLoaderTestHelper::WaitForCallback() {
   run_loop_.Run();
 }
 
-void SimpleURLLoaderTestHelper::SetRunLoopQuitThread(
-    BrowserThread::ID thread_id) {
-  run_loop_quit_thread_ = thread_id;
-}
-
 void SimpleURLLoaderTestHelper::OnCompleteCallback(
     std::unique_ptr<std::string> response_body) {
   DCHECK(!response_body_);
 
   response_body_ = std::move(response_body);
 
-  if (run_loop_quit_thread_ &&
-      !BrowserThread::CurrentlyOn(*run_loop_quit_thread_)) {
-    BrowserThread::PostTask(*run_loop_quit_thread_, FROM_HERE,
-                            run_loop_.QuitClosure());
-    return;
-  }
-
   run_loop_.Quit();
 }
 

content/public/test/simple_url_loader_test_helper.h

@@ -11,9 +11,7 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/optional.h"
 #include "base/run_loop.h"
-#include "content/public/browser/browser_thread.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 
 namespace content {
@@ -32,9 +30,6 @@ class SimpleURLLoaderTestHelper {
   // Waits until the callback returned by GetCallback() is invoked.
   void WaitForCallback();
 
-  // Specify the thread to quit the runloop.
-  void SetRunLoopQuitThread(BrowserThread::ID);
-
   // Response body passed to the callback returned by GetCallback, if there was
   // one.
   const std::string* response_body() const { return response_body_.get(); }
@@ -51,10 +46,6 @@ class SimpleURLLoaderTestHelper {
 
   base::RunLoop run_loop_;
 
-  // When set, will post |run_loop_.Quit()| to |run_loop_quit_thread_| if it's
-  // not current thread.
-  base::Optional<BrowserThread::ID> run_loop_quit_thread_;
-
   std::unique_ptr<std::string> response_body_;
 
   base::WeakPtrFactory<SimpleURLLoaderTestHelper> weak_ptr_factory_;