A couple of incremental improvements for DiscardableMemory.

Unify DCHECKs so that implementation is_locked_ refers uniformly to the
client's perspective, not the underlying implementation's perspective or
the manager's perspective.

Unify Initialize methods to check for "not failed" instead of a
particular form of success.

Don't reset page protection to default after a fresh allocation, which
already delivers pages with default protection.

R=reveman@chromium.org, reveman

Review URL: https://codereview.chromium.org/285493004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270135 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 27 insertions, 19 deletions

diff --git a/base/memory/discardable_memory_ashmem.cc b/base/memory/discardable_memory_ashmem.cc
index f830008..a590e53 100644
--- a/base/memory/discardable_memory_ashmem.cc
+++ b/base/memory/discardable_memory_ashmem.cc
@@ -22,37 +22,40 @@ DiscardableMemoryAshmem::DiscardableMemoryAshmem(
 
 DiscardableMemoryAshmem::~DiscardableMemoryAshmem() {
   if (is_locked_)
-    manager_->ReleaseLock(this);
+    Unlock();
 
   manager_->Unregister(this);
 }
 
 bool DiscardableMemoryAshmem::Initialize() {
-  return Lock() == DISCARDABLE_MEMORY_LOCK_STATUS_PURGED;
+  return Lock() != DISCARDABLE_MEMORY_LOCK_STATUS_FAILED;
 }
 
 DiscardableMemoryLockStatus DiscardableMemoryAshmem::Lock() {
+  DCHECK(!is_locked_);
+
   bool purged = false;
   if (!manager_->AcquireLock(this, &purged))
     return DISCARDABLE_MEMORY_LOCK_STATUS_FAILED;
 
+  is_locked_ = true;
   return purged ? DISCARDABLE_MEMORY_LOCK_STATUS_PURGED
                 : DISCARDABLE_MEMORY_LOCK_STATUS_SUCCESS;
 }
 
 void DiscardableMemoryAshmem::Unlock() {
+  DCHECK(is_locked_);
   manager_->ReleaseLock(this);
+  is_locked_ = false;
 }
 
 void* DiscardableMemoryAshmem::Memory() const {
+  DCHECK(is_locked_);
   DCHECK(ashmem_chunk_);
   return ashmem_chunk_->Memory();
 }
 
 bool DiscardableMemoryAshmem::AllocateAndAcquireLock() {
-  DCHECK(!is_locked_);
-  is_locked_ = true;
-
   if (ashmem_chunk_)
     return ashmem_chunk_->Lock();
 
@@ -61,13 +64,10 @@ bool DiscardableMemoryAshmem::AllocateAndAcquireLock() {
 }
 
 void DiscardableMemoryAshmem::ReleaseLock() {
-  DCHECK(is_locked_);
   ashmem_chunk_->Unlock();
-  is_locked_ = false;
 }
 
 void DiscardableMemoryAshmem::Purge() {
-  DCHECK(!is_locked_);
   ashmem_chunk_.reset();
 }
 
diff --git a/base/memory/discardable_memory_emulated.cc b/base/memory/discardable_memory_emulated.cc
index 2659f0f..b705402 100644
--- a/base/memory/discardable_memory_emulated.cc
+++ b/base/memory/discardable_memory_emulated.cc
@@ -56,7 +56,7 @@ void DiscardableMemoryEmulated::PurgeForTesting() {
 }
 
 bool DiscardableMemoryEmulated::Initialize() {
-  return Lock() == DISCARDABLE_MEMORY_LOCK_STATUS_PURGED;
+  return Lock() != DISCARDABLE_MEMORY_LOCK_STATUS_FAILED;
 }
 
 DiscardableMemoryLockStatus DiscardableMemoryEmulated::Lock() {
diff --git a/base/memory/discardable_memory_mac.cc b/base/memory/discardable_memory_mac.cc
index d8c0ada..b2982fc 100644
--- a/base/memory/discardable_memory_mac.cc
+++ b/base/memory/discardable_memory_mac.cc
@@ -47,7 +47,7 @@ class DiscardableMemoryMac
     g_shared_state.Pointer()->manager.Register(this, bytes);
   }
 
-  bool Initialize() { return Lock() == DISCARDABLE_MEMORY_LOCK_STATUS_PURGED; }
+  bool Initialize() { return Lock() != DISCARDABLE_MEMORY_LOCK_STATUS_FAILED; }
 
   virtual ~DiscardableMemoryMac() {
     if (is_locked_)
@@ -81,8 +81,8 @@ class DiscardableMemoryMac
 
   // Overridden from internal::DiscardableMemoryManagerAllocation:
   virtual bool AllocateAndAcquireLock() OVERRIDE {
-    bool persistent = true;
     kern_return_t ret;
+    bool persistent;
     if (!memory_.size()) {
       vm_address_t address = 0;
       ret = vm_allocate(
@@ -92,17 +92,24 @@ class DiscardableMemoryMac
           VM_FLAGS_ANYWHERE | VM_FLAGS_PURGABLE | kDiscardableMemoryTag);
       MACH_CHECK(ret == KERN_SUCCESS, ret) << "vm_allocate";
       memory_.reset(address, bytes_);
+
+      // When making a fresh allocation, it's impossible for |persistent| to
+      // be true.
       persistent = false;
-    }
+    } else {
+      // |persistent| will be reset to false below if appropriate, but when
+      // reusing an existing allocation, it's possible for it to be true.
+      persistent = true;
 
 #if !defined(NDEBUG)
-    ret = vm_protect(mach_task_self(),
-                     memory_.address(),
-                     memory_.size(),
-                     FALSE,
-                     VM_PROT_DEFAULT);
-    MACH_DCHECK(ret == KERN_SUCCESS, ret) << "vm_protect";
+      ret = vm_protect(mach_task_self(),
+                       memory_.address(),
+                       memory_.size(),
+                       FALSE,
+                       VM_PROT_DEFAULT);
+      MACH_DCHECK(ret == KERN_SUCCESS, ret) << "vm_protect";
 #endif
+    }
 
     int state = VM_PURGABLE_NONVOLATILE;
     ret = vm_purgable_control(mach_task_self(),
@@ -112,6 +119,7 @@ class DiscardableMemoryMac
     MACH_CHECK(ret == KERN_SUCCESS, ret) << "vm_purgable_control";
     if (state & VM_PURGABLE_EMPTY)
       persistent = false;
+
     return persistent;
   }
 
diff --git a/base/memory/discardable_memory_malloc.cc b/base/memory/discardable_memory_malloc.cc
index be9d157..a72f911 100644
--- a/base/memory/discardable_memory_malloc.cc
+++ b/base/memory/discardable_memory_malloc.cc
@@ -16,7 +16,7 @@ DiscardableMemoryMalloc::~DiscardableMemoryMalloc() {
 }
 
 bool DiscardableMemoryMalloc::Initialize() {
-  return Lock() == DISCARDABLE_MEMORY_LOCK_STATUS_PURGED;
+  return Lock() != DISCARDABLE_MEMORY_LOCK_STATUS_FAILED;
 }
 
 DiscardableMemoryLockStatus DiscardableMemoryMalloc::Lock() {