Bug 1534780 - Move ipc::FileDescriptor's UniquePtr instance into MFBT as UniqueFileHandle. r=froydnj

MozReview-Commit-ID: 7bbGVIjTTaJ

Differential Revision: https://phabricator.services.mozilla.com/D26737

--HG--
extra : moz-landing-system : lando

ipc/glue/FileDescriptor.cpp

@@ -7,104 +7,71 @@
 #include "FileDescriptor.h"
 
 #include "mozilla/Assertions.h"
-#include "mozilla/Move.h"
+#include "mozilla/TypeTraits.h"
 #include "nsDebug.h"
 
 #ifdef XP_WIN
-
 #  include <windows.h>
 #  include "ProtocolUtils.h"
-#  define INVALID_HANDLE INVALID_HANDLE_VALUE
-
 #else  // XP_WIN
-
 #  include <unistd.h>
-
-#  ifndef OS_POSIX
-#    define OS_POSIX
-#  endif
-
-#  include "base/eintr_wrapper.h"
-#  define INVALID_HANDLE -1
-
 #endif  // XP_WIN
 
 namespace mozilla {
 namespace ipc {
 
-FileDescriptor::FileDescriptor() : mHandle(INVALID_HANDLE) {}
+FileDescriptor::FileDescriptor() = default;
 
 FileDescriptor::FileDescriptor(const FileDescriptor& aOther)
-    : mHandle(INVALID_HANDLE) {
+    : mHandle(Clone(aOther.mHandle.get())) {}
-  Assign(aOther);
-}
 
 FileDescriptor::FileDescriptor(FileDescriptor&& aOther)
-    : mHandle(INVALID_HANDLE) {
+    : mHandle(std::move(aOther.mHandle)) {}
-  *this = std::move(aOther);
-}
 
 FileDescriptor::FileDescriptor(PlatformHandleType aHandle)
-    : mHandle(INVALID_HANDLE) {
+    : mHandle(Clone(aHandle)) {}
-  mHandle = Clone(aHandle);
-}
 
-FileDescriptor::FileDescriptor(const IPDLPrivate&, const PickleType& aPickle)
+FileDescriptor::FileDescriptor(const IPDLPrivate&, const PickleType& aPickle) {
-    : mHandle(INVALID_HANDLE) {
 #ifdef XP_WIN
-  mHandle = aPickle;
+  mHandle.reset(aPickle);
 #else
-  mHandle = aPickle.fd;
+  mHandle.reset(aPickle.fd);
 #endif
 }
 
-FileDescriptor::~FileDescriptor() { Close(); }
+FileDescriptor::~FileDescriptor() = default;
 
 FileDescriptor& FileDescriptor::operator=(const FileDescriptor& aOther) {
   if (this != &aOther) {
-    Assign(aOther);
+    mHandle = Clone(aOther.mHandle.get());
   }
   return *this;
 }
 
 FileDescriptor& FileDescriptor::operator=(FileDescriptor&& aOther) {
   if (this != &aOther) {
-    Close();
+    mHandle = std::move(aOther.mHandle);
-    mHandle = aOther.mHandle;
-    aOther.mHandle = INVALID_HANDLE;
   }
   return *this;
 }
 
-bool FileDescriptor::IsValid() const { return IsValid(mHandle); }
-
-void FileDescriptor::Assign(const FileDescriptor& aOther) {
-  Close();
-  mHandle = Clone(aOther.mHandle);
-}
-
-void FileDescriptor::Close() {
-  Close(mHandle);
-  mHandle = INVALID_HANDLE;
-}
-
 FileDescriptor::PickleType FileDescriptor::ShareTo(
     const FileDescriptor::IPDLPrivate&,
     FileDescriptor::ProcessId aTargetPid) const {
   PlatformHandleType newHandle;
 #ifdef XP_WIN
   if (IsValid()) {
-    if (mozilla::ipc::DuplicateHandle(mHandle, aTargetPid, &newHandle, 0,
+    if (mozilla::ipc::DuplicateHandle(mHandle.get(), aTargetPid, &newHandle, 0,
                                       DUPLICATE_SAME_ACCESS)) {
       return newHandle;
     }
     NS_WARNING("Failed to duplicate file handle for other process!");
   }
-  return INVALID_HANDLE;
+  return INVALID_HANDLE_VALUE;
 #else  // XP_WIN
   if (IsValid()) {
-    newHandle = dup(mHandle);
+    newHandle = dup(mHandle.get());
-    if (IsValid(newHandle)) {
+    if (newHandle >= 0) {
       return base::FileDescriptor(newHandle, /* auto_close */ true);
     }
     NS_WARNING("Failed to duplicate file handle for other process!");
@@ -115,9 +82,11 @@ FileDescriptor::PickleType FileDescriptor::ShareTo(
   MOZ_CRASH("Must not get here!");
 }
 
+bool FileDescriptor::IsValid() const { return mHandle != nullptr; }
+
 FileDescriptor::UniquePlatformHandle FileDescriptor::ClonePlatformHandle()
     const {
-  return UniquePlatformHandle(Clone(mHandle));
+  return Clone(mHandle.get());
 }
 
 bool FileDescriptor::operator==(const FileDescriptor& aOther) const {
@@ -125,67 +94,29 @@ bool FileDescriptor::operator==(const FileDescriptor& aOther) const {
 }
 
 // static
-bool FileDescriptor::IsValid(PlatformHandleType aHandle) {
+FileDescriptor::UniquePlatformHandle FileDescriptor::Clone(
-  return aHandle != INVALID_HANDLE;
-}
-
-// static
-FileDescriptor::PlatformHandleType FileDescriptor::Clone(
     PlatformHandleType aHandle) {
-  if (!IsValid(aHandle)) {
-    return INVALID_HANDLE;
-  }
   FileDescriptor::PlatformHandleType newHandle;
+
 #ifdef XP_WIN
+  if (aHandle == INVALID_HANDLE_VALUE) {
+    return UniqueFileHandle();
+  }
   if (::DuplicateHandle(GetCurrentProcess(), aHandle, GetCurrentProcess(),
                         &newHandle, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
-#else  // XP_WIN
+    return UniqueFileHandle(newHandle);
-  if ((newHandle = dup(aHandle)) != INVALID_HANDLE) {
-#endif
-    return newHandle;
   }
+#else  // XP_WIN
+  if (aHandle < 0) {
+    return UniqueFileHandle();
+  }
+  newHandle = dup(aHandle);
+  if (newHandle >= 0) {
+    return UniqueFileHandle(newHandle);
+  }
+#endif
   NS_WARNING("Failed to duplicate file handle for current process!");
-  return INVALID_HANDLE;
+  return UniqueFileHandle();
-}
-
-// static
-void FileDescriptor::Close(PlatformHandleType aHandle) {
-  if (IsValid(aHandle)) {
-#ifdef XP_WIN
-    if (!CloseHandle(aHandle)) {
-      NS_WARNING("Failed to close file handle for current process!");
-    }
-#else  // XP_WIN
-    IGNORE_EINTR(close(aHandle));
-#endif
-  }
-}
-
-FileDescriptor::PlatformHandleHelper::PlatformHandleHelper(
-    FileDescriptor::PlatformHandleType aHandle)
-    : mHandle(aHandle) {}
-
-FileDescriptor::PlatformHandleHelper::PlatformHandleHelper(std::nullptr_t)
-    : mHandle(INVALID_HANDLE) {}
-
-bool FileDescriptor::PlatformHandleHelper::operator!=(std::nullptr_t) const {
-  return mHandle != INVALID_HANDLE;
-}
-
-FileDescriptor::PlatformHandleHelper::
-operator FileDescriptor::PlatformHandleType() const {
-  return mHandle;
-}
-
-#ifdef XP_WIN
-FileDescriptor::PlatformHandleHelper::operator std::intptr_t() const {
-  return reinterpret_cast<std::intptr_t>(mHandle);
-}
-#endif
-
-void FileDescriptor::PlatformHandleDeleter::operator()(
-    FileDescriptor::PlatformHandleHelper aHelper) {
-  FileDescriptor::Close(aHelper);
 }
 
 void IPDLParamTraits<FileDescriptor>::Write(IPC::Message* aMsg,

ipc/glue/FileDescriptor.h

@@ -9,15 +9,11 @@
 
 #include "base/basictypes.h"
 #include "base/process.h"
-#include "mozilla/UniquePtr.h"
 #include "ipc/IPCMessageUtils.h"
+#include "mozilla/UniquePtrExtensions.h"
 #include "mozilla/ipc/IPDLParamTraits.h"
 
-#ifdef XP_WIN
+#ifdef XP_UNIX
-// Need the HANDLE typedef.
-#  include <winnt.h>
-#  include <cstdint>
-#else
 #  include "base/file_descriptor_posix.h"
 #endif
 
@@ -40,32 +36,15 @@ class FileDescriptor {
  public:
   typedef base::ProcessId ProcessId;
 
+  using UniquePlatformHandle = mozilla::UniqueFileHandle;
+  using PlatformHandleType = UniquePlatformHandle::ElementType;
+
 #ifdef XP_WIN
-  typedef HANDLE PlatformHandleType;
+  typedef PlatformHandleType PickleType;
-  typedef HANDLE PickleType;
 #else
-  typedef int PlatformHandleType;
   typedef base::FileDescriptor PickleType;
 #endif
 
-  struct PlatformHandleHelper {
-    MOZ_IMPLICIT PlatformHandleHelper(PlatformHandleType aHandle);
-    MOZ_IMPLICIT PlatformHandleHelper(std::nullptr_t);
-    bool operator!=(std::nullptr_t) const;
-    operator PlatformHandleType() const;
-#ifdef XP_WIN
-    operator std::intptr_t() const;
-#endif
-   private:
-    PlatformHandleType mHandle;
-  };
-  struct PlatformHandleDeleter {
-    typedef PlatformHandleHelper pointer;
-    void operator()(PlatformHandleHelper aHelper);
-  };
-  typedef UniquePtr<PlatformHandleType, PlatformHandleDeleter>
-      UniquePlatformHandle;
-
   // This should only ever be created by IPDL.
   struct IPDLPrivate {};
 
@@ -108,19 +87,9 @@ class FileDescriptor {
   bool operator==(const FileDescriptor& aOther) const;
 
  private:
-  friend struct PlatformHandleTrait;
+  static UniqueFileHandle Clone(PlatformHandleType aHandle);
 
-  void Assign(const FileDescriptor& aOther);
+  UniquePlatformHandle mHandle;
-
-  void Close();
-
-  static bool IsValid(PlatformHandleType aHandle);
-
-  static PlatformHandleType Clone(PlatformHandleType aHandle);
-
-  static void Close(PlatformHandleType aHandle);
-
-  PlatformHandleType mHandle;
 };
 
 template <>

mfbt/UniquePtrExtensions.cpp

@@ -0,0 +1,35 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "UniquePtrExtensions.h"
+
+#include "mozilla/Assertions.h"
+#include "mozilla/DebugOnly.h"
+
+#ifdef XP_WIN
+#  include <windows.h>
+#else
+#  include <errno.h>
+#  include <unistd.h>
+#endif
+
+namespace mozilla {
+namespace detail {
+
+void FileHandleDeleter::operator()(FileHandleHelper aHelper) {
+  if (aHelper != nullptr) {
+    DebugOnly<bool> ok;
+#ifdef XP_WIN
+    ok = CloseHandle(aHelper);
+#else
+    ok = close(aHelper) == 0 || errno == EINTR;
+#endif
+    MOZ_ASSERT(ok, "failed to close file handle");
+  }
+}
+
+}  // namespace detail
+}  // namespace mozilla

mfbt/UniquePtrExtensions.h

@@ -12,6 +12,10 @@
 #include "mozilla/fallible.h"
 #include "mozilla/UniquePtr.h"
 
+#ifdef XP_WIN
+#  include <cstdint>
+#endif
+
 namespace mozilla {
 
 /**
@@ -42,11 +46,81 @@ struct FreePolicy {
   void operator()(const void* ptr) { free(const_cast<void*>(ptr)); }
 };
 
+#if defined(XP_WIN)
+// Can't include <windows.h> to get the actual definition of HANDLE
+// because of namespace pollution.
+typedef void* FileHandleType;
+#elif defined(XP_UNIX)
+typedef int FileHandleType;
+#else
+#  error "Unsupported OS?"
+#endif
+
+struct FileHandleHelper {
+  MOZ_IMPLICIT FileHandleHelper(FileHandleType aHandle) : mHandle(aHandle) {}
+
+  MOZ_IMPLICIT constexpr FileHandleHelper(std::nullptr_t)
+      : mHandle(kInvalidHandle) {}
+
+  bool operator!=(std::nullptr_t) const {
+#ifdef XP_WIN
+    // Windows uses both nullptr and INVALID_HANDLE_VALUE (-1 cast to
+    // HANDLE) in different situations, but nullptr is more reliably
+    // null while -1 is also valid input to some calls that take
+    // handles.  So class considers both to be null (since neither
+    // should be closed) but default-constructs as nullptr.
+    if (mHandle == (void*)-1) {
+      return false;
+    }
+#endif
+    return mHandle != kInvalidHandle;
+  }
+
+  operator FileHandleType() const { return mHandle; }
+
+#ifdef XP_WIN
+  // NSPR uses an integer type for PROsfd, so this conversion is
+  // provided for working with it without needing reinterpret casts
+  // everywhere.
+  operator std::intptr_t() const {
+    return reinterpret_cast<std::intptr_t>(mHandle);
+  }
+#endif
+
+  // When there's only one user-defined conversion operator, the
+  // compiler will use that to derive equality, but that doesn't work
+  // when the conversion is ambiguoug (the XP_WIN case above).
+  bool operator==(const FileHandleHelper& aOther) const {
+    return mHandle == aOther.mHandle;
+  }
+
+ private:
+  FileHandleType mHandle;
+
+#ifdef XP_WIN
+  // See above for why this is nullptr.  (Also, INVALID_HANDLE_VALUE
+  // can't be expressed as a constexpr.)
+  static constexpr FileHandleType kInvalidHandle = nullptr;
+#else
+  static constexpr FileHandleType kInvalidHandle = -1;
+#endif
+};
+
+struct FileHandleDeleter {
+  typedef FileHandleHelper pointer;
+  MFBT_API void operator()(FileHandleHelper aHelper);
+};
+
 }  // namespace detail
 
 template <typename T>
 using UniqueFreePtr = UniquePtr<T, detail::FreePolicy<T>>;
 
+// A RAII class for the OS construct used for open files and similar
+// objects: a file descriptor on Unix or a handle on Windows.
+using UniqueFileHandle =
+    UniquePtr<detail::FileHandleType, detail::FileHandleDeleter>;
+
 }  // namespace mozilla
 
 #endif  // mozilla_UniquePtrExtensions_h

mfbt/moz.build

@@ -151,6 +151,7 @@ UNIFIED_SOURCES += [
     'RandomNum.cpp',
     'SHA1.cpp',
     'TaggedAnonymousMemory.cpp',
+    'UniquePtrExtensions.cpp',
     'Unused.cpp',
     'Utf8.cpp',
 ]