Return a valid debuggerAddress wherever expected

In all situations when use of 'remote-debugging-pipe' only is not
explicitly requested the user expects to receive 'debuggerAddress'
in the returned capabilities. This information is used by Selenium
for establishing of CDP-based BiDi communication.
In this commit we make sure that this data is returned in all the
aforementioned cases.

Bug: chromedriver:4533
Change-Id: If9ac6704f7abc7308c3a8db1a912cf650d4161ec
Validate-Test-Flakiness: skip
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4789068
Reviewed-by: Thiago Perrotta <tperrotta@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Commit-Queue: Vladimir Nechaev <nechaev@chromium.org>
Auto-Submit: Vladimir Nechaev <nechaev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185886}

chrome/test/chromedriver/chrome_launcher.cc

@@ -163,13 +163,26 @@ Status PrepareDesktopCommandLine(const Capabilities& capabilities,
     switches.RemoveSwitch(excluded_switch);
   }
   switches.SetFromSwitches(capabilities.switches);
+  // There are two special cases concerning the choice of the transport layer
+  // between ChromeDriver and Chrome:
+  // * Neither 'remote-debugging-port' nor 'remote-debugging-pipe'is provided.
+  // * Both 'remote-debugging-port' and 'remote-debugging-pipe' are provided.
+  // They are treated as 'up to ChromeDriver to choose the transport layer'.
+  // Due to historical reasons their contract must be:
+  // * 'debuggerAddress' returned to the user must contain an HTTP endpoint
+  //    of the form 'ip:port' and behaving as the browser endpoint for handling
+  //    of http requests like /json/version.
+  // This contract is relied upon by Selenium for CDP based BiDi until its
+  // support is discontinued.
+  // For now we are opting to 'remote-debugging-port' as the easiest solution
+  // satisfying this requirement.
+  if (switches.HasSwitch("remote-debugging-port") &&
+      switches.HasSwitch("remote-debugging-pipe")) {
+    switches.RemoveSwitch("remote-debugging-pipe");
+  }
   if (!switches.HasSwitch("remote-debugging-port") &&
       !switches.HasSwitch("remote-debugging-pipe")) {
-    if (PipeBuilder::PlatformIsSupported()) {
-      switches.SetSwitch("remote-debugging-pipe");
-    } else {
-      switches.SetSwitch("remote-debugging-port", "0");
-    }
+    switches.SetSwitch("remote-debugging-port", "0");
   }
   if (capabilities.exclude_switches.count("user-data-dir") > 0) {
     LOG(WARNING) << "excluding user-data-dir switch is not supported";

chrome/test/chromedriver/client/chromedriver.py

@@ -103,7 +103,7 @@ def __init__(self, server_url, server_pid, **kwargs):
             for p in processes:
               p.terminate()
 
-            gone, alive = psutil.wait_procs(processes, timeout=3)
+            _, alive = psutil.wait_procs(processes, timeout=3)
             if len(alive):
               print('Killing', len(alive), 'processes')
               for p in alive:
@@ -136,6 +136,7 @@ def _InternalInit(self, server_url,
     self._executor = command_executor.CommandExecutor(server_url)
     self._server_url = server_url
     self.w3c_compliant = False
+    self.debuggerAddress = None
 
     options = {}
 
@@ -208,7 +209,6 @@ def _InternalInit(self, server_url,
       assert type(devtools_events_to_log) is list
       options['devToolsEventsToLog'] = devtools_events_to_log
 
-    download_prefs = {}
     if download_dir:
       if 'prefs' not in options:
         options['prefs'] = {}
@@ -262,8 +262,10 @@ def _InternalInit(self, server_url,
       self.w3c_compliant = True
       self._session_id = response['value']['sessionId']
       self.capabilities = self._UnwrapValue(response['value']['capabilities'])
-      self.debuggerAddress = str(
-          self.capabilities['goog:chromeOptions']['debuggerAddress'])
+      if ('goog:chromeOptions' in self.capabilities
+          and 'debuggerAddress' in self.capabilities['goog:chromeOptions']):
+          self.debuggerAddress = str(
+              self.capabilities['goog:chromeOptions']['debuggerAddress'])
     elif isinstance(response['status'], int):
       self.w3c_compliant = False
       self._session_id = response['sessionId']

chrome/test/chromedriver/session_commands.cc

@@ -223,11 +223,13 @@ base::Value::Dict CreateCapabilities(Session* session,
       "%s.%sVersion", base::ToLowerASCII(kBrowserShortName).c_str(),
       base::ToLowerASCII(kChromeDriverProductShortName).c_str());
   caps.SetByDottedPath(chrome_driver_version_key, kChromeDriverVersion);
-  const std::string debugger_address_key =
-      base::StringPrintf("%s.debuggerAddress", kChromeDriverOptionsKeyPrefixed);
-  caps.SetByDottedPath(debugger_address_key, session->chrome->GetBrowserInfo()
-                                                 ->debugger_endpoint.Address()
-                                                 .ToString());
+  if (session->chrome->GetBrowserInfo()->debugger_endpoint.IsValid()) {
+    const std::string debugger_address_key = base::StringPrintf(
+        "%s.debuggerAddress", kChromeDriverOptionsKeyPrefixed);
+    caps.SetByDottedPath(debugger_address_key, session->chrome->GetBrowserInfo()
+                                                   ->debugger_endpoint.Address()
+                                                   .ToString());
+  }
   ChromeDesktopImpl* desktop = nullptr;
   Status status = session->chrome->GetAsDesktop(&desktop);
   if (status.IsOk()) {

chrome/test/chromedriver/test/run_py_tests.py

@@ -13,6 +13,7 @@
 
 import base64
 import codecs
+import http.client
 import imghdr
 import json
 import math
@@ -4977,6 +4978,20 @@ class ChromeSwitchesCapabilityTest(ChromeDriverBaseTest):
   Makes sure the switches are passed to Chrome.
   """
 
+  def assertDebuggerAddressIsConfigured(self, driver):
+    self.assertIsNotNone(driver.debuggerAddress)
+    self.assertIsInstance(driver.debuggerAddress, str)
+    self.assertNotEqual('', driver.debuggerAddress)
+    # The check detects the regression of https://crbug.com/chromedriver/4533
+    self.assertNotEqual(':-1', driver.debuggerAddress)
+    components = driver.debuggerAddress.split(':')
+    conn = http.client.HTTPConnection(components[0], components[1])
+    conn.request('GET', '/json/version')
+    resp = conn.getresponse()
+    resp = json.loads(resp.read())
+    self.assertIn('Browser', resp)
+    self.assertIn('webSocketDebuggerUrl', resp)
+
   def testSwitchWithoutArgument(self):
     """Tests that switch --dom-automation can be passed to Chrome.
 
@@ -5034,6 +5049,42 @@ def testUnspportedRemoteDebuggingPipe(self):
                                 "only ASCIIZ protocol mode is supported"):
       self.CreateDriver(chrome_switches=["remote-debugging-pipe=xyz"])
 
+  def testDebuggerAddressByDefault(self):
+    """Tests that capabilities returned by the "New Session" command contain
+    a workable debuggerAddress url.
+    """
+    driver = self.CreateDriver()
+    self.assertDebuggerAddressIsConfigured(driver)
+
+  def testDebuggerAddressForRemoteDebuggingPort(self):
+    """Tests that capabilities returned by the "New Session" command contain
+    a workable debuggerAddress url. This modification explicitly enables
+    communication over web sockets.
+    """
+    driver = self.CreateDriver(chrome_switches=['remote-debugging-port=0'])
+    self.assertDebuggerAddressIsConfigured(driver)
+
+  def testDebuggerAddressForRemoteDebuggingPipe(self):
+    """Tests that capabilities returned by the "New Session" command do not
+    contain a debuggerAddress url. This modification explicitly enables
+    communication over pipes.
+    The requirement of no debuggerAddress in the capabilities can be changed in
+    the future if we decide to mimic the http endpoint of the browser on the
+    ChromeDriver side.
+    """
+    driver = self.CreateDriver(chrome_switches=['remote-debugging-pipe'])
+    self.assertIsNone(driver.debuggerAddress)
+
+  def testDebuggerAddressForRemoteDebuggingPortAndPipe(self):
+    """Tests that capabilities returned by the "New Session" command contain
+    a valid debuggerAddress url. This modification explicitly enables
+    communication over web sockets and pipes.
+    """
+    driver = self.CreateDriver(chrome_switches=[
+        'remote-debugging-pipe',
+        'remote-debugging-port=0'])
+    self.assertDebuggerAddressIsConfigured(driver)
+
 
 class ChromeDesiredCapabilityTest(ChromeDriverBaseTest):
   """Tests that chromedriver properly processes desired capabilities."""