Revert "Revert 239763 "Add DiscardableMemoryAllocator to minimize use of...""

Revert "Revert 239763 "Add DiscardableMemoryAllocator to minimize use of...""

The previous attempt failed due to ASAN complaining during the
DiscardableMemory.TooLargeAllocationFails test (which is quite expected).

TBR=willchan
BUG=299828
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239816

[Philippe, 2013-12-10 15:18:38]

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

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:16: // TODO(pliard): enable this on
all platforms. This currently fails when
FYI, I'm not proud of doing this but I want to get the allocator in :) I wish
there was a compile time flag letting us know that the file is being built with
ASAN on but there isn't. It wasn't clear to me either how to disable this test
with ASAN. There was some TSAN, LSAN... suppression files but I didn't see
anything for ASAN. I believe David added his own test anyway to check this in
the provider unit tests. The test doesn't cover the code path where malloc()
returns NULL though (but the production code does correctly check the return
value of malloc()) which is why it didn't fail with ASAN.

[reveman, 2013-12-10 15:56:31]

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

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:16: // TODO(pliard): enable this on
all platforms. This currently fails when
On 2013/12/10 15:18:38, Philippe wrote:
> FYI, I'm not proud of doing this but I want to get the allocator in :) I wish
> there was a compile time flag letting us know that the file is being built
with
> ASAN on but there isn't. It wasn't clear to me either how to disable this test
> with ASAN. There was some TSAN, LSAN... suppression files but I didn't see
> anything for ASAN. I believe David added his own test anyway to check this in
> the provider unit tests. The test doesn't cover the code path where malloc()
> returns NULL though (but the production code does correctly check the return
> value of malloc()) which is why it didn't fail with ASAN.

I don't think we should have a test that pass a huge size to malloc and expect
it to fail. Can this be an allocator unit test instead of a general DM test?

[Philippe, 2013-12-10 16:00:28]

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

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:16: // TODO(pliard): enable this on
all platforms. This currently fails when
On 2013/12/10 15:56:32, David Reveman wrote:
> On 2013/12/10 15:18:38, Philippe wrote:
> > FYI, I'm not proud of doing this but I want to get the allocator in :) I
wish
> > there was a compile time flag letting us know that the file is being built
> with
> > ASAN on but there isn't. It wasn't clear to me either how to disable this
test
> > with ASAN. There was some TSAN, LSAN... suppression files but I didn't see
> > anything for ASAN. I believe David added his own test anyway to check this
in
> > the provider unit tests. The test doesn't cover the code path where malloc()
> > returns NULL though (but the production code does correctly check the return
> > value of malloc()) which is why it didn't fail with ASAN.
> 
> I don't think we should have a test that pass a huge size to malloc and expect
> it to fail. Can this be an allocator unit test instead of a general DM test?

Yeah, agreed. I need to keep it here ideally on Android though since the code
dealing with this is not in the allocator. The allocator does indirectly check
that it's safe to do page alignment though. I can remove the TODO/comment and
just keep the ANDROID ifdef. How would you feel about that?

[Philippe, 2013-12-10 16:15:07]

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

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:16: // TODO(pliard): enable this on
all platforms. This currently fails when
On 2013/12/10 16:00:28, Philippe wrote:
> On 2013/12/10 15:56:32, David Reveman wrote:
> > On 2013/12/10 15:18:38, Philippe wrote:
> > > FYI, I'm not proud of doing this but I want to get the allocator in :) I
> wish
> > > there was a compile time flag letting us know that the file is being built
> > with
> > > ASAN on but there isn't. It wasn't clear to me either how to disable this
> test
> > > with ASAN. There was some TSAN, LSAN... suppression files but I didn't see
> > > anything for ASAN. I believe David added his own test anyway to check this
> in
> > > the provider unit tests. The test doesn't cover the code path where
malloc()
> > > returns NULL though (but the production code does correctly check the
return
> > > value of malloc()) which is why it didn't fail with ASAN.
> > 
> > I don't think we should have a test that pass a huge size to malloc and
expect
> > it to fail. Can this be an allocator unit test instead of a general DM test?
> 
> Yeah, agreed. I need to keep it here ideally on Android though since the code
> dealing with this is not in the allocator. The allocator does indirectly check
> that it's safe to do page alignment though. I can remove the TODO/comment and
> just keep the ANDROID ifdef. How would you feel about that?

I would be ok with moving it to the allocator unit tests if you feel strongly
though (which would not be the right place IMO since this test doesn't exerce
the allocator code).

[reveman, 2013-12-10 16:25:45]

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

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:7: #include <limits>
#if defined(OS_ANDROID) to only include what you need

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:16: // TODO(pliard): enable this on
all platforms. This currently fails when
On 2013/12/10 16:00:28, Philippe wrote:
> On 2013/12/10 15:56:32, David Reveman wrote:
> > On 2013/12/10 15:18:38, Philippe wrote:
> > > FYI, I'm not proud of doing this but I want to get the allocator in :) I
> wish
> > > there was a compile time flag letting us know that the file is being built
> > with
> > > ASAN on but there isn't. It wasn't clear to me either how to disable this
> test
> > > with ASAN. There was some TSAN, LSAN... suppression files but I didn't see
> > > anything for ASAN. I believe David added his own test anyway to check this
> in
> > > the provider unit tests. The test doesn't cover the code path where
malloc()
> > > returns NULL though (but the production code does correctly check the
return
> > > value of malloc()) which is why it didn't fail with ASAN.
> > 
> > I don't think we should have a test that pass a huge size to malloc and
expect
> > it to fail. Can this be an allocator unit test instead of a general DM test?
> 
> Yeah, agreed. I need to keep it here ideally on Android though since the code
> dealing with this is not in the allocator. The allocator does indirectly check
> that it's safe to do page alignment though. I can remove the TODO/comment and
> just keep the ANDROID ifdef. How would you feel about that?

I'm fine with an ANDROID ifdef for now.

I think the the DiscardableMemory framework is in need of a general refactor to
be more modular and less ifdef-y. I'm looking at this as part of using ashmem on
ChromeOS.

[Philippe, 2013-12-10 16:38:43]

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

https://codereview.chromium.org/101953003/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_unittest.cc:16: // TODO(pliard): enable this on
all platforms. This currently fails when
On 2013/12/10 16:25:46, David Reveman wrote:
> On 2013/12/10 16:00:28, Philippe wrote:
> > On 2013/12/10 15:56:32, David Reveman wrote:
> > > On 2013/12/10 15:18:38, Philippe wrote:
> > > > FYI, I'm not proud of doing this but I want to get the allocator in :) I
> > wish
> > > > there was a compile time flag letting us know that the file is being
built
> > > with
> > > > ASAN on but there isn't. It wasn't clear to me either how to disable
this
> > test
> > > > with ASAN. There was some TSAN, LSAN... suppression files but I didn't
see
> > > > anything for ASAN. I believe David added his own test anyway to check
this
> > in
> > > > the provider unit tests. The test doesn't cover the code path where
> malloc()
> > > > returns NULL though (but the production code does correctly check the
> return
> > > > value of malloc()) which is why it didn't fail with ASAN.
> > > 
> > > I don't think we should have a test that pass a huge size to malloc and
> expect
> > > it to fail. Can this be an allocator unit test instead of a general DM
test?
> > 
> > Yeah, agreed. I need to keep it here ideally on Android though since the
code
> > dealing with this is not in the allocator. The allocator does indirectly
check
> > that it's safe to do page alignment though. I can remove the TODO/comment
and
> > just keep the ANDROID ifdef. How would you feel about that?
> 
> I'm fine with an ANDROID ifdef for now.
> 
> I think the the DiscardableMemory framework is in need of a general refactor
to
> be more modular and less ifdef-y. I'm looking at this as part of using ashmem
on
> ChromeOS.

Done, thanks.

[commit-bot: I haz the power, 2013-12-10 16:38:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/101953003/40001

[commit-bot: I haz the power, 2013-12-10 19:04:22]

Change committed as 239816