Bug 1479960 - Make AutoMemMap not fstat() the mapped object if it doesn't need to. r=kmag
authorJed Davis <jld@mozilla.com>
Wed, 14 Aug 2019 22:48:36 +0000
changeset 488143 0f466f2a18c0fcf9e552ea07158e847f88ca9950
parent 488142 225411558a4eaa436ef6036eee1513427a156413
child 488144 1fa598bf26998154cfc2c189a2a5f7fb862c7640
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)
reviewerskmag
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 - Make AutoMemMap not fstat() the mapped object if it doesn't need to. r=kmag One problem with using shared memory instead of files for MemMapSnapshot is that AutoMemMap was trying to use fstat() to obtain the object size; that doesn't work with ashmem on Android and was causing problems with the Mac sandbox, but it's not necessary, because we already know the size. This patch changes it to not do that. Depends on D26743 Differential Revision: https://phabricator.services.mozilla.com/D26744
js/xpconnect/loader/AutoMemMap.cpp
js/xpconnect/loader/AutoMemMap.h
--- a/js/xpconnect/loader/AutoMemMap.cpp
+++ b/js/xpconnect/loader/AutoMemMap.cpp
@@ -34,58 +34,60 @@ Result<Ok, nsresult> AutoMemMap::init(ns
   MOZ_ASSERT(!fd);
 
   MOZ_TRY(file->OpenNSPRFileDesc(flags, mode, &fd.rwget()));
 
   return initInternal(prot);
 }
 
 Result<Ok, nsresult> AutoMemMap::init(const FileDescriptor& file,
-                                      PRFileMapProtect prot,
-                                      size_t expectedSize) {
+                                      PRFileMapProtect prot, size_t maybeSize) {
   MOZ_ASSERT(!fd);
   if (!file.IsValid()) {
     return Err(NS_ERROR_INVALID_ARG);
   }
 
   auto handle = file.ClonePlatformHandle();
 
   fd = PR_ImportFile(PROsfd(handle.get()));
   if (!fd) {
     return Err(NS_ERROR_FAILURE);
   }
   Unused << handle.release();
 
-  return initInternal(prot, expectedSize);
+  return initInternal(prot, maybeSize);
 }
 
 Result<Ok, nsresult> AutoMemMap::initInternal(PRFileMapProtect prot,
-                                              size_t expectedSize) {
+                                              size_t maybeSize) {
   MOZ_ASSERT(!fileMap);
   MOZ_ASSERT(!addr);
 
-  PRFileInfo64 fileInfo;
-  MOZ_TRY(PR_GetOpenFileInfo64(fd.get(), &fileInfo));
+  if (maybeSize > 0) {
+    // Some OSes' shared memory objects can't be stat()ed, either at
+    // all (Android) or without loosening the sandbox (Mac) so just
+    // use the size.
+    size_ = maybeSize;
+  } else {
+    // But if we don't have the size, assume it's a regular file and
+    // ask for it.
+    PRFileInfo64 fileInfo;
+    MOZ_TRY(PR_GetOpenFileInfo64(fd.get(), &fileInfo));
 
-  if (fileInfo.size > UINT32_MAX) {
-    return Err(NS_ERROR_INVALID_ARG);
+    if (fileInfo.size > UINT32_MAX) {
+      return Err(NS_ERROR_INVALID_ARG);
+    }
+    size_ = fileInfo.size;
   }
 
   fileMap = PR_CreateFileMap(fd, 0, prot);
   if (!fileMap) {
     return Err(NS_ERROR_FAILURE);
   }
 
-  size_ = fileInfo.size;
-  // The memory region size passed in certain IPC messages isn't necessary on
-  // Unix-like systems, since we can always stat the file descriptor to
-  // determine it accurately. But since we have it, anyway, sanity check that
-  // it matches the size returned by the stat.
-  MOZ_ASSERT_IF(expectedSize > 0, size_ == expectedSize);
-
   addr = PR_MemMap(fileMap, 0, size_);
   if (!addr) {
     return Err(NS_ERROR_FAILURE);
   }
 
   return Ok();
 }
 
@@ -117,17 +119,18 @@ FileDescriptor AutoMemMap::cloneHandle()
   return FileDescriptor(handle_);
 }
 
 #else
 
 Result<Ok, nsresult> AutoMemMap::initWithHandle(const FileDescriptor& file,
                                                 size_t size,
                                                 PRFileMapProtect prot) {
-  return init(file, prot);
+  MOZ_DIAGNOSTIC_ASSERT(size > 0);
+  return init(file, prot, size);
 }
 
 FileDescriptor AutoMemMap::cloneHandle() const { return cloneFileDescriptor(); }
 
 #endif
 
 void AutoMemMap::reset() {
   if (addr && !persistent_) {
--- a/js/xpconnect/loader/AutoMemMap.h
+++ b/js/xpconnect/loader/AutoMemMap.h
@@ -31,17 +31,17 @@ class AutoMemMap {
 
   ~AutoMemMap();
 
   Result<Ok, nsresult> init(nsIFile* file, int flags = PR_RDONLY, int mode = 0,
                             PRFileMapProtect prot = PR_PROT_READONLY);
 
   Result<Ok, nsresult> init(const FileDescriptor& file,
                             PRFileMapProtect prot = PR_PROT_READONLY,
-                            size_t expectedSize = 0);
+                            size_t maybeSize = 0);
 
   // Initializes the mapped memory with a shared memory handle. On
   // Unix-like systems, this is identical to the above init() method. On
   // Windows, the FileDescriptor must be a handle for a file mapping,
   // rather than a file descriptor.
   Result<Ok, nsresult> initWithHandle(const FileDescriptor& file, size_t size,
                                       PRFileMapProtect prot = PR_PROT_READONLY);
 
@@ -68,18 +68,18 @@ class AutoMemMap {
   FileDescriptor cloneFileDescriptor() const;
   FileDescriptor cloneHandle() const;
 
   // Makes this mapping persistent. After calling this, the mapped memory
   // will remained mapped, even after this instance is destroyed.
   void setPersistent() { persistent_ = true; }
 
  private:
-  Result<Ok, nsresult> initInternal(PRFileMapProtect prot = PR_PROT_READONLY,
-                                    size_t expectedSize = 0);
+  Result<Ok, nsresult> initInternal(PRFileMapProtect prot,
+                                    size_t maybeSize = 0);
 
   AutoFDClose fd;
   PRFileMap* fileMap = nullptr;
 
 #ifdef XP_WIN
   // We can't include windows.h in this header, since it gets included
   // by some binding headers (which are explicitly incompatible with
   // windows.h). So we can't use the HANDLE type here.