Bug 1479960 - Get rid of base::SharedMemory::handle. r=froydnj
authorJed Davis <jld@mozilla.com>
Wed, 14 Aug 2019 22:48:22 +0000
changeset 488138 f814f58d43d66df993d9d9d492ec39c1417b1a3d
parent 488137 16d99c5a8a550366c7785edaeadf67c14d7151f1
child 488139 bdf7a041928aa4673de020df11c069e0af996d50
push id113900
push usercbrindusan@mozilla.com
push dateThu, 15 Aug 2019 09:53:50 +0000
treeherdermozilla-inbound@0db07ff50ab5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1479960
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1479960 - Get rid of base::SharedMemory::handle. r=froydnj Despite the comment saying not to use the "handle" except as an opaque identifier, it is being used to pass the handle to other OS APIs. Direct access to the handle needs to be controlled to make sure freezing is safe, so this patch replaces that with interfaces that are more explicit about ownership and lifetime. Depends on D26739 Differential Revision: https://phabricator.services.mozilla.com/D26740
gfx/ipc/SharedDIB.cpp
gfx/ipc/SharedDIBWin.cpp
ipc/chromium/src/base/shared_memory.h
ipc/chromium/src/base/shared_memory_posix.cc
ipc/chromium/src/base/shared_memory_win.cc
ipc/glue/ProcessUtils.h
ipc/glue/ProcessUtils_common.cpp
--- a/gfx/ipc/SharedDIB.cpp
+++ b/gfx/ipc/SharedDIB.cpp
@@ -17,21 +17,17 @@ nsresult SharedDIB::Create(uint32_t aSiz
   Close();
 
   mShMem = new base::SharedMemory();
   if (!mShMem || !mShMem->Create(aSize)) return NS_ERROR_OUT_OF_MEMORY;
 
   return NS_OK;
 }
 
-bool SharedDIB::IsValid() {
-  if (!mShMem) return false;
-
-  return base::SharedMemory::IsHandleValid(mShMem->handle());
-}
+bool SharedDIB::IsValid() { return mShMem && mShMem->IsValid(); }
 
 nsresult SharedDIB::Close() {
   delete mShMem;
 
   mShMem = nullptr;
 
   return NS_OK;
 }
--- a/gfx/ipc/SharedDIBWin.cpp
+++ b/gfx/ipc/SharedDIBWin.cpp
@@ -95,18 +95,19 @@ uint32_t SharedDIBWin::SetupBitmapHeader
           (-aHeader->bV4Height * aHeader->bV4Width * kBytesPerPixel));
 }
 
 nsresult SharedDIBWin::SetupSurface(HDC aHdc, BITMAPV4HEADER* aHdr) {
   mSharedHdc = ::CreateCompatibleDC(aHdc);
 
   if (!mSharedHdc) return NS_ERROR_FAILURE;
 
-  mSharedBmp = ::CreateDIBSection(mSharedHdc, (BITMAPINFO*)aHdr, DIB_RGB_COLORS,
-                                  &mBitmapBits, mShMem->handle(), kHeaderBytes);
+  mSharedBmp =
+      ::CreateDIBSection(mSharedHdc, (BITMAPINFO*)aHdr, DIB_RGB_COLORS,
+                         &mBitmapBits, mShMem->GetHandle(), kHeaderBytes);
   if (!mSharedBmp) return NS_ERROR_FAILURE;
 
   mOldObj = SelectObject(mSharedHdc, mSharedBmp);
 
   return NS_OK;
 }
 
 }  // namespace gfx
--- a/ipc/chromium/src/base/shared_memory.h
+++ b/ipc/chromium/src/base/shared_memory.h
@@ -13,16 +13,17 @@
 #  include <sys/types.h>
 #  include <semaphore.h>
 #  include "base/file_descriptor_posix.h"
 #endif
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/process.h"
+#include "mozilla/UniquePtrExtensions.h"
 
 namespace base {
 
 // SharedMemoryHandle is a platform specific type which represents
 // the underlying OS handle to a shared memory segment.
 #if defined(OS_WIN)
 typedef HANDLE SharedMemoryHandle;
 #elif defined(OS_POSIX)
@@ -52,16 +53,19 @@ class SharedMemory {
   // Initialize a new SharedMemory object from an existing, open
   // shared memory file.
   bool SetHandle(SharedMemoryHandle handle, bool read_only);
 
   // Return true iff the given handle is valid (i.e. not the distingished
   // invalid value; NULL for a HANDLE and -1 for a file descriptor)
   static bool IsHandleValid(const SharedMemoryHandle& handle);
 
+  // IsHandleValid applied to this object's handle.
+  bool IsValid() const;
+
   // Return invalid handle (see comment above for exact definition).
   static SharedMemoryHandle NULLHandle();
 
   // Creates a shared memory segment.
   // Returns true on success, false on failure.
   bool Create(size_t size);
 
   // Maps the shared memory into the caller's address space.
@@ -84,20 +88,29 @@ class SharedMemory {
   // created externally.
   // Returns 0 if not opened or unknown.
   size_t max_size() const { return max_size_; }
 
   // Gets a pointer to the opened memory space if it has been
   // Mapped via Map().  Returns NULL if it is not mapped.
   void* memory() const { return memory_; }
 
-  // Get access to the underlying OS handle for this segment.
-  // Use of this handle for anything other than an opaque
-  // identifier is not portable.
-  SharedMemoryHandle handle() const;
+  // Extracts the underlying file handle; similar to
+  // GiveToProcess(GetCurrentProcId(), ...) but returns a RAII type.
+  // Like GiveToProcess, this unmaps the memory as a side-effect.
+  mozilla::UniqueFileHandle TakeHandle();
+
+#ifdef OS_WIN
+  // Used only in gfx/ipc/SharedDIBWin.cpp; should be removable once
+  // NPAPI goes away.
+  HANDLE GetHandle() {
+    freezeable_ = false;
+    return mapped_file_;
+  }
+#endif
 
   // Closes the open shared memory segment.
   // It is safe to call Close repeatedly.
   void Close(bool unmap_view = true);
 
   // Returns a page-aligned address at which the given number of bytes could
   // probably be mapped.  Returns NULL on error or if there is insufficient
   // contiguous address space to map the required number of pages.
--- a/ipc/chromium/src/base/shared_memory_posix.cc
+++ b/ipc/chromium/src/base/shared_memory_posix.cc
@@ -51,16 +51,18 @@ bool SharedMemory::SetHandle(SharedMemor
   return true;
 }
 
 // static
 bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
   return handle.fd >= 0;
 }
 
+bool SharedMemory::IsValid() const { return mapped_file_ >= 0; }
+
 // static
 SharedMemoryHandle SharedMemory::NULLHandle() { return SharedMemoryHandle(); }
 
 // static
 bool SharedMemory::AppendPosixShmPrefix(std::string* str, pid_t pid) {
 #if defined(ANDROID)
   return false;
 #else
@@ -214,13 +216,16 @@ void SharedMemory::Close(bool unmap_view
   }
 
   if (mapped_file_ >= 0) {
     close(mapped_file_);
     mapped_file_ = -1;
   }
 }
 
-SharedMemoryHandle SharedMemory::handle() const {
-  return FileDescriptor(mapped_file_, false);
+mozilla::UniqueFileHandle SharedMemory::TakeHandle() {
+  mozilla::UniqueFileHandle fh(mapped_file_);
+  mapped_file_ = -1;
+  Unmap();
+  return fh;
 }
 
 }  // namespace base
--- a/ipc/chromium/src/base/shared_memory_win.cc
+++ b/ipc/chromium/src/base/shared_memory_win.cc
@@ -90,16 +90,18 @@ bool SharedMemory::SetHandle(SharedMemor
   return true;
 }
 
 // static
 bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
   return handle != NULL;
 }
 
+bool SharedMemory::IsValid() const { return mapped_file_ != NULL; }
+
 // static
 SharedMemoryHandle SharedMemory::NULLHandle() { return NULL; }
 
 bool SharedMemory::Create(size_t size) {
   DCHECK(mapped_file_ == NULL);
   read_only_ = false;
   mapped_file_ = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE,
                                    0, static_cast<DWORD>(size), NULL);
@@ -179,11 +181,16 @@ void SharedMemory::Close(bool unmap_view
   }
 
   if (mapped_file_ != NULL) {
     CloseHandle(mapped_file_);
     mapped_file_ = NULL;
   }
 }
 
-SharedMemoryHandle SharedMemory::handle() const { return mapped_file_; }
+mozilla::UniqueFileHandle SharedMemory::TakeHandle() {
+  mozilla::UniqueFileHandle fh(mapped_file_);
+  mapped_file_ = NULL;
+  Unmap();
+  return fh;
+}
 
 }  // namespace base
--- a/ipc/glue/ProcessUtils.h
+++ b/ipc/glue/ProcessUtils.h
@@ -22,37 +22,32 @@ void SetThisProcessName(const char* aNam
 class SharedPreferenceSerializer final {
  public:
   SharedPreferenceSerializer();
   SharedPreferenceSerializer(SharedPreferenceSerializer&& aOther);
   ~SharedPreferenceSerializer();
 
   bool SerializeToSharedMemory();
 
-  base::SharedMemoryHandle GetSharedMemoryHandle() const {
-    return mShm.handle();
-  }
+  size_t GetPrefMapSize() const { return mPrefMapSize; }
+  size_t GetPrefsLength() const { return mPrefsLength; }
 
-  const FileDescriptor::UniquePlatformHandle& GetPrefMapHandle() const {
-    return mPrefMapHandle;
-  }
+  const UniqueFileHandle& GetPrefsHandle() const { return mPrefsHandle; }
 
-  nsACString::size_type GetPrefLength() const { return mPrefs.Length(); }
-
-  size_t GetPrefMapSize() const { return mPrefMapSize; }
+  const UniqueFileHandle& GetPrefMapHandle() const { return mPrefMapHandle; }
 
   void AddSharedPrefCmdLineArgs(GeckoChildProcessHost& procHost,
                                 std::vector<std::string>& aExtraOpts) const;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SharedPreferenceSerializer);
   size_t mPrefMapSize;
-  FileDescriptor::UniquePlatformHandle mPrefMapHandle;
-  base::SharedMemory mShm;
-  nsAutoCStringN<1024> mPrefs;
+  size_t mPrefsLength;
+  UniqueFileHandle mPrefMapHandle;
+  UniqueFileHandle mPrefsHandle;
 };
 
 class SharedPreferenceDeserializer final {
  public:
   SharedPreferenceDeserializer();
   ~SharedPreferenceDeserializer();
 
   bool DeserializeFromSharedMemory(char* aPrefsHandleStr,
--- a/ipc/glue/ProcessUtils_common.cpp
+++ b/ipc/glue/ProcessUtils_common.cpp
@@ -19,79 +19,82 @@ SharedPreferenceSerializer::SharedPrefer
 
 SharedPreferenceSerializer::~SharedPreferenceSerializer() {
   MOZ_COUNT_DTOR(SharedPreferenceSerializer);
 }
 
 SharedPreferenceSerializer::SharedPreferenceSerializer(
     SharedPreferenceSerializer&& aOther)
     : mPrefMapSize(aOther.mPrefMapSize),
+      mPrefsLength(aOther.mPrefsLength),
       mPrefMapHandle(std::move(aOther.mPrefMapHandle)),
-      mShm(std::move(aOther.mShm)),
-      mPrefs(std::move(aOther.mPrefs)) {
+      mPrefsHandle(std::move(aOther.mPrefsHandle)) {
   MOZ_COUNT_CTOR(SharedPreferenceSerializer);
 }
 
 bool SharedPreferenceSerializer::SerializeToSharedMemory() {
   mPrefMapHandle =
-      Preferences::EnsureSnapshot(&mPrefMapSize).ClonePlatformHandle();
+      Preferences::EnsureSnapshot(&mPrefMapSize).TakePlatformHandle();
 
   // Serialize the early prefs.
-  Preferences::SerializePreferences(mPrefs);
+  nsAutoCStringN<1024> prefs;
+  Preferences::SerializePreferences(prefs);
+  mPrefsLength = prefs.Length();
 
+  base::SharedMemory shm;
   // Set up the shared memory.
-  if (!mShm.Create(mPrefs.Length())) {
+  if (!shm.Create(prefs.Length())) {
     NS_ERROR("failed to create shared memory in the parent");
     return false;
   }
-  if (!mShm.Map(mPrefs.Length())) {
+  if (!shm.Map(prefs.Length())) {
     NS_ERROR("failed to map shared memory in the parent");
     return false;
   }
 
   // Copy the serialized prefs into the shared memory.
-  memcpy(static_cast<char*>(mShm.memory()), mPrefs.get(), mPrefs.Length());
+  memcpy(static_cast<char*>(shm.memory()), prefs.get(), mPrefsLength);
 
+  mPrefsHandle = shm.TakeHandle();
   return true;
 }
 
 void SharedPreferenceSerializer::AddSharedPrefCmdLineArgs(
     mozilla::ipc::GeckoChildProcessHost& procHost,
     std::vector<std::string>& aExtraOpts) const {
   // Formats a pointer or pointer-sized-integer as a string suitable for passing
   // in an arguments list.
   auto formatPtrArg = [](auto arg) {
     return nsPrintfCString("%zu", uintptr_t(arg));
   };
 
 #if defined(XP_WIN)
   // Record the handle as to-be-shared, and pass it via a command flag. This
   // works because Windows handles are system-wide.
-  HANDLE prefsHandle = GetSharedMemoryHandle();
-  procHost.AddHandleToShare(prefsHandle);
+  procHost.AddHandleToShare(GetPrefsHandle().get());
   procHost.AddHandleToShare(GetPrefMapHandle().get());
   aExtraOpts.push_back("-prefsHandle");
-  aExtraOpts.push_back(formatPtrArg(prefsHandle).get());
+  aExtraOpts.push_back(formatPtrArg(GetPrefsHandle().get()).get());
   aExtraOpts.push_back("-prefMapHandle");
   aExtraOpts.push_back(formatPtrArg(GetPrefMapHandle().get()).get());
 #else
   // In contrast, Unix fds are per-process. So remap the fd to a fixed one that
   // will be used in the child.
   // XXX: bug 1440207 is about improving how fixed fds are used.
   //
   // Note: on Android, AddFdToRemap() sets up the fd to be passed via a Parcel,
   // and the fixed fd isn't used. However, we still need to mark it for
   // remapping so it doesn't get closed in the child.
-  procHost.AddFdToRemap(GetSharedMemoryHandle().fd, kPrefsFileDescriptor);
+  procHost.AddFdToRemap(GetPrefsHandle().get(), kPrefsFileDescriptor);
   procHost.AddFdToRemap(GetPrefMapHandle().get(), kPrefMapFileDescriptor);
 #endif
 
   // Pass the lengths via command line flags.
   aExtraOpts.push_back("-prefsLen");
-  aExtraOpts.push_back(formatPtrArg(GetPrefLength()).get());
+  aExtraOpts.push_back(formatPtrArg(GetPrefsLength()).get());
   aExtraOpts.push_back("-prefMapSize");
   aExtraOpts.push_back(formatPtrArg(GetPrefMapSize()).get());
 }
 
 #ifdef ANDROID
 static int gPrefsFd = -1;
 static int gPrefMapFd = -1;