From d4fc4a8cad0a8f94ea2a8bca7c76cebd8793395c Mon Sep 17 00:00:00 2001 From: mlippautz Date: Wed, 2 Dec 2015 03:43:32 -0800 Subject: [PATCH] Revert of [heap] Clean up stale store buffer entries for aborted pages. (patchset #4 id:60001 of https://codereview.chromium.org/1493653002/ ) Reason for revert: Not completely correct fix. Original issue's description: > [heap] Clean up stale store buffer entries for aborted pages. > > 1. Let X be the aborted slot (slot in an evacuated object in an aborted page) > 2. Assume X contains pointer to Y and Y is in the new space, so X is in the > store buffer. > 3. Store buffer rebuilding will not filter out X (it checks InNewSpace(Y)). > 4. The current mark-sweep finishes. The slot X is in free space and is also in > the store buffer. > 5. A string of length 9 "abcdefghi" is allocated in the new space. The string > looks like |MAP|LENGTH|hgfedcba|NNNNNNNi| in memory, where NNNNNNN is > previous garbage. Let's assume that NNNNNNN0 was pointing to a new space > object before. > 6. Scavenge happens. > 7. Slot X is still in free space and in store buffer. [It causes scavenge of > the object Y in > store_buffer()->IteratePointersToNewSpace(&Scavenger::ScavengeObject). But > it is not important]. > 8. Our string is promoted and is allocated over the slot X, such that NNNNNNNi > is written in X. > 9. The scavenge finishes. > 9. Another scavenge starts. > 10. We crash in > store_buffer()->IteratePointersToNewSpace(&Scavenger::ScavengeObject) when > processing slot X, because it doesn't point to valid map. > > BUG=chromium:524425,chromium:564498 > LOG=N > R=hpayer@chromium.org, ulan@chromium.org > > Committed: https://crrev.com/2e7eea4aef3403969fe885e30f892d46253b3572 > Cr-Commit-Position: refs/heads/master@{#32495} TBR=hpayer@chromium.org,ulan@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:524425,chromium:564498 Review URL: https://codereview.chromium.org/1489243004 Cr-Commit-Position: refs/heads/master@{#32504} --- src/heap/mark-compact.cc | 7 ------- src/heap/spaces.h | 3 +-- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc index d83b2cd92d..b7bcf9e800 100644 --- a/src/heap/mark-compact.cc +++ b/src/heap/mark-compact.cc @@ -683,8 +683,6 @@ void MarkCompactCollector::CollectEvacuationCandidates(PagedSpace* space) { PageIterator it(space); while (it.has_next()) { Page* p = it.next(); - // Invariant: No page should be marked as aborted after a GC. - DCHECK(!p->IsFlagSet(Page::COMPACTION_WAS_ABORTED)); if (p->NeverEvacuate()) continue; if (p->IsFlagSet(Page::POPULAR_PAGE)) { // This page had slots buffer overflow on previous GC, skip it. @@ -3280,13 +3278,8 @@ void MarkCompactCollector::EvacuatePagesInParallel() { // happens upon moving (which we potentially didn't do). // - Leave the page in the list of pages of a space since we could not // fully evacuate it. - // - Mark them for rescanning for store buffer entries as we otherwise - // might have stale store buffer entries that become "valid" again - // after reusing the memory. Note that all existing store buffer - // entries of such pages are filtered before rescanning. DCHECK(p->IsEvacuationCandidate()); p->SetFlag(Page::COMPACTION_WAS_ABORTED); - p->set_scan_on_scavenge(true); abandoned_pages++; break; case MemoryChunk::kCompactingFinalize: diff --git a/src/heap/spaces.h b/src/heap/spaces.h index 0e5fdef82d..5e47342602 100644 --- a/src/heap/spaces.h +++ b/src/heap/spaces.h @@ -328,8 +328,7 @@ class MemoryChunk { PRE_FREED, // |COMPACTION_WAS_ABORTED|: Indicates that the compaction in this page - // has been aborted and needs special handling, i.e., sweeper and store - // buffer. + // has been aborted and needs special handling by the sweeper. COMPACTION_WAS_ABORTED, // Last flag, keep at bottom.