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

10 files changed, 66 insertions, 106 deletions

diff --git a/chrome/browser/chromeos/drive/local_file_reader.cc b/chrome/browser/chromeos/drive/local_file_reader.cc
index e6bacc1..5041811 100644
--- a/chrome/browser/chromeos/drive/local_file_reader.cc
+++ b/chrome/browser/chromeos/drive/local_file_reader.cc
@@ -28,7 +28,7 @@ void LocalFileReader::Open(const base::FilePath& file_path,
                            const net::CompletionCallback& callback) {
   DCHECK(!callback.is_null());
   int flags = base::File::FLAG_OPEN | base::File::FLAG_READ |
-              base::PLATFORM_FILE_ASYNC;
+              base::File::FLAG_ASYNC;
 
   int rv = file_stream_.Open(file_path, flags,
                              Bind(&LocalFileReader::DidOpen,
diff --git a/chrome/browser/extensions/api/messaging/native_message_process_host.cc b/chrome/browser/extensions/api/messaging/native_message_process_host.cc
index 73af519..b4993dc 100644
--- a/chrome/browser/extensions/api/messaging/native_message_process_host.cc
+++ b/chrome/browser/extensions/api/messaging/native_message_process_host.cc
@@ -198,14 +198,8 @@ void NativeMessageProcessHost::OnHostProcessLaunched(
           GetTaskRunnerWithShutdownBehavior(
               base::SequencedWorkerPool::SKIP_ON_SHUTDOWN));
 
-  read_stream_.reset(new net::FileStream(
-      read_file.TakePlatformFile(),
-      base::PLATFORM_FILE_READ | base::PLATFORM_FILE_ASYNC,
-      task_runner));
-  write_stream_.reset(new net::FileStream(
-      write_file.TakePlatformFile(),
-      base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_ASYNC,
-      task_runner));
+  read_stream_.reset(new net::FileStream(read_file.Pass(), task_runner));
+  write_stream_.reset(new net::FileStream(write_file.Pass(), task_runner));
 
   WaitRead();
   DoWrite();
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_
diff --git a/net/base/file_stream.cc b/net/base/file_stream.cc
index d96bebb..bf56a49 100644
--- a/net/base/file_stream.cc
+++ b/net/base/file_stream.cc
@@ -13,12 +13,6 @@ FileStream::FileStream(const scoped_refptr<base::TaskRunner>& task_runner)
     : context_(new Context(task_runner)) {
 }
 
-FileStream::FileStream(base::PlatformFile file,
-                       int flags,
-                       const scoped_refptr<base::TaskRunner>& task_runner)
-    : context_(new Context(base::File(file), task_runner)) {
-}
-
 FileStream::FileStream(base::File file,
                        const scoped_refptr<base::TaskRunner>& task_runner)
     : context_(new Context(file.Pass(), task_runner)) {
diff --git a/net/base/file_stream.h b/net/base/file_stream.h
index 850c78f..57011b7 100644
--- a/net/base/file_stream.h
+++ b/net/base/file_stream.h
@@ -11,14 +11,13 @@
 #define NET_BASE_FILE_STREAM_H_
 
 #include "base/files/file.h"
-#include "base/platform_file.h"
-#include "base/task_runner.h"
 #include "net/base/completion_callback.h"
 #include "net/base/file_stream_whence.h"
 #include "net/base/net_export.h"
 
 namespace base {
 class FilePath;
+class TaskRunner;
 }
 
 namespace net {
@@ -31,20 +30,8 @@ class NET_EXPORT FileStream {
   // Uses |task_runner| for asynchronous operations.
   explicit FileStream(const scoped_refptr<base::TaskRunner>& task_runner);
 
-  // Construct a FileStream with an existing file handle and opening flags.
-  // |file| is valid file handle.
-  // |flags| is a bitfield of base::PlatformFileFlags when the file handle was
-  // opened.
+  // Construct a FileStream with an existing valid |file|.
   // Uses |task_runner| for asynchronous operations.
-  // Note: the new FileStream object takes ownership of the PlatformFile and
-  // will close it on destruction.
-  // This constructor is deprecated.
-  // TODO(rvargas): remove all references to PlatformFile.
-  FileStream(base::PlatformFile file,
-             int flags,
-             const scoped_refptr<base::TaskRunner>& task_runner);
-
-  // Non-deprecated version of the previous constructor.
   FileStream(base::File file,
              const scoped_refptr<base::TaskRunner>& task_runner);
 
@@ -58,7 +45,7 @@ class NET_EXPORT FileStream {
   //
   // Once the operation is done, |callback| will be run on the thread where
   // Open() was called, with the result code. open_flags is a bitfield of
-  // base::PlatformFileFlags.
+  // base::File::Flags.
   //
   // If the file stream is not closed manually, the underlying file will be
   // automatically closed when FileStream is destructed in an asynchronous
@@ -91,7 +78,7 @@ class NET_EXPORT FileStream {
   // copied, 0 if at end-of-file, or an error code if the operation could
   // not be performed.
   //
-  // The file must be opened with PLATFORM_FILE_ASYNC, and a non-null
+  // The file must be opened with FLAG_ASYNC, and a non-null
   // callback must be passed to this method. If the read could not
   // complete synchronously, then ERR_IO_PENDING is returned, and the
   // callback will be run on the thread where Read() was called, when the
@@ -114,7 +101,7 @@ class NET_EXPORT FileStream {
   // bytes written, or an error code if the operation could not be
   // performed.
   //
-  // The file must be opened with PLATFORM_FILE_ASYNC, and a non-null
+  // The file must be opened with FLAG_ASYNC, and a non-null
   // callback must be passed to this method. If the write could not
   // complete synchronously, then ERR_IO_PENDING is returned, and the
   // callback will be run on the thread where Write() was called when
@@ -138,7 +125,7 @@ class NET_EXPORT FileStream {
   // not have to be called, it just forces one to happen at the time of
   // calling.
   //
-  // The file must be opened with PLATFORM_FILE_ASYNC, and a non-null
+  // The file must be opened with FLAG_ASYNC, and a non-null
   // callback must be passed to this method. If the write could not
   // complete synchronously, then ERR_IO_PENDING is returned, and the
   // callback will be run on the thread where Flush() was called when
@@ -154,7 +141,7 @@ class NET_EXPORT FileStream {
   // This method should not be called if the stream was opened READ_ONLY.
   virtual int Flush(const CompletionCallback& callback);
 
-  // Returns the underlying platform file for testing.
+  // Returns the underlying file for testing.
   const base::File& GetFileForTesting() const;
 
  private:
diff --git a/net/base/file_stream_context.cc b/net/base/file_stream_context.cc
index 487144e..fbfe536 100644
--- a/net/base/file_stream_context.cc
+++ b/net/base/file_stream_context.cc
@@ -4,8 +4,10 @@
 
 #include "net/base/file_stream_context.h"
 
+#include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/values.h"
diff --git a/net/base/file_stream_context_posix.cc b/net/base/file_stream_context_posix.cc
index ecf9285..9f3d060 100644
--- a/net/base/file_stream_context_posix.cc
+++ b/net/base/file_stream_context_posix.cc
@@ -22,6 +22,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/posix/eintr_wrapper.h"
+#include "base/task_runner.h"
 #include "base/task_runner_util.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
diff --git a/net/base/file_stream_context_win.cc b/net/base/file_stream_context_win.cc
index 2477535..e906b6b 100644
--- a/net/base/file_stream_context_win.cc
+++ b/net/base/file_stream_context_win.cc
@@ -9,7 +9,7 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
-#include "base/task_runner_util.h"
+#include "base/task_runner.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"