PartitionAlloc: Leave bucket in valid state when allocation fails

PartitionAlloc: Leave bucket in valid state when allocation fails

When an allocation with the PartitionAllocReturnNull flag fails, we need to
make sure to leave all structures in a valid state when bailing out, since
the process lives on and may call again later.

Specifically, make sure that the bucket's activePagesHead has a valid value,
instead of null, when returning null from partitionAllocSlowPath(). Since
there's no meaningful page to set as the active pages head, set it to its
initial value, &PartitionRootBase::gSeedPage.

This was already necessarily supported by the allocation code paths. The
deallocation code path needs some minor adjustments to handle this state
correctly.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184983

[Jens Widell, 2014-10-22 19:34:20]

jl@opera.com changed reviewers:
+ cevans@google.com, jochen@chromium.org

[Jens Widell, 2014-10-22 19:34:22]

cevans: PTAL
jochen: FYI

This also affects the retry logic https://codereview.chromium.org/657113006/.
Fixing this bug first might make the retry logic more straight-forward.

[Jens Widell, 2014-10-22 19:40:12]

https://codereview.chromium.org/645223007/diff/40001/Source/wtf/PartitionAllo...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/645223007/diff/40001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAllocTest.cpp:1102: // blocks. This may never fail on 64-bit
systems, in which case this test fails
To make these allocations fail on 64-bit, I've been using "ulimit -v 2000000" to
set a 2 GB address space limit before running wtf_unittests. Is there anything
in the testing framework/infrastructure that could be used to the same effect to
ensure this test is meaningful?

Oh, if only we hadn't just dropped PartitionAlloc's 2 GB address space limit...
:-)

[jochen (gone - plz use gerrit), 2014-10-22 20:42:52]

thx for working on this

[jochen (gone - plz use gerrit), 2014-10-23 09:31:18]

btw, you could just programatically lower the ulimit at least on platforms where
this is supported (and disable the test for 64bit otherwise)

[Jens Widell, 2014-10-23 11:00:49]

On 2014/10/23 09:31:18, jochen wrote:
> btw, you could just programatically lower the ulimit at least on platforms
where
> this is supported (and disable the test for 64bit otherwise)

I've added code that sets the limit with setrlimit() if OS(POSIX).

Disabling the test would only be necessary if I modified it to require that an
allocation actually failed. But that would of course be meaningful to require,
so that we have a way of detecting whether the test is actually successfully
testing anything.

[Jens Widell, 2014-10-23 12:48:48]

Test is now updated to:

* Not run at all on 64-bit non-POSIX systems.

* Limit the address space to ensure allocation failure on 64-bit POSIX systems.
(Not needed on 32-bit systems.)

* Verify that the test actually did fail to allocate at some point, and thus
actually tested the issue.

For verification that the test actually reproduces the crash, I uploaded
https://codereview.chromium.org/669083003/ which is the same as this but with
the actual fix commented out, and ran some try-bots on it.

[Jens Widell, 2014-10-29 09:51:19]

cevans@: ping?

[Chris Evans, 2014-11-05 23:55:22]

On 2014/10/29 09:51:19, Jens Widell wrote:
> cevans@: ping?

Firstly, sorry for the slow reply! I was working but in a country with filtered
internet access so basically offline. And then I was on vacation. Now I am back
:)

Secondly, good find.

In terms of review: can I request a tweak to the approach? It feels to me like a
failed allocation should be a no-op and not change state of anything. That would
be ideal. However, the fact that the current proposed patch changes code in the
free() path suggests that a failed allocation must change state in some way? Is
it possible to craft the patch in a way that only code in the malloc() path is
changed? I think that would be the correct approach if it is possible.

[Jens Widell, 2014-11-06 10:55:45]

On 2014/11/05 23:55:22, Chris Evans wrote:
> In terms of review: can I request a tweak to the approach? It feels to me like
a
> failed allocation should be a no-op and not change state of anything. That
would
> be ideal. However, the fact that the current proposed patch changes code in
the
> free() path suggests that a failed allocation must change state in some way?
Is
> it possible to craft the patch in a way that only code in the malloc() path is
> changed? I think that would be the correct approach if it is possible.

While I agree that it's a bit unfortunate that a failed allocation affects the
free() code path, I have so far not really been able to come up with an
alternative solution that I'm happy with. They tend to end up more complicated
than this one, and/or still affecting the free() code path.

[Chris Evans, 2014-11-07 03:33:23]

cevans@chromium.org changed reviewers:
+ cevans@chromium.org

[Chris Evans, 2014-11-07 03:33:23]

LGTM with a bunch of nits.

Thanks for persevering with alternate approaches, even though we didn't find
any. I now see that partitionSetNewActivePage() is fundamentally state changing
at that's ok.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:702: if (returnNull) {
Nit: maybe add comment? Something like "If we get here, we will have set the
active page to NULL. If we failed to allocate a new active page, we'll need to
set it back to the seed page"

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:805: if (bucket->activePagesHead ==
&PartitionRootGeneric::gSeedPage)
Nit: add UNLIKELY!

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1153: void* ptrs[8192];
Nit: shouldn't this be 6144 ?

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1156: for (i = 0; i < 6144; ++i) {
Nit: Can we make 6144 calculated to make it clearer? Maybe have 512KB be a
constant and then
size_t numAllocs = (6 * 1024 * 1024 * 1024) / theConstant512KB

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1157: ptrs[i] =
partitionAllocGenericFlags(genericAllocator.root(),
WTF::PartitionAllocReturnNull, 512*1024);
Nit: for style consistency, add spaces? "512 * 1024"

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1159: ptrs[i] =
partitionAllocGenericFlags(genericAllocator.root(),
WTF::PartitionAllocReturnNull, 512*1024);
Same nit.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1171: ptrs[i] =
partitionAllocGenericFlags(genericAllocator.root(),
WTF::PartitionAllocReturnNull, 512*1024);
Nit: Why do we alloc again here? Thoroughness of testing? Maybe add comment?

And maybe a non-nit: should we EXPECT() that the allocation succeeded? If it
fails, we won't notice because partitionFreeGeneric(NULL) is ok.

[Jens Widell, 2014-11-07 14:17:32]

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:702: if (returnNull) {
On 2014/11/07 03:33:23, Chris Evans wrote:
> Nit: maybe add comment? Something like "If we get here, we will have set the
> active page to NULL. If we failed to allocate a new active page, we'll need to
> set it back to the seed page"

Done.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAlloc.cpp:805: if (bucket->activePagesHead ==
&PartitionRootGeneric::gSeedPage)
On 2014/11/07 03:33:23, Chris Evans wrote:
> Nit: add UNLIKELY!

Done.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1153: void* ptrs[8192];
On 2014/11/07 03:33:23, Chris Evans wrote:
> Nit: shouldn't this be 6144 ?

Done.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1156: for (i = 0; i < 6144; ++i) {
On 2014/11/07 03:33:23, Chris Evans wrote:
> Nit: Can we make 6144 calculated to make it clearer? Maybe have 512KB be a
> constant and then
> size_t numAllocs = (6 * 1024 * 1024 * 1024) / theConstant512KB

Done. Made constants out all of "6 GB", "512 kB" and "6144".

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1157: ptrs[i] =
partitionAllocGenericFlags(genericAllocator.root(),
WTF::PartitionAllocReturnNull, 512*1024);
On 2014/11/07 03:33:23, Chris Evans wrote:
> Nit: for style consistency, add spaces? "512 * 1024"

That's weird... I never write code like that normally, and complain when other
people do. Sorry.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1159: ptrs[i] =
partitionAllocGenericFlags(genericAllocator.root(),
WTF::PartitionAllocReturnNull, 512*1024);
On 2014/11/07 03:33:23, Chris Evans wrote:
> Same nit.

Done.

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1171: ptrs[i] =
partitionAllocGenericFlags(genericAllocator.root(),
WTF::PartitionAllocReturnNull, 512*1024);
On 2014/11/07 03:33:23, Chris Evans wrote:
> Nit: Why do we alloc again here? Thoroughness of testing? Maybe add comment?

Thoroughness. Since I fiddled with the free() code path, it seemed meaningful to
check that allocated memory could be freed, and that freed memory could be
allocated again.

> And maybe a non-nit: should we EXPECT() that the allocation succeeded? If it
> fails, we won't notice because partitionFreeGeneric(NULL) is ok.

Yes, that seems like a good idea. I see no reason why any of these allocations
could reasonably fail.

[Jens Widell, 2014-11-07 14:29:51]

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:1153: void* ptrs[8192];
On 2014/11/07 14:17:31, Jens Widell wrote:
> On 2014/11/07 03:33:23, Chris Evans wrote:
> > Nit: shouldn't this be 6144 ?
> 
> Done.

Actually, since this is now calculated as 6GB / 512kB, it's rather 12288.

6144 amounts to allocating 3 GB of actual memory, which is less than the 4 GB
address space limit we set (on 64-bit systems). But since we can only fit 3 x
512 kB into a super page, we ended up needing to map 4 GB of address space,
which, since we inevitably also have some other memory mapped, was still too
much.

And on 32-bit systems even mapping 3 GB of address would likely have failed, of
course.

Anyhow, the test checks that it doesn't succeed in allocating all it tries to
allocate, so all this was mostly academic.

[Chris Evans, 2014-11-07 17:13:51]

On 2014/11/07 14:29:51, Jens Widell wrote:
>
https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
> File Source/wtf/PartitionAllocTest.cpp (right):
> 
>
https://codereview.chromium.org/645223007/diff/120001/Source/wtf/PartitionAll...
> Source/wtf/PartitionAllocTest.cpp:1153: void* ptrs[8192];
> On 2014/11/07 14:17:31, Jens Widell wrote:
> > On 2014/11/07 03:33:23, Chris Evans wrote:
> > > Nit: shouldn't this be 6144 ?
> > 
> > Done.
> 
> Actually, since this is now calculated as 6GB / 512kB, it's rather 12288.
> 
> 6144 amounts to allocating 3 GB of actual memory, which is less than the 4 GB
> address space limit we set (on 64-bit systems). But since we can only fit 3 x
> 512 kB into a super page, we ended up needing to map 4 GB of address space,
> which, since we inevitably also have some other memory mapped, was still too
> much.
> 
> And on 32-bit systems even mapping 3 GB of address would likely have failed,
of
> course.
> 
> Anyhow, the test checks that it doesn't succeed in allocating all it tries to
> allocate, so all this was mostly academic.

LGTM++

[Jens Widell, 2014-11-07 17:14:57]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-07 17:15:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645223007/160001

[commit-bot: I haz the power, 2014-11-07 18:20:26]

Committed patchset #9 (id:160001) as 184983

[Nico, 2015-08-08 19:46:49]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-08-08 19:46:50]

https://codereview.chromium.org/645223007/diff/160001/Source/wtf/PartitionAll...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/645223007/diff/160001/Source/wtf/PartitionAll...
Source/wtf/PartitionAllocTest.cpp:108: return false;
cevans: I found this pretty confusing – DoReturnNullTest() is only defined if
!CPU(64BIT) || OS(POSIX), but then SetAddressSpaceLimit always returns false on
OS X, and as a consequence RepeatedReturnNull is then disabled on OS X. Are
there plans for getting this passing on OS X? If not, should

#if !CPU(64BIT) || OS(POSIX)

just be changed to

#if !CPU(64BIT) || (OS(POSIX) && !OS(MACOSX))

above DoReturnNullTest()?