In MachMultiprocess-based tests, the child must wait for the parent to

finish.

It was possible for the child process to exit before the parent had a
chance to complete the pid_for_task() portion of its verification.

TEST=util_test
R=rsesek@chromium.org

Review URL: https://codereview.chromium.org/586053002

util/mach/mach_message_server_test.cc

@@ -723,7 +723,6 @@ TEST(MachMessageServer, Complex) {
   // that resources transferred to a server process temporarily aren’t leaked.
   TestMachMessageServer::Options options;
   options.client_send_complex = true;
-  options.child_wait_for_parent_pipe = true;
   TestMachMessageServer test_mach_message_server(options);
   test_mach_message_server.Test();
 }
@@ -740,7 +739,6 @@ TEST(MachMessageServer, ComplexNotDestroyed) {
   options.server_destroy_complex = false;
   options.expect_server_destroyed_complex = false;
   options.client_send_complex = true;
-  options.child_wait_for_parent_pipe = true;
   TestMachMessageServer test_mach_message_server(options);
   test_mach_message_server.Test();
 }
@@ -754,7 +752,6 @@ TEST(MachMessageServer, ComplexDestroyedInvalidArgument) {
   options.server_mig_retcode = KERN_INVALID_TASK;
   options.server_destroy_complex = false;
   options.client_send_complex = true;
-  options.child_wait_for_parent_pipe = true;
   TestMachMessageServer test_mach_message_server(options);
   test_mach_message_server.Test();
 }

util/test/mac/mach_multiprocess.cc

@@ -26,6 +26,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/rand_util.h"
 #include "gtest/gtest.h"
+#include "util/file/fd_io.h"
 #include "util/mach/mach_extensions.h"
 #include "util/misc/scoped_forbid_return.h"
 #include "util/test/errors.h"
@@ -261,6 +262,15 @@ void MachMultiprocess::MultiprocessChild() {
   info_->remote_port.reset();
   info_->local_port.reset();
 
+  // Close the write pipe now, for cases where the parent is waiting on it to
+  // be closed as an indication that the child has finished.
+  CloseWritePipe();
+
+  // Wait for the parent process to close its end of the pipe. The child process
+  // needs to remain alive until then because the parent process will attempt to
+  // verify it using the task port it has access to via ChildTask().
+  CheckedReadFDAtEOF(ReadPipeFD());
+
   if (Test::HasFailure()) {
     // Trigger the ScopedForbidReturn destructor.
     return;

util/test/multiprocess.cc

@@ -22,6 +22,7 @@
 
 #include "base/auto_reset.h"
 #include "base/files/scoped_file.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "gtest/gtest.h"
@@ -113,9 +114,9 @@ void Multiprocess::Run() {
     }
 
     if (reason_ == kTerminationNormal) {
-      message += base::StringPrintf("exit with code %d", code_);
+      message += base::StringPrintf(" exit with code %d", code_);
     } else if (reason == kTerminationSignal) {
-      message += base::StringPrintf("termination by signal %d", code_);
+      message += base::StringPrintf(" termination by signal %d", code_);
     }
 
     if (reason != reason_ || code != code_) {
@@ -159,17 +160,33 @@ pid_t Multiprocess::ChildPID() const {
 int Multiprocess::ReadPipeFD() const {
   int fd = info_->child_pid ? info_->pipe_c2p_read.get()
                             : info_->pipe_p2c_read.get();
-  EXPECT_NE(-1, fd);
+  CHECK_NE(fd, -1);
   return fd;
 }
 
 int Multiprocess::WritePipeFD() const {
   int fd = info_->child_pid ? info_->pipe_p2c_write.get()
                             : info_->pipe_c2p_write.get();
-  EXPECT_NE(-1, fd);
+  CHECK_NE(fd, -1);
   return fd;
 }
 
+void Multiprocess::CloseReadPipe() {
+  if (info_->child_pid) {
+    info_->pipe_c2p_read.reset();
+  } else {
+    info_->pipe_p2c_read.reset();
+  }
+}
+
+void Multiprocess::CloseWritePipe() {
+  if (info_->child_pid) {
+    info_->pipe_p2c_write.reset();
+  } else {
+    info_->pipe_c2p_write.reset();
+  }
+}
+
 void Multiprocess::RunParent() {
   // The parent uses the read end of c2p and the write end of p2c.
   info_->pipe_c2p_write.reset();

util/test/multiprocess.h

@@ -116,6 +116,10 @@ class Multiprocess {
   //! This method may be called by either the parent or the child process.
   //! Anything written to the write pipe in the partner process will appear
   //! on the this file descriptor in this process.
+  //!
+  //! It is an error to call this after CloseReadPipe() has been called.
+  //!
+  //! \return The read pipe’s file descriptor.
   int ReadPipeFD() const;
 
   //! \brief Returns the write pipe’s file descriptor.
@@ -123,8 +127,26 @@ class Multiprocess {
   //! This method may be called by either the parent or the child process.
   //! Anything written to this file descriptor in this process will appear on
   //! the read pipe in the partner process.
+  //!
+  //! It is an error to call this after CloseWritePipe() has been called.
+  //!
+  //! \return The write pipe’s file descriptor.
   int WritePipeFD() const;
 
+  //! \brief Closes the read pipe.
+  //!
+  //! This method may be called by either the parent or the child process. An
+  //! attempt to write to the write pipe in the partner process will fail with
+  //! `EPIPE` or `SIGPIPE`. ReadPipeFD() must not be called after this.
+  void CloseReadPipe();
+
+  //! \brief Closes the write pipe.
+  //!
+  //! This method may be called by either the parent or the child process. An
+  //! attempt to read from the read pipe in the partner process will indicate
+  //! end-of-file. WritePipeFD() must not be called after this.
+  void CloseWritePipe();
+
  private:
   //! \brief Runs the parent side of the test.
   //!

util/test/multiprocess_test.cc

@@ -34,6 +34,8 @@ class TestMultiprocess final : public Multiprocess {
   ~TestMultiprocess() {}
 
  private:
+  // Multiprocess:
+
   virtual void MultiprocessParent() override {
     int read_fd = ReadPipeFD();
     char c;
@@ -99,6 +101,8 @@ class TestMultiprocessUnclean final : public Multiprocess {
     return type_;
   }
 
+  // Multiprocess:
+
   virtual void MultiprocessParent() override {
   }
 
@@ -115,24 +119,176 @@ class TestMultiprocessUnclean final : public Multiprocess {
   DISALLOW_COPY_AND_ASSIGN(TestMultiprocessUnclean);
 };
 
-TEST(Multiprocess, MultiprocessSuccessfulExit) {
+TEST(Multiprocess, SuccessfulExit) {
   TestMultiprocessUnclean multiprocess(TestMultiprocessUnclean::kExitSuccess);
   multiprocess.Run();
 }
 
-TEST(Multiprocess, MultiprocessUnsuccessfulExit) {
+TEST(Multiprocess, UnsuccessfulExit) {
   TestMultiprocessUnclean multiprocess(TestMultiprocessUnclean::kExitFailure);
   multiprocess.Run();
 }
 
-TEST(Multiprocess, MultiprocessExit2) {
+TEST(Multiprocess, Exit2) {
   TestMultiprocessUnclean multiprocess(TestMultiprocessUnclean::kExit2);
   multiprocess.Run();
 }
 
-TEST(Multiprocess, MultiprocessAbortSignal) {
+TEST(Multiprocess, AbortSignal) {
   TestMultiprocessUnclean multiprocess(TestMultiprocessUnclean::kAbort);
   multiprocess.Run();
 }
 
+class TestMultiprocessClosePipe final : public Multiprocess {
+ public:
+  enum WhoCloses {
+    kParentCloses = 0,
+    kChildCloses,
+  };
+  enum WhatCloses {
+    kReadCloses = 0,
+    kWriteCloses,
+    kReadAndWriteClose,
+  };
+
+  TestMultiprocessClosePipe(WhoCloses who_closes, WhatCloses what_closes)
+      : Multiprocess(),
+        who_closes_(who_closes),
+        what_closes_(what_closes) {
+  }
+
+  ~TestMultiprocessClosePipe() {}
+
+ private:
+  void VerifyInitial() {
+    ASSERT_NE(-1, ReadPipeFD());
+    ASSERT_NE(-1, WritePipeFD());
+  }
+
+  // Verifies that the partner process did what it was supposed to do. This must
+  // only be called when who_closes_ names the partner process, not this
+  // process.
+  //
+  // If the partner was supposed to close its write pipe, the read pipe will be
+  // checked to ensure that it shows end-of-file.
+  //
+  // If the partner was supposed to close its read pipe, the write pipe will be
+  // checked to ensure that a checked write causes death. This can only be done
+  // if the partner also provides some type of signal when it has closed its
+  // read pipe, which is done in the form of it closing its write pipe, causing
+  // the read pipe in this process to show end-of-file.
+  void VerifyPartner() {
+    if (what_closes_ == kWriteCloses) {
+      CheckedReadFDAtEOF(ReadPipeFD());
+    } else if (what_closes_ == kReadAndWriteClose) {
+      CheckedReadFDAtEOF(ReadPipeFD());
+      char c = '\0';
+
+      // This will raise SIGPIPE. If fatal (the normal case), that will cause
+      // process termination. If SIGPIPE is being handled somewhere, the write
+      // will still fail and set errno to EPIPE, and CheckedWriteFD() will abort
+      // execution. Regardless of how SIGPIPE is handled, the process will be
+      // terminated. Because the actual termination mechanism is not known, no
+      // regex can be specified.
+      EXPECT_DEATH(CheckedWriteFD(WritePipeFD(), &c, 1), "");
+    }
+  }
+
+  void Close() {
+    switch (what_closes_) {
+      case kReadCloses:
+        CloseReadPipe();
+        EXPECT_NE(-1, WritePipeFD());
+        EXPECT_DEATH(ReadPipeFD(), "fd");
+        break;
+      case kWriteCloses:
+        CloseWritePipe();
+        EXPECT_NE(-1, ReadPipeFD());
+        EXPECT_DEATH(WritePipeFD(), "fd");
+        break;
+      case kReadAndWriteClose:
+        CloseReadPipe();
+        CloseWritePipe();
+        EXPECT_DEATH(ReadPipeFD(), "fd");
+        EXPECT_DEATH(WritePipeFD(), "fd");
+        break;
+    }
+  }
+
+  // Multiprocess:
+
+  virtual void MultiprocessParent() override {
+    VerifyInitial();
+    if (testing::Test::HasFatalFailure()) {
+      return;
+    }
+
+    if (who_closes_ == kParentCloses) {
+      Close();
+    } else {
+      VerifyPartner();
+    }
+  }
+
+  virtual void MultiprocessChild() override {
+    VerifyInitial();
+    if (testing::Test::HasFatalFailure()) {
+      return;
+    }
+
+    if (who_closes_ == kChildCloses) {
+      Close();
+    } else {
+      VerifyPartner();
+    }
+  }
+
+  WhoCloses who_closes_;
+  WhatCloses what_closes_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestMultiprocessClosePipe);
+};
+
+TEST(MultiprocessDeathTest, ParentClosesReadPipe) {
+  TestMultiprocessClosePipe multiprocess(
+      TestMultiprocessClosePipe::kParentCloses,
+      TestMultiprocessClosePipe::kReadCloses);
+  multiprocess.Run();
+}
+
+TEST(MultiprocessDeathTest, ParentClosesWritePipe) {
+  TestMultiprocessClosePipe multiprocess(
+      TestMultiprocessClosePipe::kParentCloses,
+      TestMultiprocessClosePipe::kWriteCloses);
+  multiprocess.Run();
+}
+
+TEST(MultiprocessDeathTest, ParentClosesReadAndWritePipe) {
+  TestMultiprocessClosePipe multiprocess(
+      TestMultiprocessClosePipe::kParentCloses,
+      TestMultiprocessClosePipe::kReadAndWriteClose);
+  multiprocess.Run();
+}
+
+TEST(MultiprocessDeathTest, ChildClosesReadPipe) {
+  TestMultiprocessClosePipe multiprocess(
+      TestMultiprocessClosePipe::kChildCloses,
+      TestMultiprocessClosePipe::kReadCloses);
+  multiprocess.Run();
+}
+
+TEST(MultiprocessDeathTest, ChildClosesWritePipe) {
+  TestMultiprocessClosePipe multiprocess(
+      TestMultiprocessClosePipe::kChildCloses,
+      TestMultiprocessClosePipe::kWriteCloses);
+  multiprocess.Run();
+}
+
+TEST(MultiprocessDeathTest, ChildClosesReadAndWritePipe) {
+  TestMultiprocessClosePipe multiprocess(
+      TestMultiprocessClosePipe::kChildCloses,
+      TestMultiprocessClosePipe::kReadAndWriteClose);
+  multiprocess.Run();
+}
+
 }  // namespace