Bug 1245241 - part 1 - Close Shmem file handles after mapping them wh…

…en possible to reduce exhaustion issues. r=billm

gfx/layers/ipc/CompositorChild.cpp

@@ -450,7 +450,8 @@ CompositorChild::SharedFrameMetricsData::SharedFrameMetricsData(
   , mLayersId(aLayersId)
   , mAPZCId(aAPZCId)
 {
-  mBuffer = new ipc::SharedMemoryBasic(metrics);
+  mBuffer = new ipc::SharedMemoryBasic;
+  mBuffer->SetHandle(metrics);
   mBuffer->Map(sizeof(FrameMetrics));
   mMutex = new CrossProcessMutex(handle);
   MOZ_COUNT_CTOR(SharedFrameMetricsData);

ipc/chromium/src/base/shared_memory.h

@@ -43,17 +43,18 @@ class SharedMemory {
 
   // Create a new SharedMemory object from an existing, open
   // shared memory file.
-  SharedMemory(SharedMemoryHandle handle, bool read_only);
-
-  // Create a new SharedMemory object from an existing, open
-  // shared memory file that was created by a remote process and not shared
-  // to the current process.
-  SharedMemory(SharedMemoryHandle handle, bool read_only,
-      base::ProcessHandle process);
+  SharedMemory(SharedMemoryHandle init_handle, bool read_only)
+    : SharedMemory() {
+    SetHandle(init_handle, read_only);
+  }
 
   // Destructor.  Will close any open files.
   ~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);
@@ -116,7 +117,7 @@ class SharedMemory {
 
   // Closes the open shared memory segment.
   // It is safe to call Close repeatedly.
-  void Close();
+  void Close(bool unmap_view = true);
 
   // Share the shared memory to another process.  Attempts
   // to create a platform-specific new_handle which can be

ipc/chromium/src/base/shared_memory_posix.cc

@@ -26,33 +26,22 @@ SharedMemory::SharedMemory()
       max_size_(0) {
 }
 
-SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
-    : mapped_file_(handle.fd),
-      inode_(0),
-      memory_(NULL),
-      read_only_(read_only),
-      max_size_(0) {
-  struct stat st;
-  if (fstat(handle.fd, &st) == 0) {
-    // If fstat fails, then the file descriptor is invalid and we'll learn this
-    // fact when Map() fails.
-    inode_ = st.st_ino;
-  }
+SharedMemory::~SharedMemory() {
+  Close();
 }
 
-SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
-                           ProcessHandle process)
-    : mapped_file_(handle.fd),
-      memory_(NULL),
-      read_only_(read_only),
-      max_size_(0) {
-  // We don't handle this case yet (note the ignored parameter); let's die if
-  // someone comes calling.
-  NOTREACHED();
-}
+bool SharedMemory::SetHandle(SharedMemoryHandle handle, bool read_only) {
+  DCHECK(mapped_file_ == -1);
 
-SharedMemory::~SharedMemory() {
-  Close();
+  struct stat st;
+  if (fstat(handle.fd, &st) < 0) {
+    return false;
+  }
+
+  mapped_file_ = handle.fd;
+  inode_ = st.st_ino;
+  read_only_ = read_only;
+  return true;
 }
 
 // static
@@ -268,10 +257,12 @@ bool SharedMemory::ShareToProcessCommon(ProcessId processId,
 }
 
 
-void SharedMemory::Close() {
-  Unmap();
+void SharedMemory::Close(bool unmap_view) {
+  if (unmap_view) {
+    Unmap();
+  }
 
-  if (mapped_file_ > 0) {
+  if (mapped_file_ >= 0) {
     close(mapped_file_);
     mapped_file_ = -1;
   }

ipc/chromium/src/base/shared_memory_win.cc

@@ -19,34 +19,20 @@ SharedMemory::SharedMemory()
       lock_(NULL) {
 }
 
-SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
-    : mapped_file_(handle),
-      memory_(NULL),
-      read_only_(read_only),
-      max_size_(0),
-      lock_(NULL) {
-}
-
-SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
-                           ProcessHandle process)
-    : mapped_file_(NULL),
-      memory_(NULL),
-      read_only_(read_only),
-      max_size_(0),
-      lock_(NULL) {
-  ::DuplicateHandle(process, handle,
-                    GetCurrentProcess(), &mapped_file_,
-                    STANDARD_RIGHTS_REQUIRED |
-                    (read_only_ ? FILE_MAP_READ : FILE_MAP_ALL_ACCESS),
-                    FALSE, 0);
-}
-
 SharedMemory::~SharedMemory() {
   Close();
   if (lock_ != NULL)
     CloseHandle(lock_);
 }
 
+bool SharedMemory::SetHandle(SharedMemoryHandle handle, bool read_only) {
+  DCHECK(mapped_file_ == NULL);
+
+  mapped_file_ = handle;
+  read_only_ = read_only;
+  return true;
+}
+
 // static
 bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
   return handle != NULL;
@@ -151,10 +137,9 @@ bool SharedMemory::ShareToProcessCommon(ProcessId processId,
 }
 
 
-void SharedMemory::Close() {
-  if (memory_ != NULL) {
-    UnmapViewOfFile(memory_);
-    memory_ = NULL;
+void SharedMemory::Close(bool unmap_view) {
+  if (unmap_view) {
+    Unmap();
   }
 
   if (mapped_file_ != NULL) {

ipc/glue/CrossProcessMutex_posix.cpp

@@ -82,11 +82,15 @@ CrossProcessMutex::CrossProcessMutex(CrossProcessMutexHandle aHandle)
     : mMutex(nullptr)
     , mCount(nullptr)
 {
-  if (!ipc::SharedMemoryBasic::IsHandleValid(aHandle)) {
+  mSharedBuffer = new ipc::SharedMemoryBasic;
+
+  if (!mSharedBuffer->IsHandleValid(aHandle)) {
     MOZ_CRASH();
   }
 
-  mSharedBuffer = new ipc::SharedMemoryBasic(aHandle);
+  if (!mSharedBuffer->SetHandle(aHandle)) {
+    MOZ_CRASH();
+  }
 
   if (!mSharedBuffer->Map(sizeof(MutexData))) {
     MOZ_CRASH();

ipc/glue/SharedMemory.h

@@ -12,6 +12,9 @@
 #include "nsISupportsImpl.h"    // NS_INLINE_DECL_REFCOUNTING
 #include "mozilla/Attributes.h"
 
+#include "base/process.h"
+#include "chrome/common/ipc_message_utils.h"
+
 //
 // This is a low-level wrapper around platform shared memory.  Don't
 // use it directly; use Shmem allocated through IPDL interfaces.
@@ -55,8 +58,13 @@ class SharedMemory
   virtual bool Create(size_t size) = 0;
   virtual bool Map(size_t nBytes) = 0;
 
+  virtual void CloseHandle() = 0;
+
   virtual SharedMemoryType Type() const = 0;
 
+  virtual bool ShareHandle(base::ProcessId aProcessId, IPC::Message* aMessage) = 0;
+  virtual bool ReadHandle(const IPC::Message* aMessage, void** aIter) = 0;
+
   void
   Protect(char* aAddr, size_t aSize, int aRights)
   {
@@ -110,6 +118,35 @@ class SharedMemory
   size_t mMappedSize;
 };
 
+template<typename HandleImpl>
+class SharedMemoryCommon : public SharedMemory
+{
+public:
+  typedef HandleImpl Handle;
+
+  virtual bool ShareToProcess(base::ProcessId aProcessId, Handle* aHandle) = 0;
+  virtual bool IsHandleValid(const Handle& aHandle) const = 0;
+  virtual bool SetHandle(const Handle& aHandle) = 0;
+
+  virtual bool ShareHandle(base::ProcessId aProcessId, IPC::Message* aMessage) override
+  {
+    Handle handle;
+    if (!ShareToProcess(aProcessId, &handle)) {
+      return false;
+    }
+    IPC::WriteParam(aMessage, handle);
+    return true;
+  }
+
+  virtual bool ReadHandle(const IPC::Message* aMessage, void** aIter) override
+  {
+    Handle handle;
+    return IPC::ReadParam(aMessage, aIter, &handle) &&
+           IsHandleValid(handle) &&
+           SetHandle(handle);
+  }
+};
+
 } // namespace ipc
 } // namespace mozilla
 

ipc/glue/SharedMemoryBasic_android.cpp

@@ -40,15 +40,18 @@ SharedMemoryBasic::SharedMemoryBasic()
   , mMemory(nullptr)
 { }
 
-SharedMemoryBasic::SharedMemoryBasic(const Handle& aHandle)
-  : mShmFd(aHandle.fd)
-  , mMemory(nullptr)
-{ }
-
 SharedMemoryBasic::~SharedMemoryBasic()
 {
   Unmap();
-  Destroy();
+  CloseHandle();
+}
+
+bool
+SharedMemoryBasic::SetHandle(const Handle& aHandle)
+{
+  MOZ_ASSERT(-1 == mShmFd, "Already Create()d");
+  mShmFd = aHandle.fd;
+  return true;
 }
 
 bool
@@ -125,7 +128,7 @@ SharedMemoryBasic::Unmap()
 }
 
 void
-SharedMemoryBasic::Destroy()
+SharedMemoryBasic::CloseHandle()
 {
   if (mShmFd > 0) {
     close(mShmFd);

ipc/glue/SharedMemoryBasic_android.h

@@ -20,19 +20,19 @@
 namespace mozilla {
 namespace ipc {
 
-class SharedMemoryBasic final : public SharedMemory
+class SharedMemoryBasic final : public SharedMemoryCommon<base::FileDescriptor>
 {
 public:
-  typedef base::FileDescriptor Handle;
-
   SharedMemoryBasic();
 
-  SharedMemoryBasic(const Handle& aHandle);
+  virtual bool SetHandle(const Handle& aHandle) override;
 
   virtual bool Create(size_t aNbytes) override;
 
   virtual bool Map(size_t nBytes) override;
 
+  virtual void CloseHandle() override;
+
   virtual void* memory() const override
   {
     return mMemory;
@@ -48,19 +48,18 @@ class SharedMemoryBasic final : public SharedMemory
     return Handle();
   }
 
-  static bool IsHandleValid(const Handle &aHandle)
+  virtual bool IsHandleValid(const Handle &aHandle) const override
   {
     return aHandle.fd >= 0;
   }
 
-  bool ShareToProcess(base::ProcessId aProcessId,
-                      Handle* aNewHandle);
+  virtual bool ShareToProcess(base::ProcessId aProcessId,
+                              Handle* aNewHandle) override;
 
 private:
   ~SharedMemoryBasic();
 
   void Unmap();
-  void Destroy();
 
   // The /dev/ashmem fd we allocate.
   int mShmFd;

ipc/glue/SharedMemoryBasic_chromium.h

@@ -21,18 +21,15 @@
 namespace mozilla {
 namespace ipc {
 
-class SharedMemoryBasic final : public SharedMemory
+class SharedMemoryBasic final : public SharedMemoryCommon<base::SharedMemoryHandle>
 {
 public:
-  typedef base::SharedMemoryHandle Handle;
-
   SharedMemoryBasic()
   {
   }
 
-  explicit SharedMemoryBasic(const Handle& aHandle)
-    : mSharedMemory(aHandle, false)
-  {
+  virtual bool SetHandle(const Handle& aHandle) override {
+    return mSharedMemory.SetHandle(aHandle, false);
   }
 
   virtual bool Create(size_t aNbytes) override
@@ -53,6 +50,11 @@ class SharedMemoryBasic final : public SharedMemory
     return ok;
   }
 
+  virtual void CloseHandle() override
+  {
+    mSharedMemory.Close(false);
+  }
+
   virtual void* memory() const override
   {
     return mSharedMemory.memory();
@@ -68,13 +70,13 @@ class SharedMemoryBasic final : public SharedMemory
     return base::SharedMemory::NULLHandle();
   }
 
-  static bool IsHandleValid(const Handle &aHandle)
+  virtual bool IsHandleValid(const Handle &aHandle) const override
   {
     return base::SharedMemory::IsHandleValid(aHandle);
   }
 
-  bool ShareToProcess(base::ProcessId aProcessId,
-                      Handle* new_handle)
+  virtual bool ShareToProcess(base::ProcessId aProcessId,
+                              Handle* new_handle) override
   {
     base::SharedMemoryHandle handle;
     bool ret = mSharedMemory.ShareToProcess(aProcessId, &handle);