[ChromeDriver] More robust adb server socket protocol implementation

This implementation separates the reading of the socket from the
parsing of the data from the socket to avoid edge cases.

The existing implementation makes assumptions about how the result
will be pushed to the socket from the other end. It had different
behavior depending on whether we read in an output like
"OKAYOKAY0005abcde"
in separate chucks like
"OKAY"
"OKAY"
"0005abcde"
versus as larger chunks like
"OKAYOKAY"
"0005abcde"
or one big chunk like
"OKAYOKAY0005abcde"

Given that the adb server has some interesting edge cases (documented
in this new code), it was more complex to handle them with the old
implementation.

Note also that these edge cases were being hit when I tried to pass
port=0 to the adb server (so that it would allocate a remote forwarding
port itself so there would be no race condition). So that work is
blocked on this work.

Bug: chromedriver:2161
Change-Id: Ia83f8c3e4e0261a9467ab2814502c37f3f7e0645
Reviewed-on: https://chromium-review.googlesource.com/1011131
Commit-Queue: Caleb Rouleau <crouleau@chromium.org>
Reviewed-by: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554886}

chrome/test/chromedriver/BUILD.gn

@@ -388,6 +388,7 @@ test("chromedriver_unittests") {
     "command_listener_proxy_unittest.cc",
     "commands_unittest.cc",
     "logging_unittest.cc",
+    "net/adb_client_socket_unittest.cc",
     "net/timeout_unittest.cc",
     "performance_logger_unittest.cc",
     "server/http_handler_unittest.cc",
@@ -409,6 +410,8 @@ test("chromedriver_unittests") {
     "//base/test:run_all_unittests",
     "//net",
     "//net:http_server",
+    "//net:test_support",
+    "//testing/gmock",
     "//testing/gtest",
     "//ui/base",
     "//ui/gfx",

chrome/test/chromedriver/chrome/adb_impl.cc

@@ -20,6 +20,7 @@
 #include "base/time/time.h"
 #include "chrome/test/chromedriver/chrome/status.h"
 #include "chrome/test/chromedriver/net/adb_client_socket.h"
+#include "net/base/net_errors.h"
 
 namespace {
 
@@ -48,11 +49,20 @@ class ResponseBuffer : public base::RefCountedThreadSafe<ResponseBuffer> {
             static_cast<int>(timeout.InSeconds())));
       ready_.TimedWait(timeout);
     }
-    if (result_ < 0)
+    if (result_ < 0) {
+      return Status(kUnknownError,
+                    "Failed to run adb command with networking error: " +
+                        net::ErrorToString(result_) +
+                        ". Is the adb server running? Extra response: <" +
+                        response_ + ">.");
+    }
+    if (result_ > 0) {
       return Status(
+          // TODO(crouleau): Use an error code that can differentiate this from
+          // the above networking error.
           kUnknownError,
-          "Failed to run adb command, is the adb server running? Error: " +
-              response_ + ".");
+          "The adb command failed. Extra response: <" + response_ + ">.");
+    }
     *response = response_;
     return Status(kOk);
   }

chrome/test/chromedriver/net/adb_client_socket.h

@@ -16,6 +16,7 @@ class AdbClientSocket {
   typedef base::Callback<void(int, const std::string&)> CommandCallback;
   typedef base::Callback<void(int result,
                               net::StreamSocket*)> SocketCallback;
+  typedef base::Callback<void(const std::string&)> ParserCallback;
 
   static void AdbQuery(int port,
                        const std::string& query,
@@ -51,38 +52,34 @@ class AdbClientSocket {
   void Connect(const net::CompletionCallback& callback);
 
   void SendCommand(const std::string& command,
-                   bool is_void,
+                   bool has_output,
                    bool has_length,
-                   const CommandCallback& callback);
+                   const CommandCallback& response_callback);
 
   std::unique_ptr<net::StreamSocket> socket_;
 
   void ReadResponse(const CommandCallback& callback,
-                    bool is_void,
+                    bool has_output,
                     bool has_length,
                     int result);
 
  private:
-  void OnResponseStatus(const CommandCallback& callback,
-                        bool is_void,
-                        bool has_length,
-                        scoped_refptr<net::IOBuffer> response_buffer,
-                        int result);
-
-  void OnResponseLength(const CommandCallback& callback,
-                        const std::string& response,
-                        scoped_refptr<net::IOBuffer> response_buffer,
-                        int result);
-
-  void OnResponseData(const CommandCallback& callback,
-                      const std::string& response,
-                      scoped_refptr<net::IOBuffer> response_buffer,
-                      int bytes_left,
-                      int result);
+  static void ReadStatusOutput(const CommandCallback& response_callback,
+                               scoped_refptr<net::IOBuffer> socket_buffer,
+                               int socket_result);
+
+  void ReadUntilEOF(const ParserCallback& parse_output_callback,
+                    const CommandCallback& response_callback,
+                    scoped_refptr<net::GrowableIOBuffer> socket_buffer,
+                    int socket_result);
+
+  static void ParseOutput(bool has_length,
+                          const CommandCallback& response_callback,
+                          const std::string& adb_output);
 
   int port_;
 
+  friend class AdbClientSocketTest;
   DISALLOW_COPY_AND_ASSIGN(AdbClientSocket);
 };
-
 #endif  // CHROME_TEST_CHROMEDRIVER_NET_ADB_CLIENT_SOCKET_H_

chrome/test/chromedriver/net/adb_client_socket_unittest.cc

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/test/chromedriver/net/adb_client_socket.h"
+#include "base/logging.h"
+#include "base/run_loop.h"
+#include "base/test/gtest_util.h"
+#include "base/test/mock_callback.h"
+#include "net/socket/socket_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class MockSocket : public net::MockClientSocket {
+ public:
+  int return_values_length;
+  std::string* return_values_array;
+  MockSocket(std::string* return_values_array, int return_values_length)
+      : MockClientSocket(net::NetLogWithSource()),
+        return_values_length(return_values_length),
+        return_values_array(return_values_array) {}
+
+  MockSocket() : MockClientSocket(net::NetLogWithSource()) {}
+
+  int Read(net::IOBuffer* buf,
+           int buf_len,
+           net::CompletionOnceCallback callback) override {
+    int result = ReadHelper(buf, buf_len);
+    if (result == net::ERR_IO_PENDING) {
+      // Note this really should be posted as a task, but that is a pain to
+      // figure out.
+      std::move(callback).Run(ReadLoop(buf, buf_len));
+    }
+    return result;
+  }
+
+  int ReadLoop(net::IOBuffer* buf, int buf_len) {
+    int result;
+    do {
+      result = ReadHelper(buf, buf_len);
+    } while (result == net::ERR_IO_PENDING);
+    return result;
+  }
+
+  int ReadHelper(net::IOBuffer* buf, int buf_len) {
+    if (return_values_length) {
+      int chunk_length = return_values_array[0].length();
+      if (chunk_length > buf_len) {
+        strncpy(buf->data(), return_values_array[0].data(), buf_len);
+        return_values_array[0] = return_values_array[0].substr(buf_len);
+        return buf_len;
+      }
+      strncpy(buf->data(), return_values_array[0].data(), chunk_length);
+      return_values_length--;
+      return_values_array++;
+      if (chunk_length == 0) {
+        return net::ERR_IO_PENDING;
+      }
+      return chunk_length;
+    }
+    return 0;
+  }
+
+  int Write(
+      net::IOBuffer* buf,
+      int buf_len,
+      const net::CompletionOnceCallback callback,
+      const net::NetworkTrafficAnnotationTag& traffic_annotation) override {
+    return 0;
+  }
+
+  // The following functions are not expected to be used.
+  int Connect(const net::CompletionOnceCallback callback) override {
+    return net::ERR_UNEXPECTED;
+  }
+  bool GetSSLInfo(net::SSLInfo* ssl_info) override { return false; }
+  bool WasEverUsed() const override { return false; }
+};
+
+class AdbClientSocketTest : public testing::Test {
+ public:
+  void TestParsing(const char* adb_output,
+                   bool has_length,
+                   int expected_result_code,
+                   const char* expected_response) {
+    base::MockCallback<AdbClientSocket::CommandCallback> callback;
+    EXPECT_CALL(callback, Run(expected_result_code, expected_response));
+    AdbClientSocket::ParseOutput(has_length, callback.Get(),
+                                 std::string(adb_output));
+  }
+
+  void TestReadUntilEOF_EOF(const char* data_on_buffer) {
+    std::unique_ptr<MockSocket> socket = std::make_unique<MockSocket>();
+    AdbClientSocket adb_socket(3);
+
+    base::MockCallback<AdbClientSocket::ParserCallback> parse_callback;
+    EXPECT_CALL(parse_callback, Run(data_on_buffer));
+
+    base::MockCallback<AdbClientSocket::CommandCallback> response_callback;
+    // The following means "expect not to be called."
+    EXPECT_CALL(response_callback, Run(0, "")).Times(0);
+
+    scoped_refptr<net::GrowableIOBuffer> buffer = new net::GrowableIOBuffer;
+    buffer->SetCapacity(100);
+    strcpy(buffer->data(), data_on_buffer);
+    buffer->set_offset(strlen(data_on_buffer));
+
+    adb_socket.ReadUntilEOF(parse_callback.Get(), response_callback.Get(),
+                            buffer, 0);
+  }
+
+  void TestReadUntilEOF_Error() {
+    int error_code = -1;
+    std::unique_ptr<MockSocket> socket = std::make_unique<MockSocket>();
+    // 3 is an arbitrary meaningless number in the following call.
+    AdbClientSocket adb_socket(3);
+
+    base::MockCallback<AdbClientSocket::ParserCallback> parse_callback;
+    // The following means "expect not to be called."
+    EXPECT_CALL(parse_callback, Run("")).Times(0);
+
+    base::MockCallback<AdbClientSocket::CommandCallback> response_callback;
+    EXPECT_CALL(response_callback, Run(error_code, "IO error")).Times(1);
+    scoped_refptr<net::GrowableIOBuffer> buffer = new net::GrowableIOBuffer;
+    buffer->SetCapacity(100);
+    adb_socket.ReadUntilEOF(parse_callback.Get(), response_callback.Get(),
+                            buffer, error_code);
+  }
+
+  void TestReadUntilEOF_Recurse(std::string* chunks,
+                                int number_chunks,
+                                std::string expected_result) {
+    // 3 is an arbitrary meaningless number in the following call.
+    AdbClientSocket adb_socket(3);
+    adb_socket.socket_.reset(new MockSocket(chunks, number_chunks));
+
+    base::MockCallback<AdbClientSocket::ParserCallback> parse_callback;
+    EXPECT_CALL(parse_callback, Run(expected_result.c_str())).Times(1);
+
+    base::MockCallback<AdbClientSocket::CommandCallback> response_callback;
+    // The following means "expect not to be called."
+    EXPECT_CALL(response_callback, Run(0, "")).Times(0);
+    scoped_refptr<net::GrowableIOBuffer> buffer = new net::GrowableIOBuffer;
+    int initial_capacity = 4;
+    buffer->SetCapacity(initial_capacity);
+    int result = adb_socket.socket_->Read(
+        buffer.get(), initial_capacity,
+        base::Bind(&AdbClientSocket::ReadUntilEOF,
+                   base::Unretained(&adb_socket), parse_callback.Get(),
+                   response_callback.Get(), buffer));
+    if (result != net::ERR_IO_PENDING) {
+      adb_socket.ReadUntilEOF(parse_callback.Get(), response_callback.Get(),
+                              buffer, result);
+    }
+  }
+  void TestReadStatus(const char* buffer_data,
+                      int result,
+                      const char* expected_result_string,
+                      int expected_result_code) {
+    base::MockCallback<AdbClientSocket::ParserCallback> parse_callback;
+    // The following means "expect not to be called."
+    EXPECT_CALL(parse_callback, Run("")).Times(0);
+
+    base::MockCallback<AdbClientSocket::CommandCallback> response_callback;
+    EXPECT_CALL(response_callback,
+                Run(expected_result_code, expected_result_string))
+        .Times(1);
+
+    scoped_refptr<net::GrowableIOBuffer> buffer = new net::GrowableIOBuffer;
+    int initial_capacity = 100;
+    buffer->SetCapacity(initial_capacity);
+    if (result > 0) {
+      strncpy(buffer->data(), buffer_data, result);
+    }
+    AdbClientSocket::ReadStatusOutput(response_callback.Get(), buffer, result);
+  }
+};
+
+TEST_F(AdbClientSocketTest, ParseOutput) {
+  TestParsing("OKAY000512345", true, 0, "12345");
+  TestParsing("FAIL000512345", true, 1, "12345");
+  TestParsing("OKAY12345", false, 0, "12345");
+  TestParsing("FAIL12345", false, 1, "12345");
+  TestParsing("ABC", false, 1, "ABC");
+  TestParsing("ABC", true, 1, "ABC");
+  TestParsing("OKAYAB", false, 0, "AB");
+  TestParsing("OKAYAB", true, 1, "AB");
+  TestParsing("OKAYOKAYOKAY", false, 0, "OKAY");
+  TestParsing("", false, 1, "");
+  TestParsing("", true, 1, "");
+  TestParsing("OKAYOKAY", true, 0, "");
+  TestParsing("OKAYOKAY", false, 0, "");
+}
+
+TEST_F(AdbClientSocketTest, ReadUntilEOF_EOFEmptyString) {
+  TestReadUntilEOF_EOF("");
+}
+TEST_F(AdbClientSocketTest, ReadUntilEOF_EOFNonEmptyString1) {
+  TestReadUntilEOF_EOF("blah");
+}
+TEST_F(AdbClientSocketTest, ReadUntilEOF_EOFNonEmptyString2) {
+  TestReadUntilEOF_EOF("blahbla");
+}
+
+TEST_F(AdbClientSocketTest, ReadUntilEOF_Error) {
+  TestReadUntilEOF_Error();
+}
+TEST_F(AdbClientSocketTest, ReadUntilEOF_GrowBuffer) {
+  std::string chunks[7] = {"This", "",     " data",   " should",
+                           "",     " be ", "read in."};
+  TestReadUntilEOF_Recurse(chunks, 7, "This data should be read in.");
+}
+TEST_F(AdbClientSocketTest, ReadUntilEOF_EmptyChunks) {
+  std::string chunks[5] = {"", "", "", "", ""};
+  TestReadUntilEOF_Recurse(chunks, 5, "");
+}
+TEST_F(AdbClientSocketTest, ReadUntilEOF_Empty) {
+  std::string chunks[0] = {};
+  TestReadUntilEOF_Recurse(chunks, 0, "");
+}
+TEST_F(AdbClientSocketTest, ReadUntilEOF_EmptyEndingChunk) {
+  std::string chunks[2] = {"yeah", ""};
+  TestReadUntilEOF_Recurse(chunks, 2, "yeah");
+}
+
+TEST_F(AdbClientSocketTest, ReadStatusOutput_Okay) {
+  TestReadStatus("OKAY", 4, "OKAY", 0);
+}
+TEST_F(AdbClientSocketTest, ReadStatusOutput_OkayNulls) {
+  TestReadStatus("OKAY\0\0\0\0", 8, "OKAY", 0);
+}
+TEST_F(AdbClientSocketTest, ReadStatusOutput_Fail) {
+  TestReadStatus("FAIL", 4, "FAIL", 1);
+}
+TEST_F(AdbClientSocketTest, ReadStatusOutput_FailEmpty) {
+  TestReadStatus("", 0, "FAIL", 1);
+}