Kill devtools forwarded port at device->TearDown

After running chromedriver with Android Chrome, there will be
a listening port not being cleaned up. This commit addresses the
issue by calling `adb forward --remove` call.

Bug: chromedriver:2421
Change-Id: I606aab3154bc7e5a7cf587de5fd82c9894d1bec6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385036
Reviewed-by: Shengfa Lin <shengfa@google.com>
Reviewed-by: John Chen <johnchen@chromium.org>
Commit-Queue: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806405}

AUTHORS

@@ -659,6 +659,7 @@ Martin Rogalla <martin@martinrogalla.com>
 Martina Kollarova <martina.kollarova@intel.com>
 Masahiro Yado <yado.masa@gmail.com>
 Masaru Nishida <msr.i386@gmail.com>
+Masayuki Wakizaka <mwakizaka0108@gmail.com>
 Matej Knopp <matej.knopp@gmail.com>
 Matheus Bratfisch <matheusbrat@gmail.com>
 Mathias Bynens <mathias@qiwi.be>

chrome/test/chromedriver/chrome/adb.h

@@ -18,6 +18,8 @@ class Adb {
   virtual Status ForwardPort(const std::string& device_serial,
                              const std::string& remote_abstract,
                              int* local_port_output) = 0;
+  virtual Status KillForwardPort(const std::string& device_serial,
+                                 int port) = 0;
   virtual Status SetCommandLineFile(const std::string& device_serial,
                                     const std::string& command_line_file,
                                     const std::string& exec_name,

chrome/test/chromedriver/chrome/adb_impl.cc

@@ -169,6 +169,22 @@ Status AdbImpl::ForwardPort(const std::string& device_serial,
   return Status(kOk);
 }
 
+Status AdbImpl::KillForwardPort(const std::string& device_serial,
+                                int port) {
+  std::string response;
+  Status adb_command_status = ExecuteHostCommand(
+      device_serial, "killforward:tcp:" + base::NumberToString(port),
+      &response);
+  if (adb_command_status.IsError())
+    return Status(kUnknownError, "Failed to kill forward port of device " +
+                                     device_serial + ": " +
+                                     base::NumberToString(port) + ": " +
+                                     response + ". " +
+                                     adb_command_status.message());
+  return Status(kOk);
+}
+
+
 Status AdbImpl::SetCommandLineFile(const std::string& device_serial,
                                    const std::string& command_line_file,
                                    const std::string& exec_name,

chrome/test/chromedriver/chrome/adb_impl.h

@@ -30,6 +30,8 @@ class AdbImpl : public Adb {
   Status ForwardPort(const std::string& device_serial,
                      const std::string& remote_abstract,
                      int* local_port_output) override;
+  Status KillForwardPort(const std::string& device_serial,
+                         int port) override;
   Status SetCommandLineFile(const std::string& device_serial,
                             const std::string& command_line_file,
                             const std::string& exec_name,

chrome/test/chromedriver/chrome/device_manager.cc

@@ -190,7 +190,10 @@ Status Device::ForwardDevtoolsPort(const std::string& package,
     *device_socket = socket_name.substr(1);
   }
 
-  return adb_->ForwardPort(serial_, *device_socket, devtools_port);
+  Status status = adb_->ForwardPort(serial_, *device_socket, devtools_port);
+  if (status.IsOk())
+    devtools_port_ = *devtools_port;
+  return status;
 }
 
 Status Device::TearDown() {
@@ -201,6 +204,12 @@ Status Device::TearDown() {
       return status;
     active_package_ = "";
   }
+  if (devtools_port_ != 0) {
+    Status status = adb_->KillForwardPort(serial_, devtools_port_);
+    if (status.IsError())
+      return status;
+    devtools_port_ = 0;
+  }
   return Status(kOk);
 }
 

chrome/test/chromedriver/chrome/device_manager.h

@@ -49,6 +49,7 @@ class Device {
   const std::string serial_;
   std::string active_package_;
   Adb* adb_;
+  int devtools_port_ = 0;
   base::OnceCallback<void()> release_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(Device);

chrome/test/chromedriver/chrome/device_manager_unittest.cc

@@ -32,6 +32,11 @@ class FakeAdb : public Adb {
     return Status(kOk);
   }
 
+  Status KillForwardPort(const std::string& device_serial,
+                         int port) override {
+    return Status(kOk);
+  }
+
   Status SetCommandLineFile(const std::string& device_serial,
                             const std::string& command_line_file,
                             const std::string& exec_name,
@@ -68,7 +73,7 @@ class FakeAdb : public Adb {
   Status GetPidByName(const std::string& device_serial,
                       const std::string& process_name,
                       int* pid) override {
-    *pid = 0; // avoid uninit error crbug.com/393231
+    *pid = 0;  // avoid uninit error crbug.com/393231
     return Status(kOk);
   }
 
@@ -80,6 +85,45 @@ class FakeAdb : public Adb {
   }
 };
 
+class SucceedsForwardPortFakeAdb : public FakeAdb {
+ public:
+  SucceedsForwardPortFakeAdb() {}
+  ~SucceedsForwardPortFakeAdb() override {}
+
+  Status ForwardPort(const std::string& device_serial,
+                     const std::string& remote_abstract,
+                     int* local_port) override {
+    *local_port = 1;
+    return Status(kOk);
+  }
+
+  Status KillForwardPort(const std::string& device_serial,
+                         int port) override {
+    kill_forward_port_is_called_ = true;
+    return Status(kOk);
+  }
+
+  bool KillForwardPortIsCalled() {
+    return kill_forward_port_is_called_;
+  }
+ private:
+  bool kill_forward_port_is_called_ = false;
+};
+
+class FailsForwardPortFakeAdb : public SucceedsForwardPortFakeAdb {
+ public:
+  FailsForwardPortFakeAdb() {}
+  ~FailsForwardPortFakeAdb() override {}
+
+  Status ForwardPort(const std::string& device_serial,
+                     const std::string& remote_abstract,
+                     int* local_port) override {
+    *local_port = 1;
+    return Status(kUnknownError);
+  }
+};
+
+
 }  // namespace
 
 TEST(DeviceManager, AcquireDevice) {
@@ -112,39 +156,65 @@ TEST(DeviceManager, AcquireSpecificDevice) {
 }
 
 TEST(Device, StartStopApp) {
+  int devtools_port;
   FakeAdb adb;
   DeviceManager device_manager(&adb);
   std::unique_ptr<Device> device1;
   ASSERT_TRUE(device_manager.AcquireDevice(&device1).IsOk());
   ASSERT_TRUE(device1->TearDown().IsOk());
   ASSERT_TRUE(
-      device1->SetUp("a.chrome.package", "", "", "", "", "", false, 0).IsOk());
+      device1->SetUp("a.chrome.package", "", "",
+                     "", "", "", false, &devtools_port).IsOk());
   ASSERT_FALSE(
-      device1->SetUp("a.chrome.package", "", "", "", "", "", false, 0).IsOk());
+      device1->SetUp("a.chrome.package", "", "",
+                     "", "", "", false, &devtools_port).IsOk());
   ASSERT_TRUE(device1->TearDown().IsOk());
   ASSERT_FALSE(
-      device1
-          ->SetUp("a.chrome.package", "an.activity", "", "", "", "", false, 0)
-          .IsOk());
+      device1->SetUp("a.chrome.package", "an.activity", "",
+                     "", "", "", false, &devtools_port).IsOk());
   ASSERT_FALSE(
-      device1->SetUp("a.package", "", "", "", "", "", false, 0).IsOk());
+      device1->SetUp("a.package", "", "",
+                     "", "", "", false, &devtools_port).IsOk());
   ASSERT_TRUE(
-      device1->SetUp("a.package", "an.activity", "", "", "", "", false, 0)
-          .IsOk());
+      device1->SetUp("a.package", "an.activity", "",
+                     "", "", "", false, &devtools_port).IsOk());
   ASSERT_TRUE(device1->TearDown().IsOk());
   ASSERT_TRUE(
-      device1
-          ->SetUp("a.package", "an.activity", "a.process", "", "", "", false, 0)
-          .IsOk());
+      device1->SetUp("a.package", "an.activity", "a.process",
+                     "", "", "", false, &devtools_port).IsOk());
   ASSERT_TRUE(device1->TearDown().IsOk());
-  ASSERT_TRUE(device1
-                  ->SetUp("a.package", "an.activity", "a.process",
-                          "a.deviceSocket", "", "", false, 0)
-                  .IsOk());
+  ASSERT_TRUE(
+      device1->SetUp("a.package", "an.activity", "a.process",
+                     "a.deviceSocket", "", "", false,
+                     &devtools_port).IsOk());
   ASSERT_TRUE(device1->TearDown().IsOk());
-  ASSERT_TRUE(device1
-                  ->SetUp("a.package", "an.activity", "a.process",
-                          "a.deviceSocket", "an.execName", "", false, 0)
-                  .IsOk());
+  ASSERT_TRUE(
+      device1->SetUp("a.package", "an.activity", "a.process",
+                     "a.deviceSocket", "an.execName", "", false,
+                     &devtools_port).IsOk());
   ASSERT_TRUE(device1->TearDown().IsOk());
 }
+
+TEST(ForwardPort, Success) {
+  int devtools_port;
+  SucceedsForwardPortFakeAdb adb;
+  DeviceManager device_manager(&adb);
+  std::unique_ptr<Device> device1;
+  device_manager.AcquireDevice(&device1);
+  device1->SetUp("a.chrome.package", "", "",
+                 "", "", "", false, &devtools_port);
+  device1->TearDown();
+  ASSERT_TRUE(adb.KillForwardPortIsCalled());
+}
+
+TEST(ForwardPort, Failure) {
+  int devtools_port;
+  FailsForwardPortFakeAdb adb;
+  DeviceManager device_manager(&adb);
+  std::unique_ptr<Device> device1;
+  device_manager.AcquireDevice(&device1);
+  device1->SetUp("a.package", "an.activity", "",
+                 "", "", "", false, &devtools_port);
+  device1->TearDown();
+  ASSERT_FALSE(adb.KillForwardPortIsCalled());
+}