[Mac] A couple of improvements for DiscardableMemory

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270135

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

https://codereview.chromium.org/285493004/diff/20001/base/memory/discardable_...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/285493004/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:50: bool Initialize() { return Lock() !=
DISCARDABLE_MEMORY_LOCK_STATUS_FAILED; }
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.

https://codereview.chromium.org/285493004/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:149: DCHECK(!is_locked_);
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]

https://codereview.chromium.org/285493004/diff/20001/base/memory/discardable_...
File base/memory/discardable_memory_mac.cc (right):

https://codereview.chromium.org/285493004/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:50: bool Initialize() { return Lock() !=
DISCARDABLE_MEMORY_LOCK_STATUS_FAILED; }
reveman wrote:
> 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.

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.

https://codereview.chromium.org/285493004/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_mac.cc:149: DCHECK(!is_locked_);
reveman wrote:
> 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.

This problem would apply to discardable_memory_ashmem.cc too, then.

> 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.

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_.

[Mark Mentovai, 2014-05-13 15:59:01]

OK, I’m just going to use this CL to make the incremental improvements we
discussed, and I’ll save a larger interface-implementation refactor for a
different bite-sized piece. Ready for another look.

[reveman, 2014-05-13 16:01:32]

lgtm

[Mark Mentovai, 2014-05-13 17:40:58]

Committed patchset #2 manually as r270135 (presubmit successful).