[heap] Fix crash in promoted pages iteration

Iterating promoted pages uses a snapshot of the allocated pages to avoid
using locks (which locally resulted in regressions). Large pages may
have been freed between taking the snapshot and concurrent sweeping.
If that page is found by LookupChunkContainingAddress as the closest
page, we will try to access it and crash.

Fix by refresshing the snapshot after all pages have been freed.

Bug: v8:12612, chromium:1399331, chromium:1399328, chromium:1399330
Change-Id: I01a1dbcb9efde3a34a99d01260b0529dcf04c37a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4088363
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84740}
This commit is contained in:
Omer Katz 2022-12-08 15:06:53 +01:00 committed by V8 LUCI CQ
parent 54256360ff
commit e5dbd05dcf
3 changed files with 38 additions and 3 deletions

View File

@ -1593,7 +1593,9 @@ class Heap {
// Sweeping. =================================================================
// ===========================================================================
bool sweeping_in_progress() const { return sweeper_->sweeping_in_progress(); }
bool sweeping_in_progress() const {
return sweeper_ && sweeper_->sweeping_in_progress();
}
void FinishSweepingIfOutOfWork();

View File

@ -811,6 +811,8 @@ void MemoryAllocator::RecordNormalPageCreated(const Page& page) {
void MemoryAllocator::RecordNormalPageDestroyed(const Page& page) {
base::MutexGuard guard(&pages_mutex_);
DCHECK_IMPLIES(v8_flags.minor_mc && isolate_->heap()->sweeping_in_progress(),
isolate_->heap()->tracer()->IsInAtomicPause());
auto size = normal_pages_.erase(&page);
USE(size);
DCHECK_EQ(1u, size);
@ -825,6 +827,8 @@ void MemoryAllocator::RecordLargePageCreated(const LargePage& page) {
void MemoryAllocator::RecordLargePageDestroyed(const LargePage& page) {
base::MutexGuard guard(&pages_mutex_);
DCHECK_IMPLIES(v8_flags.minor_mc && isolate_->heap()->sweeping_in_progress(),
isolate_->heap()->tracer()->IsInAtomicPause());
auto size = large_pages_.erase(&page);
USE(size);
DCHECK_EQ(1u, size);

View File

@ -4,6 +4,7 @@
#include "src/heap/sweeper.h"
#include <algorithm>
#include <memory>
#include <vector>
@ -230,8 +231,8 @@ void Sweeper::StartSweeping(GarbageCollector collector) {
});
DCHECK(snapshot_normal_pages_set_.empty());
DCHECK(snapshot_large_pages_set_.empty());
if (v8_flags.minor_mc &&
(collector == GarbageCollector::MINOR_MARK_COMPACTOR)) {
if (v8_flags.minor_mc && (current_new_space_collector_ ==
GarbageCollector::MINOR_MARK_COMPACTOR)) {
std::tie(snapshot_normal_pages_set_, snapshot_large_pages_set_) =
heap_->memory_allocator()->SnapshotPageSets();
}
@ -246,6 +247,34 @@ int Sweeper::NumberOfConcurrentSweepers() const {
void Sweeper::StartSweeperTasks() {
DCHECK(current_new_space_collector_.has_value());
DCHECK(!job_handle_ || !job_handle_->IsValid());
if (v8_flags.minor_mc && (current_new_space_collector_ ==
GarbageCollector::MINOR_MARK_COMPACTOR)) {
// Some large pages may have been freed since starting sweeping.
#if DEBUG
MemoryAllocator::NormalPagesSet old_snapshot_normal_pages_set =
std::move(snapshot_normal_pages_set_);
MemoryAllocator::LargePagesSet old_snapshot_large_pages_set =
std::move(snapshot_large_pages_set_);
#endif // DEBUG
std::tie(snapshot_normal_pages_set_, snapshot_large_pages_set_) =
heap_->memory_allocator()->SnapshotPageSets();
#if DEBUG
// Normal pages may only have been added.
DCHECK(std::all_of(old_snapshot_normal_pages_set.begin(),
old_snapshot_normal_pages_set.end(),
[this](const Page* page) {
return snapshot_normal_pages_set_.find(page) !=
snapshot_normal_pages_set_.end();
}));
// Large pages may only have been removed.
DCHECK(std::all_of(snapshot_large_pages_set_.begin(),
snapshot_large_pages_set_.end(),
[old_snapshot_large_pages_set](const LargePage* page) {
return old_snapshot_large_pages_set.find(page) !=
old_snapshot_large_pages_set.end();
}));
#endif // DEBUG
}
if (v8_flags.concurrent_sweeping && sweeping_in_progress_ &&
!heap_->delay_sweeper_tasks_for_testing_) {
if (concurrent_sweepers_.empty()) {