Bug 1479960 - Clean up shared_memory_posix error handling. r=froydnj
authorJed Davis <jld@mozilla.com>
Wed, 14 Aug 2019 22:48:31 +0000
changeset 488045 6da235c5a77c5fc23b11a7456c588974f1de878c
parent 488044 bdf7a041928aa4673de020df11c069e0af996d50
child 488046 ddfa5ff8106195214d932cedc963fe2f9191c5d0
push id36434
push usercbrindusan@mozilla.com
push dateThu, 15 Aug 2019 09:44:30 +0000
treeherdermozilla-central@144fbfb409b7 [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 - Clean up shared_memory_posix error handling. r=froydnj This uses RAII to handle error-case cleanup in the POSIX backend for SharedMemory::Create, to simplify the complexity that will be added to support freezing. Depends on D26741 Differential Revision: https://phabricator.services.mozilla.com/D26742
ipc/chromium/src/base/shared_memory_posix.cc
--- a/ipc/chromium/src/base/shared_memory_posix.cc
+++ b/ipc/chromium/src/base/shared_memory_posix.cc
@@ -15,16 +15,17 @@
 #ifdef ANDROID
 #  include <linux/ashmem.h>
 #endif
 
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "mozilla/Atomics.h"
+#include "mozilla/UniquePtrExtensions.h"
 #include "prenv.h"
 
 namespace base {
 
 SharedMemory::SharedMemory()
     : mapped_file_(-1),
       mapped_size_(0),
       memory_(nullptr),
@@ -99,68 +100,67 @@ bool SharedMemory::AppendPosixShmPrefix(
 }
 
 bool SharedMemory::Create(size_t size) {
   read_only_ = false;
 
   DCHECK(size > 0);
   DCHECK(mapped_file_ == -1);
 
-  int fd;
+  mozilla::UniqueFileHandle fd;
   bool needs_truncate = true;
 
 #ifdef ANDROID
   // Android has its own shared memory facility:
-  fd = open("/" ASHMEM_NAME_DEF, O_RDWR, 0600);
-  if (fd < 0) {
+  fd.reset(open("/" ASHMEM_NAME_DEF, O_RDWR, 0600));
+  if (!fd) {
     CHROMIUM_LOG(WARNING) << "failed to open shm: " << strerror(errno);
     return false;
   }
-  if (ioctl(fd, ASHMEM_SET_SIZE, size) != 0) {
+  if (ioctl(fd.get(), ASHMEM_SET_SIZE, size) != 0) {
     CHROMIUM_LOG(WARNING) << "failed to set shm size: " << strerror(errno);
-    close(fd);
     return false;
   }
   needs_truncate = false;
 #else
   // Generic Unix: shm_open + shm_unlink
   do {
     // The names don't need to be unique, but it saves time if they
     // usually are.
     static mozilla::Atomic<size_t> sNameCounter;
     std::string name;
     CHECK(AppendPosixShmPrefix(&name, getpid()));
     StringAppendF(&name, "%zu", sNameCounter++);
     // O_EXCL means the names being predictable shouldn't be a problem.
-    fd = HANDLE_EINTR(shm_open(name.c_str(), O_RDWR | O_CREAT | O_EXCL, 0600));
-    if (fd >= 0) {
+    fd.reset(
+        HANDLE_EINTR(shm_open(name.c_str(), O_RDWR | O_CREAT | O_EXCL, 0600)));
+    if (fd) {
       if (shm_unlink(name.c_str()) != 0) {
         // This shouldn't happen, but if it does: assume the file is
         // in fact leaked, and bail out now while it's still 0-length.
         DLOG(FATAL) << "failed to unlink shm: " << strerror(errno);
         return false;
       }
     }
-  } while (fd < 0 && errno == EEXIST);
+  } while (!fd && errno == EEXIST);
 #endif
 
-  if (fd < 0) {
+  if (!fd) {
     CHROMIUM_LOG(WARNING) << "failed to open shm: " << strerror(errno);
     return false;
   }
 
   if (needs_truncate) {
-    if (HANDLE_EINTR(ftruncate(fd, static_cast<off_t>(size))) != 0) {
+    if (HANDLE_EINTR(ftruncate(fd.get(), static_cast<off_t>(size))) != 0) {
       CHROMIUM_LOG(WARNING) << "failed to set shm size: " << strerror(errno);
-      close(fd);
       return false;
     }
   }
 
-  mapped_file_ = fd;
+  mapped_file_ = fd.release();
   max_size_ = size;
   return true;
 }
 
 bool SharedMemory::Map(size_t bytes, void* fixed_address) {
   if (mapped_file_ == -1) return false;
 
   // Don't use MAP_FIXED when a fixed_address was specified, since that can