[Mac] A couple of improvements for DiscardableMemory

base/memory/discardable_memory_mac.cc

Index: base/memory/discardable_memory_mac.cc
diff --git a/base/memory/discardable_memory_mac.cc b/base/memory/discardable_memory_mac.cc
index d8c0ada7e2405c4c5adb7ce2f9bdcde2c6c2ce87..600bda7b4910c1a0bcd13407e97257749ae519d1 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; }

[reveman, 2014/05/12 20:57:44]

Can this ever return _SUCCESS? If you're interested in changing it just as a
cleanup then we should probably change discardable_memory_ashmem/emulated.cc too
for consistency.

[Mark Mentovai, 2014/05/12 21:35:49]

reveman wrote:
It shouldn’t ever return _SUCCESS, but I think that testing “did it
fail?” expresses the intent more than “has it been purged?”. For something
that’s being freshly allocated, there isn’t anything to purge, so testing
against _PURGED doesn’t express the intent as well. I will adjust the other
implementations too.

 
   virtual ~DiscardableMemoryMac() {
     if (is_locked_)
@@ -81,8 +81,10 @@ class DiscardableMemoryMac
 
   // Overridden from internal::DiscardableMemoryManagerAllocation:
   virtual bool AllocateAndAcquireLock() OVERRIDE {
-    bool persistent = true;
+    DCHECK(!is_locked_);
+
     kern_return_t ret;
+    bool persistent;
     if (!memory_.size()) {
       vm_address_t address = 0;
       ret = vm_allocate(
@@ -92,17 +94,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,10 +121,13 @@ class DiscardableMemoryMac
     MACH_CHECK(ret == KERN_SUCCESS, ret) << "vm_purgable_control";
     if (state & VM_PURGABLE_EMPTY)
       persistent = false;
+
     return persistent;
   }
 
   virtual void ReleaseLock() OVERRIDE {
+    DCHECK(is_locked_);
+
     int state = VM_PURGABLE_VOLATILE | VM_VOLATILE_GROUP_DEFAULT;
     kern_return_t ret = vm_purgable_control(mach_task_self(),
                                             memory_.address(),
@@ -134,6 +146,7 @@ class DiscardableMemoryMac
   }
 
   virtual void Purge() OVERRIDE {
+    DCHECK(!is_locked_);

[reveman, 2014/05/12 20:57:44]

There's a race here as this can be called on any thread by the
DiscardableMemoryManager instance. ie. Unlock() might not have had a chance to
set is_locked_ to false yet.

The current idea with these is_locked_ DCHECKs is to protect against the client
misbehaving and not the internal manager instance. Maybe use a different
variable (lock_acquired_?) if we want to also check that the manager is behaving
properly.

[Mark Mentovai, 2014/05/12 21:35:49]

reveman wrote:
This problem would apply to discardable_memory_ashmem.cc too, then.
If that’s the case, then is_locked_ is more broken than just this DCHECK. If
Purge() can actually be called while is_locked_ is true, then even without this
DCHECK, is_locked_ will stay true. is_locked_ implies not purged and Purge()
implies purged (and not locked). The next time someone calls either Lock() or
Unlock(), things will be out of place. Unlock() won’t trip a DCHECK on
is_locked_ but will call vm_purgable_control on the zero page, which will fail
(its protection is _NONE and setting the purgeable state requires _WRITE) and
trip the MACH_CHECK. Lock() will trip the DCHECK on is_locked_.

     memory_.reset();
   }