Revert "Remove SpawnedTestServer timeouts."

This reverts commit 8e4bd238.

Reason for revert: want to verify that workarounds to https://crbug.com/869227 inside vpython itself are actually useful
plus to read more logs.

The original can re-landed any time if new occurrences of https://crbug.com/869227 manifest themselves.

Original change's description:
> Remove SpawnedTestServer timeouts.
> 
> These timeouts are ancient. The test infrastructure has its own
> timeouts, which should make these obsolete. Also, this may help with
> https://crbug.com/869227, where starting the Python test server
> times out.
> 
> Bug: 869227
> Change-Id: I9ea4f3174bcaa86daf09b965c82921432d8d556a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869717
> Commit-Queue: Matt Menke <mmenke@chromium.org>
> Reviewed-by: David Benjamin <davidben@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#707894}

TBR=davidben@chromium.org,mmenke@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 869227
Change-Id: Ic11c2e9a78568738a7712a1141876840f8a63740
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893789
Auto-Submit: Andrii Shyshkalov <tandrii@google.com>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711706}

net/test/spawned_test_server/local_test_server_posix.cc

@@ -19,6 +19,7 @@
 #include "base/process/process_iterator.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/test/test_timeouts.h"
 #include "net/test/python_utils.h"
 
 namespace {
@@ -58,9 +59,13 @@ class OrphanedTestServerFilter : public base::ProcessFilter {
 
 // Given a file descriptor, reads into |buffer| until |bytes_max|
 // bytes has been read or an error has been encountered.  Returns true
-// if the read was successful.
-bool ReadData(int fd, ssize_t bytes_max, uint8_t* buffer) {
+// if the read was successful.  |remaining_time| is used as a timeout.
+bool ReadData(int fd,
+              ssize_t bytes_max,
+              uint8_t* buffer,
+              base::TimeDelta* remaining_time) {
   ssize_t bytes_read = 0;
+  base::TimeTicks previous_time = base::TimeTicks::Now();
   while (bytes_read < bytes_max) {
     struct pollfd poll_fds[1];
 
@@ -68,8 +73,8 @@ bool ReadData(int fd, ssize_t bytes_max, uint8_t* buffer) {
     poll_fds[0].events = POLLIN | POLLPRI;
     poll_fds[0].revents = 0;
 
-    // Each test itself has its own timeout, so no need to use one here.
-    int rv = HANDLE_EINTR(poll(poll_fds, 1, -1));
+    int rv = HANDLE_EINTR(poll(poll_fds, 1,
+                               remaining_time->InMilliseconds()));
     if (rv == 0) {
       LOG(ERROR) << "poll() timed out; bytes_read=" << bytes_read;
       return false;
@@ -79,6 +84,12 @@ bool ReadData(int fd, ssize_t bytes_max, uint8_t* buffer) {
       return false;
     }
 
+    base::TimeTicks current_time = base::TimeTicks::Now();
+    base::TimeDelta elapsed_time_cycle = current_time - previous_time;
+    DCHECK_GE(elapsed_time_cycle.InMilliseconds(), 0);
+    *remaining_time -= elapsed_time_cycle;
+    previous_time = current_time;
+
     ssize_t num_bytes = HANDLE_EINTR(read(fd, buffer + bytes_read,
                                           bytes_max - bytes_read));
     if (num_bytes <= 0)
@@ -152,15 +163,18 @@ bool LocalTestServer::LaunchPython(
 bool LocalTestServer::WaitToStart() {
   base::ScopedFD our_fd(child_fd_.release());
 
+  base::TimeDelta remaining_time = TestTimeouts::action_timeout();
+
   uint32_t server_data_len = 0;
   if (!ReadData(our_fd.get(), sizeof(server_data_len),
-                reinterpret_cast<uint8_t*>(&server_data_len))) {
+                reinterpret_cast<uint8_t*>(&server_data_len),
+                &remaining_time)) {
     LOG(ERROR) << "Could not read server_data_len";
     return false;
   }
   std::string server_data(server_data_len, '\0');
   if (!ReadData(our_fd.get(), server_data_len,
-                reinterpret_cast<uint8_t*>(&server_data[0]))) {
+                reinterpret_cast<uint8_t*>(&server_data[0]), &remaining_time)) {
     LOG(ERROR) << "Could not read server_data (" << server_data_len
                << " bytes)";
     return false;

net/test/spawned_test_server/local_test_server_win.cc

@@ -13,14 +13,32 @@
 #include "base/files/file_path.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
+#include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/thread.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/win/scoped_handle.h"
 #include "net/test/python_utils.h"
 
 namespace {
 
+// Writes |size| bytes to |handle| and sets |*unblocked| to true.
+// Used as a crude timeout mechanism by ReadData().
+void UnblockPipe(HANDLE handle, DWORD size, bool* unblocked) {
+  std::string unblock_data(size, '\0');
+  // Unblock the ReadFile in LocalTestServer::WaitToStart by writing to the
+  // pipe. Make sure the call succeeded, otherwise we are very likely to hang.
+  DWORD bytes_written = 0;
+  LOG(WARNING) << "Timeout reached; unblocking pipe by writing "
+               << size << " bytes";
+  CHECK(WriteFile(handle, unblock_data.data(), size, &bytes_written, nullptr));
+  CHECK_EQ(size, bytes_written);
+  *unblocked = true;
+}
+
 // Given a file handle, reads into |buffer| until |bytes_max| bytes
 // has been read or an error has been encountered.  Returns
 // true if the read was successful.
@@ -28,6 +46,16 @@ bool ReadData(HANDLE read_fd,
               HANDLE write_fd,
               DWORD bytes_max,
               uint8_t* buffer) {
+  base::Thread thread("test_server_watcher");
+  if (!thread.Start())
+    return false;
+
+  // Prepare a timeout in case the server fails to start.
+  bool unblocked = false;
+  thread.task_runner()->PostDelayedTask(
+      FROM_HERE, base::BindOnce(UnblockPipe, write_fd, bytes_max, &unblocked),
+      TestTimeouts::action_max_timeout());
+
   DWORD bytes_read = 0;
   while (bytes_read < bytes_max) {
     DWORD num_bytes;
@@ -43,6 +71,15 @@ bool ReadData(HANDLE read_fd,
     bytes_read += num_bytes;
   }
 
+  base::ScopedAllowBaseSyncPrimitivesForTesting allow_thread_join;
+  thread.Stop();
+
+  // If the timeout kicked in, abort.
+  if (unblocked) {
+    LOG(ERROR) << "Timeout exceeded for ReadData";
+    return false;
+  }
+
   return true;
 }
 

net/test/spawned_test_server/remote_test_server_spawner_request.cc

@@ -12,7 +12,10 @@
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/test_timeouts.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/io_buffer.h"
@@ -47,6 +50,7 @@ class RemoteTestServerSpawnerRequest::Core : public URLRequest::Delegate {
 
   void ReadResponse();
   void OnCommandCompleted(int net_error);
+  void OnTimeout();
 
   // Request results.
   int result_code_ = 0;
@@ -60,6 +64,8 @@ class RemoteTestServerSpawnerRequest::Core : public URLRequest::Delegate {
 
   scoped_refptr<IOBuffer> read_buffer_;
 
+  std::unique_ptr<base::OneShotTimer> timeout_timer_;
+
   THREAD_CHECKER(thread_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(Core);
@@ -96,6 +102,10 @@ void RemoteTestServerSpawnerRequest::Core::SendRequest(
                                           /*override=*/true);
   }
 
+  timeout_timer_ = std::make_unique<base::OneShotTimer>();
+  timeout_timer_->Start(FROM_HERE, TestTimeouts::action_max_timeout(),
+                        base::Bind(&Core::OnTimeout, base::Unretained(this)));
+
   request_->Start();
 }
 
@@ -114,6 +124,13 @@ bool RemoteTestServerSpawnerRequest::Core::WaitForCompletion(
   return result_code_ == OK;
 }
 
+void RemoteTestServerSpawnerRequest::Core::OnTimeout() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  int result = request_->CancelWithError(ERR_TIMED_OUT);
+  OnCommandCompleted(result);
+}
+
 void RemoteTestServerSpawnerRequest::Core::OnCommandCompleted(int net_error) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK_NE(ERR_IO_PENDING, net_error);
@@ -135,6 +152,7 @@ void RemoteTestServerSpawnerRequest::Core::OnCommandCompleted(int net_error) {
 
   request_.reset();
   context_.reset();
+  timeout_timer_.reset();
 
   event_.Signal();
 }