[Mac] A couple of improvements for DiscardableMemory

base/memory/discardable_memory_mac.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // 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 <mach/mach.h>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
@@ skipping 29 matching lines @@
     : public DiscardableMemory,
       public internal::DiscardableMemoryManagerAllocation {
  public:
   explicit DiscardableMemoryMac(size_t bytes)
       : memory_(0, 0),
         bytes_(mach_vm_round_page(bytes)),
         is_locked_(false) {
     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_)
       Unlock();
     g_shared_state.Pointer()->manager.Unregister(this);
   }
 
   // Overridden from DiscardableMemory:
   virtual DiscardableMemoryLockStatus Lock() OVERRIDE {
     DCHECK(!is_locked_);
@@ skipping 13 matching lines @@
     is_locked_ = false;
   }
 
   virtual void* Memory() const OVERRIDE {
     DCHECK(is_locked_);
     return reinterpret_cast<void*>(memory_.address());
   }
 
   // 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(
           mach_task_self(),
           &address,
           bytes_,
           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(),
                               memory_.address(),
                               VM_PURGABLE_SET_STATE,
                               &state);
     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(),
                                             VM_PURGABLE_SET_STATE,
                                             &state);
     MACH_CHECK(ret == KERN_SUCCESS, ret) << "vm_purgable_control";
 
 #if !defined(NDEBUG)
     ret = vm_protect(mach_task_self(),
                      memory_.address(),
                      memory_.size(),
                      FALSE,
                      VM_PROT_NONE);
     MACH_DCHECK(ret == KERN_SUCCESS, ret) << "vm_protect";
 #endif
   }
 
   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();
   }
 
  private:
   mac::ScopedMachVM memory_;
   const size_t bytes_;
   bool is_locked_;
 
   DISALLOW_COPY_AND_ASSIGN(DiscardableMemoryMac);
 };
@@ skipping 58 matching lines @@
 }
 
 // static
 void DiscardableMemory::PurgeForTesting() {
   int state = 0;
   vm_purgable_control(mach_task_self(), 0, VM_PURGABLE_PURGE_ALL, &state);
   internal::DiscardableMemoryEmulated::PurgeForTesting();
 }
 
 }  // namespace base