From b8dbeb621fbc2ac3311a580da0514cd84fab3d46 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Tue, 16 Feb 2021 11:59:16 +0000 Subject: [PATCH] Bug 1692221 - Update list of free committed arenas in TenuredChunk::decommitFreeArenasWithoutUnlocking r=sfink This adds methods to verify the chunk metadata very GC in debug builds (held relocated arenas mess things up because they're mprotected so it's checked after we free those). The code in question was hard to exercise and I had to update reportLargeAllocationFailure to take a byte count so we could avoid calling the shell's large allocation failure callback, which causes the shell to exit (see JSRuntime::onOutOfMemoryCanGC). It's debateable whether it's worth handling OOM from MarkPagesUnusedSoft. If we assume pages have been been successfully marked unused regardless of failure I think the worst that happens is that we don't reuse them immediately... I ended up handling this anyway though, and rebuilding the list rather than clearing it entirely in decommitFreeArenasWithoutUnlocking. Differential Revision: https://phabricator.services.mozilla.com/D104837 --- js/public/HeapAPI.h | 2 +- js/src/builtin/TestingFunctions.cpp | 19 +++++-- js/src/gc/Allocator.cpp | 4 ++ js/src/gc/GC.cpp | 70 +++++++++++++++++++++++-- js/src/gc/GCRuntime.h | 4 ++ js/src/gc/Heap.h | 6 +++ js/src/jit-test/tests/gc/bug-1692221.js | 39 ++++++++++++++ 7 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 js/src/jit-test/tests/gc/bug-1692221.js diff --git a/js/public/HeapAPI.h b/js/public/HeapAPI.h index c208aacbcb0d..9ffc1908cfec 100644 --- a/js/public/HeapAPI.h +++ b/js/public/HeapAPI.h @@ -98,7 +98,7 @@ struct TenuredChunkInfo { TenuredChunk* prev = nullptr; public: - /* Free arenas are linked together with arena.next. */ + /* List of free committed arenas, linked together with arena.next. */ Arena* freeArenasHead; /* diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp index ac7fda963cb5..73c1889eea2f 100644 --- a/js/src/builtin/TestingFunctions.cpp +++ b/js/src/builtin/TestingFunctions.cpp @@ -4212,8 +4212,21 @@ static bool ThrowOutOfMemory(JSContext* cx, unsigned argc, Value* vp) { static bool ReportLargeAllocationFailure(JSContext* cx, unsigned argc, Value* vp) { CallArgs args = CallArgsFromVp(argc, vp); - void* buf = cx->runtime()->onOutOfMemoryCanGC( - AllocFunction::Malloc, js::MallocArena, JSRuntime::LARGE_ALLOCATION); + + size_t bytes = JSRuntime::LARGE_ALLOCATION; + if (args.length() >= 1) { + if (!args[0].isInt32()) { + RootedObject callee(cx, &args.callee()); + ReportUsageErrorASCII(cx, callee, + "First argument must be an integer if specified."); + return false; + } + bytes = args[0].toInt32(); + } + + void* buf = cx->runtime()->onOutOfMemoryCanGC(AllocFunction::Malloc, + js::MallocArena, bytes); + js_free(buf); args.rval().setUndefined(); return true; @@ -6993,7 +7006,7 @@ gc::ZealModeHelpText), " Throw out of memory exception, for OOM handling testing."), JS_FN_HELP("reportLargeAllocationFailure", ReportLargeAllocationFailure, 0, 0, -"reportLargeAllocationFailure()", +"reportLargeAllocationFailure([bytes])", " Call the large allocation failure callback, as though a large malloc call failed,\n" " then return undefined. In Gecko, this sends a memory pressure notification, which\n" " can free up some memory."), diff --git a/js/src/gc/Allocator.cpp b/js/src/gc/Allocator.cpp index a5fd34b3f06d..3dfbd1f66324 100644 --- a/js/src/gc/Allocator.cpp +++ b/js/src/gc/Allocator.cpp @@ -872,6 +872,10 @@ void TenuredChunk::init(GCRuntime* gc) { */ decommitAllArenas(); +#ifdef DEBUG + verify(); +#endif + /* The rest of info fields are initialized in pickChunk. */ } diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp index 49950cbe38c8..707cd40b3aae 100644 --- a/js/src/gc/GC.cpp +++ b/js/src/gc/GC.cpp @@ -723,6 +723,7 @@ bool ChunkPool::isSorted() const { } #ifdef DEBUG + bool ChunkPool::contains(TenuredChunk* chunk) const { verify(); for (TenuredChunk* cursor = head_; cursor; cursor = cursor->info.next) { @@ -744,6 +745,48 @@ bool ChunkPool::verify() const { MOZ_ASSERT(count_ == count); return true; } + +void GCRuntime::verifyAllChunks() { + AutoLockGC lock(this); + fullChunks(lock).verifyChunks(); + availableChunks(lock).verifyChunks(); + emptyChunks(lock).verifyChunks(); +} + +void ChunkPool::verifyChunks() const { + for (TenuredChunk* chunk = head_; chunk; chunk = chunk->info.next) { + chunk->verify(); + } +} + +void TenuredChunk::verify() const { + size_t freeCount = 0; + size_t freeCommittedCount = 0; + for (size_t i = 0; i < ArenasPerChunk; ++i) { + if (decommittedArenas[i]) { + // Free but not committed. + freeCount++; + continue; + } + + if (!arenas[i].allocated()) { + // Free and committed. + freeCount++; + freeCommittedCount++; + } + } + + MOZ_ASSERT(freeCount == info.numArenasFree); + MOZ_ASSERT(freeCommittedCount == info.numArenasFreeCommitted); + + size_t freeListCount = 0; + for (Arena* arena = info.freeArenasHead; arena; arena = arena->next) { + freeListCount++; + } + + MOZ_ASSERT(freeListCount == info.numArenasFreeCommitted); +} + #endif void ChunkPool::Iter::next() { @@ -852,16 +895,31 @@ bool TenuredChunk::decommitOneFreeArena(GCRuntime* gc, AutoLockGC& lock) { } void TenuredChunk::decommitFreeArenasWithoutUnlocking(const AutoLockGC& lock) { + info.freeArenasHead = nullptr; + Arena** freeCursor = &info.freeArenasHead; + for (size_t i = 0; i < ArenasPerChunk; ++i) { - if (decommittedArenas[i] || arenas[i].allocated()) { + Arena* arena = &arenas[i]; + if (decommittedArenas[i] || arena->allocated()) { continue; } - if (MarkPagesUnusedSoft(&arenas[i], ArenaSize)) { - info.numArenasFreeCommitted--; - decommittedArenas[i] = true; + if (js::oom::ShouldFailWithOOM() || + !MarkPagesUnusedSoft(arena, ArenaSize)) { + *freeCursor = arena; + freeCursor = &arena->next; + continue; } + + info.numArenasFreeCommitted--; + decommittedArenas[i] = true; } + + *freeCursor = nullptr; + +#ifdef DEBUG + verify(); +#endif } void TenuredChunk::updateChunkListAfterAlloc(GCRuntime* gc, @@ -5592,6 +5650,10 @@ void GCRuntime::beginSweepPhase(JS::GCReason reason, AutoGCSession& session) { releaseHeldRelocatedArenas(); +#ifdef DEBUG + verifyAllChunks(); +#endif + #ifdef JS_GC_ZEAL computeNonIncrementalMarkingForValidation(session); #endif diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h index e715fe0f1105..62b02b967406 100644 --- a/js/src/gc/GCRuntime.h +++ b/js/src/gc/GCRuntime.h @@ -111,6 +111,7 @@ class ChunkPool { public: bool contains(TenuredChunk* chunk) const; bool verify() const; + void verifyChunks() const; #endif public: @@ -518,6 +519,9 @@ class GCRuntime { NonEmptyChunksIter allNonEmptyChunks(const AutoLockGC& lock) { return NonEmptyChunksIter(availableChunks(lock), fullChunks(lock)); } +#ifdef DEBUG + void verifyAllChunks(); +#endif TenuredChunk* getOrAllocChunk(AutoLockGCBgAlloc& lock); void recycleChunk(TenuredChunk* chunk, const AutoLockGC& lock); diff --git a/js/src/gc/Heap.h b/js/src/gc/Heap.h index 00e4dac92b80..8494c01efff9 100644 --- a/js/src/gc/Heap.h +++ b/js/src/gc/Heap.h @@ -263,6 +263,8 @@ class alignas(ArenaSize) Arena { hasDelayedGrayMarking_ = 0; nextDelayedMarkingArena_ = 0; bufferedCells_ = nullptr; + + MOZ_ASSERT(!allocated()); } // Return an allocated arena to its unallocated state. @@ -653,6 +655,10 @@ class TenuredChunk : public TenuredChunkBase { /* Unlink and return the freeArenasHead. */ Arena* fetchNextFreeArena(GCRuntime* gc); +#ifdef DEBUG + void verify() const; +#endif + private: /* Search for a decommitted arena to allocate. */ unsigned findDecommittedArenaOffset(); diff --git a/js/src/jit-test/tests/gc/bug-1692221.js b/js/src/jit-test/tests/gc/bug-1692221.js new file mode 100644 index 000000000000..6300788ad90e --- /dev/null +++ b/js/src/jit-test/tests/gc/bug-1692221.js @@ -0,0 +1,39 @@ +// |jit-test| allow-oom; skip-if: !('oomAtAllocation' in this) + +// Test TenuredChunk::decommitFreeArenasWithoutUnlocking updates chunk +// metadata correctly. The data is checked by assertions so this test is about +// exercising the code in question. + +function allocateGarbage() { + gc(); + for (let j = 0; j < 100000; j++) { + Symbol(); + } +} + +function collectUntilDecommit() { + startgc(1); + while (gcstate() != "NotActive" && gcstate() != "Decommit") { + gcslice(1000); + } +} + +function triggerSyncDecommit() { + reportLargeAllocationFailure(1); +} + +gczeal(0); + +// Normally we skip decommit if GCs are happening frequently. Disable that for +// this test +gcparam("highFrequencyTimeLimit", 0); + +allocateGarbage(); +collectUntilDecommit(); +triggerSyncDecommit(); + +allocateGarbage(); +collectUntilDecommit(); +oomAtAllocation(10); +triggerSyncDecommit(); +resetOOMFailure();