[PartitionAlloc] Properly return nullptr rather than crashing. [chromium/src : main]
Benoit L (Gerrit)
Attention is currently required from: Bartek Nowierski.
Benoit L would like Bartek Nowierski to review this change.
[PartitionAlloc] Properly return nullptr rather than crashing.
By default, PartitionAlloc doesn't return nullptr, it crashes. This is
the common case in Chromium, and the desired behavior in most of the
codebase. Nevertheless, to properly support base::UncheckedMalloc() and
nothrow C++ operators, the PartitionAllocReturnNull flag exists.
However, until this commit, it is only used for direct-mapped
allocations. While these are the most likely to trigger OOM, and also
the most likely to be called from base::UncheckedMalloc(), it is not
correct to not handle the other ones.
This commit changes that. It also isolates the two causes of OOM:
address space and commit failure into their own functions, to
disambiguate in crash reports.
Bug: 1202100
Change-Id: Id489cf3e9fc43510e1788786929e41c3e2c91b6b
---
M base/allocator/partition_allocator/partition_bucket.cc
M base/allocator/partition_allocator/partition_bucket.h
M base/allocator/partition_allocator/partition_root.cc
M base/allocator/partition_allocator/partition_root.h
4 files changed, 94 insertions(+), 68 deletions(-)
To view, visit change 2853549. To unsubscribe, or for help writing mail filters, visit settings.
Benoit L (Gerrit)
Attention is currently required from: Bartek Nowierski.
Bartek Nowierski (Gerrit)
Attention is currently required from: Benoit L.
13 comments:
Patchset:
I really like the direction it's going to, but I'm afraid I found an issue.
File base/allocator/partition_allocator/partition_bucket.cc:
// The lock is here to protect PA from:
// 1. Concurrent calls
// 2. Reentrant calls
//
// This is fine here however, as:I know it's copy+paste, but this reads weird. How about, start the first paragraph with:
The caller held the lock to protect PA form:and then:
This is fine to unlock here, however, as:, so the lock
// will not be re-acquired, which would lead to acting on inconsistent data
// that could have been modified in-between releasing and acquiring itThis also reads weird. How about end the sentence after "never returns" or replace with
thus no risk of touching any protected data."would" sounds better
Patch Set #3, Line 109:
raw_sizereserved_size for similar reason you did it below. It'll become more relevant if we implement your idea for aligned allocation by padding with extra guard pages.
Patch Set #3, Line 140:
raw_sizeslot_size maybe?
Patch Set #3, Line 327:
return nullptrI know I asked you to do this, but just realized that this has a problem, because next request that falls into this slot span will crash due to uncommitted memory. PCScan will also try to walk through it without consideration.
It may be too complex to solve in this CL. I think it's ok to put a TODO for now.
I have a CL in the works that brings lazy commit to all platform, without incurring the cost we incurred in the past. I de-prioritized it, but if I get back to it, it'll automatically fix the problem, as this block will go away.
Patch Set #3, Line 329:
kSuperPageSizeslot_span_committed_size
Accounting like num_allocated_slots and num_unprovisioned_slots should be moved below this if
Patch Set #3, Line 743:
return nullptrI'm afraid this suffers from the same problem as above :(
Patch Set #3, Line 772:
further down"Further down" doesn't make quite sense. How about simply say:
We would have triggered an OOM crash by now if PartitionAllocReturnNull flag isn't used.Patch Set #3, Line 773:
PA_CHECKI think PA_DCHECK is more appropriate. If you stick with PA_CHECK, then |if (flags & PartitionAllocReturnNull)| isn't necessary.
File base/allocator/partition_allocator/partition_root.cc:
Patch Set #3, Line 691:
PA_CHECK(ok)Don't we wanna go through PartitionOutOfMemoryCommitFailure here and above?
To view, visit change 2853549. To unsubscribe, or for help writing mail filters, visit settings.
Bartek Nowierski (Gerrit)
Attention is currently required from: Benoit L.
1 comment:
Patchset:
I'm looking forward to splitting address space exhaustion and commit failure OOM crashes. This will improve diagnosability.
To view, visit change 2853549. To unsubscribe, or for help writing mail filters, visit settings.
Mike Frysinger (Gerrit)
Mike Frysinger abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |