net: Remove one of FileStream's ctors which takes PlatformFile

Fix users, NativeMessageProcessHost and PipeReader.

BUG=322664, 369183
TEST=For NativeMessageProcessHost: unit_tests --gtest_filter="NativeMessagingTest.*"
TEST=For PipeReader: On a Chromebook, open chrome://tracing and start recording with "System tracing" checked, see "Kernel" section appears on the result.

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@268662 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 52 insertions, 70 deletions

diff --git a/chromeos/dbus/debug_daemon_client.cc b/chromeos/dbus/debug_daemon_client.cc
index 2c81c17..8a8804f 100644
--- a/chromeos/dbus/debug_daemon_client.cc
+++ b/chromeos/dbus/debug_daemon_client.cc
@@ -18,6 +18,7 @@
 #include "base/platform_file.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/strings/string_util.h"
+#include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
 #include "chromeos/dbus/pipe_reader.h"
 #include "dbus/bus.h"
@@ -53,8 +54,8 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
     // issue the D-Bus request to stop tracing and collect results.
     base::WorkerPool::PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&DebugDaemonClientImpl::CheckValidity,
-                   file_descriptor),
+        base::Bind(&dbus::FileDescriptor::CheckValidity,
+                   base::Unretained(file_descriptor)),
         base::Bind(&DebugDaemonClientImpl::OnCheckValidityGetDebugLogs,
                    weak_ptr_factory_.GetWeakPtr(),
                    base::Owned(file_descriptor),
@@ -221,33 +222,27 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
       return false;
     }
 
+    scoped_refptr<base::TaskRunner> task_runner =
+        base::WorkerPool::GetTaskRunner(true /* task_is_slow */);
+
     pipe_reader_.reset(new PipeReaderForString(
-        base::WorkerPool::GetTaskRunner(true /* task_is_slow */),
+        task_runner,
         base::Bind(&DebugDaemonClientImpl::OnIOComplete,
                    weak_ptr_factory_.GetWeakPtr())));
-    int write_fd = -1;
-    if (!pipe_reader_->StartIO()) {
-      LOG(ERROR) << "Cannot create pipe reader";
-      // NB: continue anyway to shutdown tracing; toss trace data
-      write_fd = HANDLE_EINTR(open("/dev/null", O_WRONLY));
-      // TODO(sleffler) if this fails AppendFileDescriptor will abort
-    } else {
-      write_fd = pipe_reader_->write_fd();
-    }
 
-    dbus::FileDescriptor* file_descriptor = new dbus::FileDescriptor(write_fd);
-    // Punt descriptor validity check to a worker thread; on return we'll
+    base::File pipe_write_end = pipe_reader_->StartIO();
+    // Create dbus::FileDescriptor on the worker thread; on return we'll
     // issue the D-Bus request to stop tracing and collect results.
-    base::WorkerPool::PostTaskAndReply(
+    base::PostTaskAndReplyWithResult(
+        task_runner,
         FROM_HERE,
-        base::Bind(&DebugDaemonClientImpl::CheckValidity,
-                   file_descriptor),
-        base::Bind(&DebugDaemonClientImpl::OnCheckValidityRequestStopSystem,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   base::Owned(file_descriptor),
-                   callback),
-        false);
-
+        base::Bind(
+            &DebugDaemonClientImpl::CreateFileDescriptorToStopSystemTracing,
+            base::Passed(&pipe_write_end)),
+        base::Bind(
+            &DebugDaemonClientImpl::OnCreateFileDescriptorRequestStopSystem,
+            weak_ptr_factory_.GetWeakPtr(),
+            callback));
     return true;
   }
 
@@ -316,11 +311,6 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
   }
 
  private:
-  // Called to check descriptor validity on a thread where i/o is permitted.
-  static void CheckValidity(dbus::FileDescriptor* file_descriptor) {
-    file_descriptor->CheckValidity();
-  }
-
   // Called when a CheckValidity response is received.
   void OnCheckValidityGetDebugLogs(dbus::FileDescriptor* file_descriptor,
                                    const GetDebugLogsCallback& callback) {
@@ -469,10 +459,28 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
     }
   }
 
+  // Creates dbus::FileDescriptor from base::File.
+  static scoped_ptr<dbus::FileDescriptor>
+  CreateFileDescriptorToStopSystemTracing(base::File pipe_write_end) {
+    if (!pipe_write_end.IsValid()) {
+      LOG(ERROR) << "Cannot create pipe reader";
+      // NB: continue anyway to shutdown tracing; toss trace data
+      pipe_write_end.Initialize(base::FilePath(FILE_PATH_LITERAL("/dev/null")),
+                                base::File::FLAG_OPEN | base::File::FLAG_WRITE);
+      // TODO(sleffler) if this fails AppendFileDescriptor will abort
+    }
+    scoped_ptr<dbus::FileDescriptor> file_descriptor(new dbus::FileDescriptor);
+    file_descriptor->PutValue(pipe_write_end.TakePlatformFile());
+    file_descriptor->CheckValidity();
+    return file_descriptor.Pass();
+  }
+
   // Called when a CheckValidity response is received.
-  void OnCheckValidityRequestStopSystem(
-      dbus::FileDescriptor* file_descriptor,
-      const StopSystemTracingCallback& callback) {
+  void OnCreateFileDescriptorRequestStopSystem(
+      const StopSystemTracingCallback& callback,
+      scoped_ptr<dbus::FileDescriptor> file_descriptor) {
+    DCHECK(file_descriptor);
+
     // Issue the dbus request to stop system tracing
     dbus::MethodCall method_call(
         debugd::kDebugdInterface,
@@ -488,8 +496,6 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
         dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(&DebugDaemonClientImpl::OnRequestStopSystemTracing,
                    weak_ptr_factory_.GetWeakPtr()));
-
-    pipe_reader_->CloseWriteFD();  // close our copy of fd after send
   }
 
   // Called when a response for RequestStopSystemTracing() is received.
diff --git a/chromeos/dbus/pipe_reader.cc b/chromeos/dbus/pipe_reader.cc
index 1e91d75..497f514 100644
--- a/chromeos/dbus/pipe_reader.cc
+++ b/chromeos/dbus/pipe_reader.cc
@@ -5,7 +5,6 @@
 #include "chromeos/dbus/pipe_reader.h"
 
 #include "base/bind.h"
-#include "base/platform_file.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/task_runner.h"
 #include "net/base/file_stream.h"
@@ -16,39 +15,26 @@ namespace chromeos {
 
 PipeReader::PipeReader(const scoped_refptr<base::TaskRunner>& task_runner,
                        const IOCompleteCallback& callback)
-    : write_fd_(-1),
-      io_buffer_(new net::IOBufferWithSize(4096)),
+    : io_buffer_(new net::IOBufferWithSize(4096)),
       task_runner_(task_runner),
       callback_(callback),
       weak_ptr_factory_(this) {}
 
 PipeReader::~PipeReader() {
-  CloseWriteFD();
 }
 
-void PipeReader::CloseWriteFD() {
-  if (write_fd_ == -1)
-    return;
-  if (IGNORE_EINTR(close(write_fd_)) < 0)
-    PLOG(ERROR) << "close";
-  write_fd_ = -1;
-}
-
-bool PipeReader::StartIO() {
+base::File PipeReader::StartIO() {
   // Use a pipe to collect data
   int pipe_fds[2];
   const int status = HANDLE_EINTR(pipe(pipe_fds));
   if (status < 0) {
     PLOG(ERROR) << "pipe";
-    return false;
+    return base::File();
   }
-  write_fd_ = pipe_fds[1];
-  base::PlatformFile data_file_ = pipe_fds[0];
-  // Pass ownership of pipe_fds[0] to data_stream_, which will will close it.
+  base::File pipe_write_end(pipe_fds[1]);
+  // Pass ownership of pipe_fds[0] to data_stream_, which will close it.
   data_stream_.reset(new net::FileStream(
-      data_file_,
-      base::PLATFORM_FILE_READ | base::PLATFORM_FILE_ASYNC,
-      task_runner_));
+      base::File(pipe_fds[0]), task_runner_));
 
   // Post an initial async read to setup data collection
   int rv = data_stream_->Read(
@@ -56,9 +42,9 @@ bool PipeReader::StartIO() {
       base::Bind(&PipeReader::OnDataReady, weak_ptr_factory_.GetWeakPtr()));
   if (rv != net::ERR_IO_PENDING) {
     LOG(ERROR) << "Unable to post initial read";
-    return false;
+    return base::File();
   }
-  return true;
+  return pipe_write_end.Pass();
 }
 
 void PipeReader::OnDataReady(int byte_count) {
@@ -90,7 +76,6 @@ void PipeReaderForString::AcceptData(const char *data, int byte_count) {
   data_.append(data, byte_count);
 }
 
-
 void PipeReaderForString::GetData(std::string* data) {
   data_.swap(*data);
 }
diff --git a/chromeos/dbus/pipe_reader.h b/chromeos/dbus/pipe_reader.h
index 59d291b..41dcb5e 100644
--- a/chromeos/dbus/pipe_reader.h
+++ b/chromeos/dbus/pipe_reader.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/files/file.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -27,10 +28,6 @@ namespace chromeos {
 // string.  To use:
 //   - Instantiate the appropriate subclass of PipeReader
 //   - Call StartIO() which will create the appropriate FDs.
-//   - Call GetWriteFD() which will return a file descriptor that can
-//     be sent to a separate process which will write data there.
-//   - After handing off the FD, call CloseWriteFD() so there is
-//     only one copy of the FD open.
 //   - As data is received, the PipeReader will collect this data
 //     as appropriate to the subclass.
 //   - When the there is no more data to read, the PipeReader calls
@@ -43,14 +40,12 @@ class PipeReader {
              const IOCompleteCallback& callback);
   virtual ~PipeReader();
 
-  // Closes writeable descriptor; normally used in parent process after fork.
-  void CloseWriteFD();
-
-  // Starts data collection.  Returns true if stream was setup correctly.
+  // Starts data collection.
+  // Returns the write end of the pipe if stream was setup correctly.
   // On success data will automatically be accumulated into a string that
   // can be retrieved with PipeReader::data().  To shutdown collection delete
   // the instance and/or use PipeReader::OnDataReady(-1).
-  bool StartIO();
+  base::File StartIO();
 
   // Called when pipe data are available.  Can also be used to shutdown
   // data collection by passing -1 for |byte_count|.
@@ -60,11 +55,7 @@ class PipeReader {
   // with incoming data.
   virtual void AcceptData(const char *data, int length) = 0;
 
-  // Getter for |write_fd_|.
-  int write_fd() const { return write_fd_; }
-
  private:
-  int write_fd_;
   scoped_ptr<net::FileStream> data_stream_;
   scoped_refptr<net::IOBufferWithSize> io_buffer_;
   scoped_refptr<base::TaskRunner> task_runner_;
@@ -94,6 +85,6 @@ class PipeReaderForString : public PipeReader {
   DISALLOW_COPY_AND_ASSIGN(PipeReaderForString);
 };
 
-}
+}  // namespace chromeos
 
 #endif  // CHROMEOS_DBUS_PIPE_READER_H_