[ChromeDriver] Solve bug in adb_client_socket.cc.

This fixes a "net::ERR_FAILED" (-2) error. That error was being
silently ignored because Device::SetUp was not returning the status
of ForwardDevtoolsPort. Since the bug is fixed, we can re-enable
returning the status.

This change is setup for a future change to send port 0 to adb server
to avoid a race condition.

Change-Id: Ic0aece82cd0ff0a413f32bd5587d12998be64a21
Reviewed-on: https://chromium-review.googlesource.com/1000846
Commit-Queue: Caleb Rouleau <crouleau@chromium.org>
Reviewed-by: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549311}

chrome/test/chromedriver/chrome/adb_impl.cc

@@ -49,8 +49,10 @@ class ResponseBuffer : public base::RefCountedThreadSafe<ResponseBuffer> {
       ready_.TimedWait(timeout);
     }
     if (result_ < 0)
-      return Status(kUnknownError,
-          "Failed to run adb command, is the adb server running?");
+      return Status(
+          kUnknownError,
+          "Failed to run adb command, is the adb server running? Error: " +
+              response_ + ".");
     *response = response_;
     return Status(kOk);
   }
@@ -115,15 +117,13 @@ Status AdbImpl::ForwardPort(
     const std::string& device_serial, int local_port,
     const std::string& remote_abstract) {
   std::string response;
-  Status status = ExecuteHostCommand(
-      device_serial,
-      "forward:tcp:" + base::IntToString(local_port) + ";localabstract:" +
-          remote_abstract,
-      &response);
-  if (!status.IsOk())
+  Status status =
+      ExecuteHostCommand(device_serial,
+                         "forward:tcp:" + base::IntToString(local_port) +
+                             ";localabstract:" + remote_abstract,
+                         &response);
+  if (status.IsOk())
     return status;
-  if (response == "OKAY")
-    return Status(kOk);
   return Status(kUnknownError, "Failed to forward ports to device " +
                 device_serial + ": " + response);
 }
@@ -207,8 +207,8 @@ Status AdbImpl::GetPidByName(const std::string& device_serial,
                              int* pid) {
   std::string response;
   // on Android O `ps` returns only user processes, so also try with `-A` flag.
-  Status status = ExecuteHostShellCommand(device_serial, "ps && ps -A",
-      &response);
+  Status status =
+      ExecuteHostShellCommand(device_serial, "ps && ps -A", &response);
 
   if (!status.IsOk())
     return status;

chrome/test/chromedriver/chrome/device_manager.cc

@@ -127,9 +127,8 @@ Status Device::SetUp(const std::string& package,
 
     active_package_ = package;
   }
-  this->ForwardDevtoolsPort(package, process, port, &known_device_socket);
-
-  return status;
+  return this->ForwardDevtoolsPort(package, process, port,
+                                   &known_device_socket);
 }
 
 Status Device::ForwardDevtoolsPort(const std::string& package,

chrome/test/chromedriver/net/adb_client_socket.cc

@@ -623,7 +623,25 @@ void AdbClientSocket::OnResponseLength(
 
   std::string new_response =
       response + std::string(response_buffer->data(), result);
+  // Sometimes ADB server will respond like "OKAYOKAY<payload_length><payload>"
+  // instead of the expected "OKAY<payload_length><payload>".
+  // For the former case, the first OKAY is cropped off in the OnResponseStatus
+  // but we still need to crop the second OKAY.
+  if (new_response.substr(0, 4) == "OKAY") {
+    VLOG(3) << "cutting new_response down to size";
+    new_response = new_response.substr(4);
+  }
   if (new_response.length() < 4) {
+    if (new_response.length() == 0 && result == net::OK) {
+      // The socket is shut down and all data has been read.
+      // Note that this is a hack because is_void is false,
+      // but we are not returning any data. The upstream logic
+      // to determine is_void is not in a good state.
+      // However, this is a better solution than trusting is_void anyway
+      // because otherwise this can become an infinite loop.
+      callback.Run(net::OK, new_response);
+      return;
+    }
     result = socket_->Read(response_buffer.get(),
                            kBufferSize,
                            base::Bind(&AdbClientSocket::OnResponseLength,
@@ -636,7 +654,9 @@ void AdbClientSocket::OnResponseLength(
   } else {
     int payload_length = 0;
     if (!base::HexStringToInt(new_response.substr(0, 4), &payload_length)) {
-      callback.Run(net::ERR_FAILED, new_response);
+      VLOG(1) << "net error since payload length wasn't readable.";
+      callback.Run(net::ERR_FAILED, "response <" + new_response +
+                                        "> from adb server was unexpected");
       return;
     }