Die on an OOM situation in many more cases....

Die on an OOM situation in many more cases.

BUG=http://crbug.com/12673
TEST=run out of memory and die


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41834

[Avi (use Gerrit), 2010-03-12 20:28:45]

It feels like I'm planting landmines in the ground as if they were flowers, each
with gentle care.

[Mark Mentovai, 2010-03-12 20:42:19]

http://codereview.chromium.org/875004/diff/1/2
File base/process_util_mac.mm (right):

http://codereview.chromium.org/875004/diff/1/2#newcode14
base/process_util_mac.mm:14: #include <new>
Sort C++ system headers together, after C system headers - move this one down
with <string>.

http://codereview.chromium.org/875004/diff/1/2#newcode421
base/process_util_mac.mm:421: struct CFAllocator {
I would use a different name to make it clear that this is our own definition of
CFAllocator, like ChromeCFAllocator.

I'd also add a typedef for ChromeCFAllocatorRef so that the code below has "Ref"
in the type names as is expected for CF.

http://codereview.chromium.org/875004/diff/1/2#newcode539
base/process_util_mac.mm:539: // all allocation done by system-provided ones.
Hmm.  kCFAllocatorSystemDefault instead of kCFAllocatorDefault?  Why not both? 
(I seem to recall that they're synonyms in CF, although the "default" can be
replaced with CFAllocatorSetDefault.  Have you considered calling
CFAllocatorSetDefault?)

http://codereview.chromium.org/875004/diff/1/2#newcode573
base/process_util_mac.mm:573: if (m1 && m2)
Don'tcha wanna CHECK that you've got m1 && m2?

[Mark Mentovai, 2010-03-12 20:42:56]

It's overall LGTM, but the CF bits are the scariest things here.  I want to make
sure we take a considered approach, hence my question.

[Avi (use Gerrit), 2010-03-12 20:57:21]

Understood. Unfortunately I can't think of any good way at runtime to tell that
the definition of CFAllocator is what we think it is.

http://codereview.chromium.org/875004/diff/1/2
File base/process_util_mac.mm (right):

http://codereview.chromium.org/875004/diff/1/2#newcode14
base/process_util_mac.mm:14: #include <new>
On 2010/03/12 20:42:19, Mark Mentovai wrote:
> Sort C++ system headers together, after C system headers - move this one down
> with <string>.

Done.

http://codereview.chromium.org/875004/diff/1/2#newcode421
base/process_util_mac.mm:421: struct CFAllocator {
On 2010/03/12 20:42:19, Mark Mentovai wrote:
> I would use a different name to make it clear that this is our own definition
of
> CFAllocator, like ChromeCFAllocator.
> 
> I'd also add a typedef for ChromeCFAllocatorRef so that the code below has
"Ref"
> in the type names as is expected for CF.

Done.

http://codereview.chromium.org/875004/diff/1/2#newcode539
base/process_util_mac.mm:539: // all allocation done by system-provided ones.
On 2010/03/12 20:42:19, Mark Mentovai wrote:
> Hmm.  kCFAllocatorSystemDefault instead of kCFAllocatorDefault?  Why not both?


The implementation of kCFAllocatorDefault is:

- was CFAllocatorSetDefault set?
-- if so return it
- return kCFAllocatorSystemDefault

I've intercepted on all the Apple-provided allocators, so if someone calls
SetDefault on them, we're OK. If someone creates a new allocator and calls
SetDefault then we're stuck, but it's not the default part that we get stuck on,
but the new allocator part.

http://codereview.chromium.org/875004/diff/1/2#newcode573
base/process_util_mac.mm:573: if (m1 && m2)
On 2010/03/12 20:42:19, Mark Mentovai wrote:
> Don'tcha wanna CHECK that you've got m1 && m2?

Done.

[Mark Mentovai, 2010-03-12 21:02:36]

OK.  One last question before I invoke jrg: have you tried this out on both 10.5
and 10.6, with things that try to overallocate to blow memory?  (You can putz
around with ulimit to restrict memory.)

Now, to invoke jrg:

This reallyreally needs some sort of unit test, which should try to exhaust
memory using each of the allocators we're able to intercept, and should verify
DEATH in each case.

[Avi (use Gerrit), 2010-03-15 23:29:38]

On 2010/03/15 23:12:23, Avi wrote:
> 

Works on 10.5 and 10.6.

[Mark Mentovai, 2010-03-16 00:57:50]

http://codereview.chromium.org/875004/diff/21001/22003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/875004/diff/21001/22003#newcode527
base/process_util_unittest.cc:527:
test_size_(std::numeric_limits<std::size_t>::max() - 12288),
I'd say 12 * 1024.

What's magic about 12k that 8k wouldn't cut?

http://codereview.chromium.org/875004/diff/21001/22003#newcode550
base/process_util_unittest.cc:550: ASSERT_DEATH(value_ = new char[test_size_],
"");
This tests new[], which may be different from the (non-array) new.  We should
test both of them.

http://codereview.chromium.org/875004/diff/21001/22003#newcode598
base/process_util_unittest.cc:598: // We call these allocation functions twice
since they can only allocate 2G and
Is this true under 64-bit mode?

signed_test_size_ won't be 2G in 64-bit mode.

http://codereview.chromium.org/875004/diff/21001/22005
File base/process_util_unittest_mac.mm (right):

http://codereview.chromium.org/875004/diff/21001/22005#newcode11
base/process_util_unittest_mac.mm:11: // Objective C objects must be < 2G in
size.
Is this true in 64-bit mode too?  Adjust the comment as needed.

http://codereview.chromium.org/875004/diff/21001/22005#newcode12
base/process_util_unittest_mac.mm:12: int justunder2gigs[536870911];
This should be:

  int justUnder2Gigs_[(2U * 1024 * 1024 * 1024 - 1) / sizeof(int)];

You could get even closer to 2GB if you use char at both ends of the line.

[Avi (use Gerrit), 2010-03-16 14:26:20]

http://codereview.chromium.org/875004/diff/21001/22003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/875004/diff/21001/22003#newcode527
base/process_util_unittest.cc:527:
test_size_(std::numeric_limits<std::size_t>::max() - 12288),
On 2010/03/16 00:57:50, Mark Mentovai wrote:
> I'd say 12 * 1024.
> 
> What's magic about 12k that 8k wouldn't cut?

8k is the magic cutoff where our malloc patch can't catch a nil result. We have
to allocate just a bit smaller so I bumped it down a little.

http://codereview.chromium.org/875004/diff/21001/22003#newcode550
base/process_util_unittest.cc:550: ASSERT_DEATH(value_ = new char[test_size_],
"");
On 2010/03/16 00:57:50, Mark Mentovai wrote:
> This tests new[], which may be different from the (non-array) new.  We should
> test both of them.

Good point.

[Mark Mentovai, 2010-03-16 20:17:08]

LGTM