Fix occasional test crashes if fake HTTP post times out

Prior to this patch, if the command times out (usually during shutdown),
the unretained string pointers may point to freed memory, and the
FakeServer may crash with:
BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x0000f6acf3b8 base::debug::CollectStackTrace()
Pissandshittium#1 0x0000f682c97f base::debug::StackTrace::StackTrace()
Pissandshittium#2 0x0000f682c90e base::debug::StackTrace::StackTrace()
Pissandshittium#3 0x00005d659bac content::(anonymous namespace)::DumpStackTraceSignalHandler()
Pissandshittium#4 0x0000f76e0cb0 ([vdso]+0xcaf)
Pissandshittium#5 0x0000569dd026 std::__Cr::char_traits<>::assign()
Pissandshittium#6 0x0000569e138b std::__Cr::basic_string<>::clear()
Pissandshittium#7 0x00005d7c965e fake_server::FakeServer::HandleCommand()
Pissandshittium#8 0x00005d7d0a54 fake_server::(anonymous namespace)::HandleCommandOnFakeServerThread()
Pissandshittium#9 0x00005d7d130f base::internal::FunctorTraits<>::Invoke<>()

The new approach avoids time-based codepaths and instead handles the
Abort() case explicitly.

Bug: 869404
Change-Id: Ibae008d16d331db85d7fe1dd3609255813818fa8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1511353
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#639026}

components/sync/test/fake_server/fake_server_http_post_provider.cc

@@ -19,21 +19,6 @@ namespace fake_server {
 // static
 bool FakeServerHttpPostProvider::network_enabled_ = true;
 
-namespace {
-
-void HandleCommandOnFakeServerThread(base::WeakPtr<FakeServer> fake_server,
-                                     const std::string& request,
-                                     int* http_status_code,
-                                     std::string* response,
-                                     base::WaitableEvent* completion_event) {
-  if (fake_server) {
-    *http_status_code = fake_server->HandleCommand(request, response);
-  }
-  completion_event->Signal();
-}
-
-}  // namespace
-
 FakeServerHttpPostProviderFactory::FakeServerHttpPostProviderFactory(
     const base::WeakPtr<FakeServer>& fake_server,
     scoped_refptr<base::SequencedTaskRunner> fake_server_task_runner)
@@ -62,7 +47,11 @@ FakeServerHttpPostProvider::FakeServerHttpPostProvider(
     const base::WeakPtr<FakeServer>& fake_server,
     scoped_refptr<base::SequencedTaskRunner> fake_server_task_runner)
     : fake_server_(fake_server),
-      fake_server_task_runner_(fake_server_task_runner) {}
+      fake_server_task_runner_(fake_server_task_runner),
+      synchronous_post_completion_(
+          base::WaitableEvent::ResetPolicy::AUTOMATIC,
+          base::WaitableEvent::InitialState::NOT_SIGNALED),
+      aborted_(false) {}
 
 FakeServerHttpPostProvider::~FakeServerHttpPostProvider() {}
 
@@ -93,20 +82,19 @@ bool FakeServerHttpPostProvider::MakeSynchronousPost(int* net_error_code,
     return false;
   }
 
+  synchronous_post_completion_.Reset();
+  aborted_ = false;
+
   // It is assumed that a POST is being made to /command.
   int post_status_code = -1;
   std::string post_response;
 
-  base::WaitableEvent post_complete(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
   bool result = fake_server_task_runner_->PostTask(
       FROM_HERE,
-      base::BindOnce(&HandleCommandOnFakeServerThread, fake_server_,
-                     request_content_, base::Unretained(&post_status_code),
-                     base::Unretained(&post_response),
-                     base::Unretained(&post_complete)));
+      base::BindOnce(
+          &FakeServerHttpPostProvider::HandleCommandOnFakeServerThread,
+          base::RetainedRef(this), base::Unretained(&post_status_code),
+          base::Unretained(&post_response)));
 
   if (!result) {
     response_.clear();
@@ -115,14 +103,10 @@ bool FakeServerHttpPostProvider::MakeSynchronousPost(int* net_error_code,
     return false;
   }
 
-  // Note: This is a potential deadlock. Here we're on the sync thread, and
-  // we're waiting for something to happen on the UI thread (where the
-  // FakeServer lives). If at the same time, ProfileSyncService is trying to
-  // Stop() the sync thread, we're deadlocked. For a lack of better ideas, let's
-  // just give up after a few seconds.
-  // TODO(crbug.com/869404): Maybe the FakeServer should live on its own thread.
-  if (!post_complete.TimedWait(base::TimeDelta::FromSeconds(5))) {
-    *net_error_code = net::ERR_TIMED_OUT;
+  synchronous_post_completion_.Wait();
+
+  if (aborted_) {
+    *net_error_code = net::ERR_ABORTED;
     return false;
   }
 
@@ -147,7 +131,14 @@ const std::string FakeServerHttpPostProvider::GetResponseHeaderValue(
   return std::string();
 }
 
-void FakeServerHttpPostProvider::Abort() {}
+void FakeServerHttpPostProvider::Abort() {
+  // The sync thread could be blocked in MakeSynchronousPost(), waiting
+  // for HandleCommandOnFakeServerThread() to be processed and completed.
+  // This causes an immediate unblocking which will be returned as
+  // net::ERR_ABORTED.
+  aborted_ = true;
+  synchronous_post_completion_.Signal();
+}
 
 void FakeServerHttpPostProvider::DisableNetwork() {
   network_enabled_ = false;
@@ -157,4 +148,18 @@ void FakeServerHttpPostProvider::EnableNetwork() {
   network_enabled_ = true;
 }
 
+void FakeServerHttpPostProvider::HandleCommandOnFakeServerThread(
+    int* http_status_code,
+    std::string* response) {
+  DCHECK(fake_server_task_runner_->RunsTasksInCurrentSequence());
+
+  if (!fake_server_ || aborted_) {
+    // Command explicitly aborted or server destroyed.
+    return;
+  }
+
+  *http_status_code = fake_server_->HandleCommand(request_content_, response);
+  synchronous_post_completion_.Signal();
+}
+
 }  // namespace fake_server

components/sync/test/fake_server/fake_server_http_post_provider.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SYNC_TEST_FAKE_SERVER_FAKE_SERVER_HTTP_POST_PROVIDER_H_
 #define COMPONENTS_SYNC_TEST_FAKE_SERVER_FAKE_SERVER_HTTP_POST_PROVIDER_H_
 
+#include <atomic>
 #include <string>
 
 #include "base/callback.h"
@@ -54,13 +55,19 @@ class FakeServerHttpPostProvider
  private:
   friend class base::RefCountedThreadSafe<FakeServerHttpPostProvider>;
 
+  void HandleCommandOnFakeServerThread(int* http_status_code,
+                                       std::string* response);
+
   static bool network_enabled_;
 
   // |fake_server_| should only be dereferenced on the same thread as
   // |fake_server_task_runner_| runs on.
   base::WeakPtr<FakeServer> fake_server_;
   scoped_refptr<base::SequencedTaskRunner> fake_server_task_runner_;
 
+  base::WaitableEvent synchronous_post_completion_;
+  std::atomic_bool aborted_;
+
   std::string response_;
   std::string request_url_;
   int request_port_;