[Android] Redirect std{in,out,err} to sockets for layout tests.

content/shell/browser/layout_test/layout_test_android.cc

 // Copyright 2013 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 "content/shell/browser/layout_test/layout_test_android.h"
 
+#include <fcntl.h>
+#include <iostream>
 #include <memory>
 
 #include "base/android/context_utils.h"
 #include "base/android/fifo_utils.h"

[Peter Beverloo, 2016/12/06 19:24:06]

The entirety of fifo_utils (and implementation) are now unused! Mind deleting
them? :)

[jbudorick, 2016/12/08 02:04:13]

Done.

 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/test/test_support_android.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/test/nested_message_pump_android.h"
+#include "content/shell/browser/layout_test/blink_test_controller.h"
+#include "content/shell/common/layout_test/layout_test_switches.h"
 #include "content/shell/common/shell_switches.h"
 #include "jni/ShellLayoutTestUtils_jni.h"
+#include "net/base/ip_address.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
+#include "net/base/sockaddr_storage.h"
+#include "net/socket/socket_posix.h"
 #include "url/gurl.h"
 
 using base::android::ScopedJavaLocalRef;
 
 namespace {
 
-base::FilePath GetTestFilesDirectory(JNIEnv* env) {
-  ScopedJavaLocalRef<jstring> directory =
-      content::Java_ShellLayoutTestUtils_getApplicationFilesDirectory(
-          env, base::android::GetApplicationContext());
-  return base::FilePath(ConvertJavaStringToUTF8(directory));
-}
-
-void EnsureCreateFIFO(const base::FilePath& path) {
-  unlink(path.value().c_str());
-  CHECK(base::android::CreateFIFO(path, 0666))
-    << "Unable to create the Android's FIFO: " << path.value().c_str();
-}
-
 std::unique_ptr<base::MessagePump> CreateMessagePumpForUI() {
   return std::unique_ptr<base::MessagePump>(
       new content::NestedMessagePumpAndroid());
 }
 
 }  // namespace
 
 namespace content {
 
-void EnsureInitializeForAndroidLayoutTests() {
+ScopedAndroidConfiguration::ScopedAndroidConfiguration() : sockets_() {}

[Peter Beverloo, 2016/12/06 19:24:06]

nit - no need to explicitly construct |sockets_|. The compiler does that for
you. I'd use:

  ScopedAndroidConfiguration::ScopedAndroidConfiguration() = default;

[jbudorick, 2016/12/08 02:04:13]

Done.

+
+void ConnectCompleted(base::Closure socket_connected, int rv) {

[Peter Beverloo, 2016/12/06 19:24:06]

nit: please keep the functions that aren't declared in the header file in the
anonymous namespace. That avoids accidental namespace pollution.

(It's not unreasonable that somebody else may also try to define e.g.
content::RedirectStream(), causing odd linking errors.)

[jbudorick, 2016/12/08 02:04:13]

Done.

+  CHECK_EQ(net::OK, rv) << " Failed to redirect to socket: "
+                        << net::ErrorToString(rv);
+  socket_connected.Run();
+}
+
+void CreateAndConnectSocket(
+    uint16_t port,
+    base::Callback<void(std::unique_ptr<net::SocketPosix>)> socket_open,
+    base::Callback<void(int)> socket_connected) {

[Peter Beverloo, 2016/12/06 19:24:06]

nit - avoid the base::Callback refcount churn by marking the base::Callback
parameters as const& (here and elsewhere)

[jbudorick, 2016/12/08 02:04:13]

Done.

+  net::SockaddrStorage storage;
+  net::IPAddress address;
+  CHECK(address.AssignFromIPLiteral("127.0.0.1"))
+      << "Failed to create IPAddress from IP literal 127.0.0.1.";
+  net::IPEndPoint endpoint(address, port);
+  CHECK(endpoint.ToSockAddr(storage.addr, &storage.addr_len))
+      << "Failed to convert " << endpoint.ToString() << " to sockaddr.";
+
+  std::unique_ptr<net::SocketPosix> socket(new net::SocketPosix);
+
+  int result = net::OK;
+  result = socket->Open(AF_INET);

[Peter Beverloo, 2016/12/06 19:24:06]

nit - you can combine these lines:

  int result = socket->Open(AF_INET);

[jbudorick, 2016/12/08 02:04:13]

I imagine there was some reason I had them split at some point ... at least, I
hope there was.

Done.

+  CHECK_EQ(net::OK, result) << "Failed to open socket for "
+                            << endpoint.ToString() << ": "
+                            << net::ErrorToString(result);
+
+  // Set the socket as blocking.
+  const int flags = fcntl(socket->socket_fd(), F_GETFL);

[Peter Beverloo, 2016/12/06 19:24:06]

nit - check for the error condition for the fcntl calls:

  CHECK_NE(flags, -1);

[jbudorick, 2016/12/08 02:04:13]

Done.

+  if (flags & O_NONBLOCK) {
+    fcntl(socket->socket_fd(), F_SETFL, flags & ~O_NONBLOCK);
+  }
+
+  net::CompletionCallback connect_completed = base::Bind(
+      &ConnectCompleted, base::Bind(socket_connected, socket->socket_fd()));
+  result = socket->Connect(storage, connect_completed);
+  if (result != net::ERR_IO_PENDING) {

[Peter Beverloo, 2016/12/06 19:24:06]

nit - what about other values for |result|? Should we CHECK that |result| is
either net::ERR_IO_PENDING or net::OK?

[jbudorick, 2016/12/08 02:04:13]

These currently get checked in the callback s.t. they get handled in either
case.

+    connect_completed.Run(result);
+  }
+  socket_open.Run(std::move(socket));
+}
+
+void RedirectStdout(int fd) {
+  CHECK_NE(dup2(fd, STDOUT_FILENO), -1) << "Failed to dup2 stdout: "
+                                        << strerror(errno);
+}
+
+void RedirectStdin(int fd) {
+  CHECK_NE(dup2(fd, STDIN_FILENO), -1) << "Failed to dup2 stdin: "
+                                       << strerror(errno);
+}
+
+void RedirectStderr(int fd) {
+  CHECK_NE(dup2(fd, STDERR_FILENO), -1) << "Failed to dup2 stderr: "
+                                        << strerror(errno);
+}
+
+void FinishRedirection(base::Callback<void(int)> redirect,
+                       base::WaitableEvent* event,
+                       int fd) {
+  redirect.Run(fd);
+  event->Signal();
+}
+
+void RedirectStream(
+    uint16_t port,
+    base::Callback<void(std::unique_ptr<net::SocketPosix>)> socket_open,
+    base::Callback<void(base::WaitableEvent*, int)> finish_redirection) {
+  base::WaitableEvent redirected(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&CreateAndConnectSocket, port, socket_open,
+                 base::Bind(finish_redirection, &redirected)));
+  content::ScopedAllowWaitForAndroidLayoutTests allow_wait;

[Peter Beverloo, 2016/12/06 19:24:06]

micro nit (4x in this file) - you're in content::, no need to specifically
qualify that

[jbudorick, 2016/12/08 02:04:13]

Done.

+  while (!redirected.IsSignaled())
+    redirected.Wait();
+}
+
+void ScopedAndroidConfiguration::RedirectStreams() {
+  base::Callback<void(std::unique_ptr<net::SocketPosix>)> add_socket =
+      base::Bind(&ScopedAndroidConfiguration::AddSocket,
+                 base::Unretained(this));

[Peter Beverloo, 2016/12/06 19:24:06]

nit: add a comment explaining that it's safe to use base::Unretained() because
the code's executed "synchronously".

[jbudorick, 2016/12/08 02:04:12]

Done.

+
+  std::string stdout_port_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueNative(
+          switches::kAndroidStdoutPort);
+  unsigned stdout_port = 0;
+  if (base::StringToUint(stdout_port_str, &stdout_port)) {
+    RedirectStream(base::checked_cast<uint16_t>(stdout_port), add_socket,
+                   base::Bind(&FinishRedirection, base::Bind(&RedirectStdout)));
+  }
+
+  std::string stdin_port_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueNative(
+          switches::kAndroidStdinPort);
+  unsigned stdin_port = 0;
+  if (base::StringToUint(stdin_port_str, &stdin_port)) {
+    RedirectStream(base::checked_cast<uint16_t>(stdin_port), add_socket,
+                   base::Bind(&FinishRedirection, base::Bind(&RedirectStdin)));
+  }
+
+  std::string stderr_port_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueNative(
+          switches::kAndroidStderrPort);
+  unsigned stderr_port = 0;
+  if (base::StringToUint(stderr_port_str, &stderr_port)) {
+    RedirectStream(base::checked_cast<uint16_t>(stderr_port), add_socket,
+                   base::Bind(&FinishRedirection, base::Bind(&RedirectStderr)));
+  }
+}
+
+void ScopedAndroidConfiguration::AddSocket(
+    std::unique_ptr<net::SocketPosix> socket) {
+  sockets_.push_back(std::move(socket));
+}
+
+ScopedAndroidConfiguration::~ScopedAndroidConfiguration() {}
+
+void ScopedAndroidConfiguration::Initialize() {
   JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jstring> jtest_data_dir =
+      content::Java_ShellLayoutTestUtils_getIsolatedTestRoot(env);
+  base::FilePath test_data_dir(
+      base::android::ConvertJavaStringToUTF8(env, jtest_data_dir));
+  base::InitAndroidTestPaths(test_data_dir);
 
-  bool success = base::MessageLoop::InitMessagePumpForUIFactory(
-      &CreateMessagePumpForUI);
+  bool success =
+      base::MessageLoop::InitMessagePumpForUIFactory(&CreateMessagePumpForUI);
   CHECK(success) << "Unable to initialize the message pump for Android.";
-
-  // Android will need three FIFOs to communicate with the Blink test runner,
-  // one for each of [stdout, stderr, stdin].
-  base::FilePath files_dir(GetTestFilesDirectory(env));
-
-  base::FilePath stdout_fifo(files_dir.Append(FILE_PATH_LITERAL("test.fifo")));
-  EnsureCreateFIFO(stdout_fifo);
-
-  base::FilePath stderr_fifo(
-      files_dir.Append(FILE_PATH_LITERAL("stderr.fifo")));
-  EnsureCreateFIFO(stderr_fifo);
-
-  base::FilePath stdin_fifo(files_dir.Append(FILE_PATH_LITERAL("stdin.fifo")));
-  EnsureCreateFIFO(stdin_fifo);
-
-  // Redirecting stdout needs to happen before redirecting stdin, which needs
-  // to happen before redirecting stderr.
-  success = base::android::RedirectStream(stdout, stdout_fifo, "w") &&
-            base::android::RedirectStream(stdin, stdin_fifo, "r") &&
-            base::android::RedirectStream(stderr, stderr_fifo, "w");
-
-  CHECK(success) << "Unable to initialize the Android FIFOs.";
 }
 
 }  // namespace content