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

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

gfx/ipc/SharedDIB.cpp

@@ -22,11 +22,7 @@ nsresult SharedDIB::Create(uint32_t aSize) {
   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;

gfx/ipc/SharedDIBWin.cpp

@@ -100,8 +100,9 @@ nsresult SharedDIBWin::SetupSurface(HDC aHdc, BITMAPV4HEADER* aHdr) {
 
   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);

ipc/chromium/src/base/shared_memory.h

@@ -18,6 +18,7 @@
 
 #include "base/basictypes.h"
 #include "base/process.h"
+#include "mozilla/UniquePtrExtensions.h"
 
 namespace base {
 
@@ -57,6 +58,9 @@ class SharedMemory {
   // 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();
 
@@ -89,10 +93,19 @@ class SharedMemory {
   // 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.

ipc/chromium/src/base/shared_memory_posix.cc

@@ -56,6 +56,8 @@ bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
   return handle.fd >= 0;
 }
 
+bool SharedMemory::IsValid() const { return mapped_file_ >= 0; }
+
 // static
 SharedMemoryHandle SharedMemory::NULLHandle() { return SharedMemoryHandle(); }
 
@@ -219,8 +221,11 @@ void SharedMemory::Close(bool unmap_view) {
   }
 }
 
-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

ipc/chromium/src/base/shared_memory_win.cc

@@ -95,6 +95,8 @@ bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
   return handle != NULL;
 }
 
+bool SharedMemory::IsValid() const { return mapped_file_ != NULL; }
+
 // static
 SharedMemoryHandle SharedMemory::NULLHandle() { return NULL; }
 
@@ -184,6 +186,11 @@ void SharedMemory::Close(bool unmap_view) {
   }
 }
 
-SharedMemoryHandle SharedMemory::handle() const { return mapped_file_; }
+mozilla::UniqueFileHandle SharedMemory::TakeHandle() {
+  mozilla::UniqueFileHandle fh(mapped_file_);
+  mapped_file_ = NULL;
+  Unmap();
+  return fh;
+}
 
 }  // namespace base

ipc/glue/ProcessUtils.h

@@ -27,17 +27,12 @@ class SharedPreferenceSerializer final {
 
   bool SerializeToSharedMemory();
 
-  base::SharedMemoryHandle GetSharedMemoryHandle() const {
-    return mShm.handle();
-  }
-
-  const FileDescriptor::UniquePlatformHandle& GetPrefMapHandle() const {
-    return mPrefMapHandle;
-  }
-
-  nsACString::size_type GetPrefLength() const { return mPrefs.Length(); }
-
   size_t GetPrefMapSize() const { return mPrefMapSize; }
+  size_t GetPrefsLength() const { return mPrefsLength; }
+
+  const UniqueFileHandle& GetPrefsHandle() const { return mPrefsHandle; }
+
+  const UniqueFileHandle& GetPrefMapHandle() const { return mPrefMapHandle; }
 
   void AddSharedPrefCmdLineArgs(GeckoChildProcessHost& procHost,
                                 std::vector<std::string>& aExtraOpts) const;
@@ -45,9 +40,9 @@ class SharedPreferenceSerializer final {
  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 {

ipc/glue/ProcessUtils_common.cpp

@@ -24,32 +24,36 @@ SharedPreferenceSerializer::~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;
 }
 
@@ -65,11 +69,10 @@ void SharedPreferenceSerializer::AddSharedPrefCmdLineArgs(
 #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
@@ -80,13 +83,13 @@ void SharedPreferenceSerializer::AddSharedPrefCmdLineArgs(
   // 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());
 }