Refactor DiscardableMemory for the upcoming DiscardableMemoryAllocator.

base/memory/discardable_memory_android.cc

Index: base/memory/discardable_memory_android.cc
diff --git a/base/memory/discardable_memory_android.cc b/base/memory/discardable_memory_android.cc
index 73a25ae419e897b7bf2f3f903edd931a1599be27..ffead39307e7a8c9a2a071a1179b30ed8e61b910 100644
--- a/base/memory/discardable_memory_android.cc
+++ b/base/memory/discardable_memory_android.cc
@@ -2,19 +2,24 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/memory/discardable_memory.h"
+#include "base/memory/discardable_memory_android.h"
 
 #include <sys/mman.h>
 #include <unistd.h>
 
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/synchronization/lock.h"
 #include "third_party/ashmem/ashmem.h"
 
+namespace base {
 namespace {
 
+// Protects |g_num_discardable_memory| below.
 base::LazyInstance<base::Lock>::Leaky g_discardable_memory_lock =
     LAZY_INSTANCE_INITIALIZER;
 
@@ -25,140 +30,141 @@ int g_num_discardable_memory = 0;
 // descriptor limit.
 const int kDiscardableMemoryNumLimit = 128;
 
-}
+class DiscardableMemoryAndroid : public DiscardableMemory {
+ public:
+  DiscardableMemoryAndroid(int fd, void* address, size_t size)
+      : fd_(fd),
+        memory_(address),
+        size_(size) {
+    DCHECK_GE(fd_, 0);
+    DCHECK(memory_);
+  }
 
-namespace base {
+  virtual ~DiscardableMemoryAndroid() {
+    internal::DeleteAshmemRegion(fd_, size_, memory_);
+  }
+
+  // DiscardableMemory:
+  virtual LockDiscardableMemoryStatus Lock() OVERRIDE {
+    return internal::LockAshmemRegion(fd_, 0, 0, memory_);
+  }
+
+  virtual void Unlock() OVERRIDE {
+    internal::UnlockAshmemRegion(fd_, 0, 0, memory_);
+  }
+
+  virtual void* Memory() const OVERRIDE {
+    return memory_;
+  }
+
+ private:
+  const int fd_;
+  void* const memory_;
+  const size_t size_;
+
+  DISALLOW_COPY_AND_ASSIGN(DiscardableMemoryAndroid);
+};
+
+}  // namespace
 
 // static
 bool DiscardableMemory::Supported() {
   return true;
 }
 
-DiscardableMemory::~DiscardableMemory() {
-  if (is_locked_)
-    Unlock();
-  // If fd_ is smaller than 0, initialization must have failed and
-  // g_num_discardable_memory is not incremented by the caller.
-  if (fd_ < 0)
-    return;
-  HANDLE_EINTR(close(fd_));
-  fd_ = -1;
-  ReleaseFileDescriptor();
+// static
+scoped_ptr<DiscardableMemory> DiscardableMemory::CreateLockedMemory(
+    size_t size) {
+  int fd;
+  void* address;
+  if (!internal::CreateAshmemRegion("", size, &fd, &address))
+    return scoped_ptr<DiscardableMemory>();
+  return scoped_ptr<DiscardableMemory>(
+      new DiscardableMemoryAndroid(fd, address, size));
 }
 
-bool DiscardableMemory::ReserveFileDescriptor() {
-  base::AutoLock lock(g_discardable_memory_lock.Get());
-  if (g_num_discardable_memory < kDiscardableMemoryNumLimit) {
-    ++g_num_discardable_memory;
-    return true;
-  }
+// static
+bool DiscardableMemory::PurgeForTestingSupported() {
   return false;
 }
 
-void DiscardableMemory::ReleaseFileDescriptor() {
-  base::AutoLock lock(g_discardable_memory_lock.Get());
-  --g_num_discardable_memory;
-  DCHECK_LE(0, g_num_discardable_memory);
+// static
+void DiscardableMemory::PurgeForTesting() {
+  NOTIMPLEMENTED();
 }
 
-bool DiscardableMemory::InitializeAndLock(size_t size) {
-  // When this function returns true, fd_ should be larger or equal than 0
-  // and g_num_discardable_memory is incremented by 1. Otherwise, fd_
-  // is less than 0 and g_num_discardable_memory is not incremented by
-  // the caller.
-  DCHECK_EQ(fd_, -1);
-  DCHECK(!memory_);
-  if (!ReserveFileDescriptor())
-    return false;
-
-  size_ = size;
-  fd_ = ashmem_create_region("", size);
+namespace internal {
 
-  if (fd_ < 0) {
+bool CreateAshmemRegion(const char* name,
+                        size_t size,
+                        int* out_fd,
+                        void** out_address) {
+  base::AutoLock lock(g_discardable_memory_lock.Get());
+  if (g_num_discardable_memory + 1 > kDiscardableMemoryNumLimit)
+    return false;
+  int fd = ashmem_create_region(name, size);
+  if (fd < 0) {
     DLOG(ERROR) << "ashmem_create_region() failed";
-    ReleaseFileDescriptor();
     return false;
   }
+  file_util::ScopedFD fd_closer(&fd);
 
-  int err = ashmem_set_prot_region(fd_, PROT_READ | PROT_WRITE);
+  const int err = ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE);
   if (err < 0) {
     DLOG(ERROR) << "Error " << err << " when setting protection of ashmem";
-    HANDLE_EINTR(close(fd_));
-    fd_ = -1;
-    ReleaseFileDescriptor();
     return false;
   }
 
-  if (!Map()) {
-    // Close the file descriptor in case of any initialization errors.
-    HANDLE_EINTR(close(fd_));
-    fd_ = -1;
-    ReleaseFileDescriptor();
-    return false;
-  }
-
-  is_locked_ = true;
-  return true;
-}
-
-LockDiscardableMemoryStatus DiscardableMemory::Lock() {
-  DCHECK_NE(fd_, -1);
-  DCHECK(!is_locked_);
-
-  bool purged = false;
-  if (ashmem_pin_region(fd_, 0, 0) == ASHMEM_WAS_PURGED)
-    purged = true;
-
-  if (!Map())
-    return DISCARDABLE_MEMORY_FAILED;
-
-  is_locked_ = true;
-  return purged ? DISCARDABLE_MEMORY_PURGED : DISCARDABLE_MEMORY_SUCCESS;
-}
-
-void DiscardableMemory::Unlock() {
-  DCHECK_GE(fd_, 0);
-  DCHECK(is_locked_);
-
-  Unmap();
-  if (ashmem_unpin_region(fd_, 0, 0))
-    DLOG(ERROR) << "Failed to unpin memory.";
-  is_locked_ = false;
-}
-
-bool DiscardableMemory::Map() {
-  DCHECK(!memory_);
   // There is a problem using MAP_PRIVATE here. As we are constantly calling
   // Lock() and Unlock(), data could get lost if they are not written to the
   // underlying file when Unlock() gets called.
-  memory_ = mmap(NULL, size_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_, 0);
-  if (memory_ == (void*)-1) {
+  void* const address = mmap(
+      NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+  if (address == MAP_FAILED) {
     DPLOG(ERROR) << "Failed to map memory.";
-    memory_ = NULL;
-    if (ashmem_unpin_region(fd_, 0, 0))
-      DLOG(ERROR) << "Failed to unpin memory.";
     return false;
   }
+
+  ignore_result(fd_closer.release());
+  ++g_num_discardable_memory;
+  *out_fd = fd;
+  *out_address = address;
   return true;
 }
 
-void DiscardableMemory::Unmap() {
-  DCHECK(memory_);
-
-  if (-1 == munmap(memory_, size_))
+bool DeleteAshmemRegion(int fd, size_t size, void* address) {
+  base::AutoLock lock(g_discardable_memory_lock.Get());
+  --g_num_discardable_memory;
+  if (munmap(address, size) == -1) {
     DPLOG(ERROR) << "Failed to unmap memory.";
-
-  memory_ = NULL;
+    HANDLE_EINTR(close(fd));

[willchan no longer on Chromium, 2013/10/17 01:21:37]

Please drop this HANDLE_EINTR:
https://code.google.com/p/chromium/issues/detail?id=269623.

[Philippe, 2013/10/17 08:56:24]

Thanks, I didn't know about this issue.

+    return false;
+  }
+  return HANDLE_EINTR(close(fd)) == 0;

[willchan no longer on Chromium, 2013/10/17 01:21:37]

Ditto

[Philippe, 2013/10/17 08:56:24]

Done.

 }
 
-// static
-bool DiscardableMemory::PurgeForTestingSupported() {
-  return false;
+LockDiscardableMemoryStatus LockAshmemRegion(int fd,
+                                             size_t off,
+                                             size_t size,
+                                             const void* address) {
+  const int result = ashmem_pin_region(fd, off, size);
+#if !defined(NDEBUG)

[willchan no longer on Chromium, 2013/10/17 01:21:37]

I don't get this. Why is this NDEBUG stuff needed? In opt mode, DCHECK_EQ will
get compiled out anyway.

[Philippe, 2013/10/17 08:56:24]

Yeah, good point. I guess I added the DCHECK after the mprotect without thinking
about this.

+  DCHECK_EQ(0, mprotect(address, size, PROT_READ | PROT_WRITE));
+#endif
+  return result == ASHMEM_WAS_PURGED ?
+      DISCARDABLE_MEMORY_PURGED : DISCARDABLE_MEMORY_SUCCESS;
 }
 
-// static
-void DiscardableMemory::PurgeForTesting() {
-  NOTIMPLEMENTED();
+bool UnlockAshmemRegion(int fd, size_t off, size_t size, const void* address) {
+  const int failed = ashmem_unpin_region(fd, off, size);
+  if (failed)
+    DLOG(ERROR) << "Failed to unpin memory.";
+#if !defined(NDEBUG)
+  // This allows us to catch accesses to unlocked memory.
+  DCHECK_EQ(0, mprotect(address, size, PROT_NONE));
+#endif
+  return !failed;
 }
 
+}  // namespace internal
 }  // namespace base