Fix Chromedriver Issue 749

chrome/test/chromedriver/chrome/adb_impl.cc

Index: chrome/test/chromedriver/chrome/adb_impl.cc
diff --git a/chrome/test/chromedriver/chrome/adb_impl.cc b/chrome/test/chromedriver/chrome/adb_impl.cc
index a60d8bce5dfaf31e0242ac49126df9da868effa1..512027464cbb7ce8bf440c27c36156f158a24f7e 100644
--- a/chrome/test/chromedriver/chrome/adb_impl.cc
+++ b/chrome/test/chromedriver/chrome/adb_impl.cc
@@ -226,6 +226,50 @@ Status AdbImpl::GetPidByName(const std::string& device_serial,
                 "Failed to get PID for the following process: " + process_name);
 }
 
+Status AdbImpl::GetDevtoolsRemoteSocket(const std::string& device_serial,
+                                        const std::string& process_name,
+                                        std::string* device_socket) {
+  // Parse 'cat /proc/net/unix' output which on Android looks like this:
+  //
+  // Num       RefCount Protocol Flags    Type St Inode Path
+  // 00000000: 00000002 00000000 00010000 0001 01 331813 /dev/socket/zygote
+  // 00000000: 00000002 00000000 00010000 0001 01 358606 @xxx_devtools_remote
+  // 00000000: 00000002 00000000 00010000 0001 01 347300 @yyy_devtools_remote
+  //
+  // We need to find records with paths starting from '@' (abstract socket)
+  // and containing the channel pattern ("_devtools_remote").
+
+  std::string response;
+  Status status = ExecuteHostShellCommand(device_serial, "cat /proc/net/unix", &response);

[samuong, 2016/10/03 18:50:40]

this line is too long (>80 chars)

[Rahul Kavalapara, 2016/10/05 08:06:42]

Done.

+  if (!status.IsOk())
+    return status;
+  for (const base::StringPiece& line :
+       base::SplitStringPiece(response, "\n", base::KEEP_WHITESPACE,
+                              base::SPLIT_WANT_NONEMPTY)) {
+    std::vector<std::string> fields =
+        base::SplitString(line, " \r", base::KEEP_WHITESPACE,
+                          base::SPLIT_WANT_NONEMPTY);
+    if (fields.size() < 8)
+      continue;
+    if (fields[3] != "00010000" || fields[5] != "01")

[samuong, 2016/10/03 18:50:40]

it's not really clear to me what these magic numbers/flags mean - can you link
to somewhere that documents the format of this response?

[Rahul Kavalapara, 2016/10/03 22:15:52]

Please check MapSocketsToProcesses function.
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo...

[Rahul Kavalapara, 2016/10/04 01:18:04]

I did some more digging, Similar question was posted here: 
http://unix.stackexchange.com/questions/183140/what-is-the-meaning-of-the-con...
If you want me to document the answer as well please let me know.

Answer:

The code that generates this file is in the unix_seq_show() function in
net/unix/af_unix.c in the kernel source. Looking at include/net/af_unix.h is
also helpful, to see the data structures in use.

The socket path is always the last column in the output, and the Android kernel
source matches the stock kernel in this respect. So unless I'm mistaken, that
number that looks like a column isn't actually a separate column.

You can name UNIX domain sockets practically anything you want, as long as the
total path length is less than 108 bytes. So you can't make any assumptions as
to what these paths will look like. It's possible that the userspace code that's
choosing those names is using a tab character followed by a number, or even
padding the name out to a certain length with spaces. To test my theory, you
might try looking at the socket files in /dev/socket/qmux_radio/.

[samuong, 2016/10/05 17:29:49]

Thanks for digging that up. I was mostly curious about what "flags"="00010000"
and "st"="01" mean, and why we only care about entries with these values. It
looks like "flags" is used to determine whether the socket is accepting
connections, and "st(ate)" is used to determine the unconnected/disconnecting
state. This is based on what I'm reading in the kernel sources:
https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L2795

Can you please document this in the comments?

Also, if the socket name can have whitespace in it, that sounds like something
that needs handling. It's possible that the DevTools code only cares about
devtools sockets, which they can control the naming of. But is the idea here
that we're going to accept arbitrarily named sockets?

[Rahul Kavalapara, 2016/10/05 18:41:53]

This code is essentially exercised targeting android devices. I believe there is
a naming convention for the apps like "com.<companyname>.appname". So in this
scenario I am expecting devtools_socket name to be something like
"com.abc.myapp_devtools_remote". I am highly skeptical on the whitespace issue
if that would happen. We are just getting the string prior to devtools_remote as
that will passed as a chromedriver capabilities value by the end user.

I have documented my findings in comments as well. 

[samuong, 2016/10/12 05:18:35]

OK, that makes sense. However I'd still like to make sure there is a comment
explaining specifically what 'fields[3] != "00010000"' and 'fields[5] != "01"'
mean. Just a one-liner is fine.

[Rahul Kavalapara, 2016/10/12 08:23:04]

Fixed.

+      continue;
+    std::string path_field = fields[7];
+    if (path_field.size() < 1 || path_field[0] != '@')
+      continue;
+    if (path_field.find(process_name) == std::string::npos)

[samuong, 2016/10/03 18:50:40]

what if there are two processes, where one of the process names is a substring
of the other?

[Rahul Kavalapara, 2016/10/03 22:15:52]

This is the adaptation from devtools code. That would be an interesting
scenario.

[samuong, 2016/10/05 17:29:49]

I think we should know the full socket name that we're looking for (correct me
if I'm wrong) so it should be possible to do a full string comparison rather
than looking for a substring?

[Rahul Kavalapara, 2016/10/05 18:41:53]

I am sending back the entire name when trying to connect to the socket. I would
not know the pid of this socket ahead of time if there was one, but just the app
name. So i am comparing only the appname detected from the socket with the what
was passed from the capabilities by the end user.

+      continue;
+
+    *device_socket = path_field.substr(1);
+  }
+
+  // We could not find a device socket for the expected process name
+  if (device_socket->empty()) {
+    return Status(kUnknownError,
+                "Failed to find the remote devtools socket for the following process: " + process_name);

[samuong, 2016/10/03 18:50:40]

this line is too long (>80 chars)

[Rahul Kavalapara, 2016/10/05 08:06:42]

Done.

+  }
+  return Status(kOk);
+}
+
 Status AdbImpl::ExecuteCommand(
     const std::string& command, std::string* response) {
   scoped_refptr<ResponseBuffer> response_buffer = new ResponseBuffer;