commit 3508030f2a03abed180ca7b9c7e08b9f8a6aebcf
parent 88fce639ea50689baefaa07e0af5813611ed4e79
Author: Jon Coppeard <jcoppeard@mozilla.com>
Date: Mon, 10 Nov 2025 18:03:44 +0000
Bug 1997894 - Fix delayed check in AssertCellIsNotGray to work for symbols r=jandem
The JS::AssertCellIsNotGray check is delayed if incremental gray marking is
currently happening in the cell's zone, because the final marking state has not
been reached yet. Currently this uses a single vector for the runtime to record
such cells which are checked when their zone finishes gray marking.
When we allowed symbols to be gray we started seeing assertion failures. This
is because the atoms zone starts gray marking at the start of GC unlike all the
other zones (see Zone::initialMarkingState). This means the assertion can
unexpectedly see elements in the vector in GCRuntime::beginMarkingSweepGroup.
This is a problem with the assertions and can be fixed by splitting up the
vector so it is per-zone.
I wasn't able to come up with a testcase to reproduce this in the shell.
Differential Revision: https://phabricator.services.mozilla.com/D271954
Diffstat:
4 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp
@@ -3116,6 +3116,8 @@ void GCRuntime::beginMarkPhase(AutoGCSession& session) {
{
BufferAllocator::MaybeLock lock;
for (GCZonesIter zone(this); !zone.done(); zone.next()) {
+ MOZ_ASSERT(zone->cellsToAssertNotGray().empty());
+
// In an incremental GC, clear the arena free lists to ensure that
// subsequent allocations refill them and end up marking new cells black.
// See arenaAllocatedDuringGC().
@@ -3576,11 +3578,11 @@ void GCRuntime::checkGCStateNotInUse() {
MOZ_ASSERT(!zone->isOnList());
MOZ_ASSERT(!zone->gcNextGraphNode);
MOZ_ASSERT(!zone->gcNextGraphComponent);
+ MOZ_ASSERT(zone->cellsToAssertNotGray().empty());
zone->bufferAllocator.checkGCStateNotInUse();
}
MOZ_ASSERT(zonesToMaybeCompact.ref().isEmpty());
- MOZ_ASSERT(cellsToAssertNotGray.ref().empty());
MOZ_ASSERT(!atomsUsedByUncollectedZones.ref());
@@ -5553,15 +5555,15 @@ JS_PUBLIC_API void js::gc::detail::AssertCellIsNotGray(const Cell* cell) {
// called during GC and while iterating the heap for memory reporting.
MOZ_ASSERT(!JS::RuntimeHeapIsCycleCollecting());
- if (tc->zone()->isGCMarkingBlackAndGray()) {
+ Zone* zone = tc->zone();
+ if (zone->isGCMarkingBlackAndGray()) {
// We are doing gray marking in the cell's zone. Even if the cell is
// currently marked gray it may eventually be marked black. Delay checking
// non-black cells until we finish gray marking.
if (!tc->isMarkedBlack()) {
- JSRuntime* rt = tc->zone()->runtimeFromMainThread();
AutoEnterOOMUnsafeRegion oomUnsafe;
- if (!rt->gc.cellsToAssertNotGray.ref().append(cell)) {
+ if (!zone->cellsToAssertNotGray().append(cell)) {
oomUnsafe.crash("Can't append to delayed gray checks list");
}
}
diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h
@@ -1302,13 +1302,6 @@ class GCRuntime {
/* The test marking queue might want to be marking a particular color. */
mozilla::Maybe<js::gc::MarkColor> queueMarkColor;
-
- // During gray marking, delay AssertCellIsNotGray checks by
- // recording the cell pointers here and checking after marking has
- // finished.
- MainThreadData<Vector<const Cell*, 0, SystemAllocPolicy>>
- cellsToAssertNotGray;
- friend void js::gc::detail::AssertCellIsNotGray(const Cell*);
#endif
friend class SweepGroupsIter;
diff --git a/js/src/gc/Sweeping.cpp b/js/src/gc/Sweeping.cpp
@@ -945,6 +945,9 @@ void GCRuntime::moveToNextSweepGroup() {
zone->arenas.unmarkPreMarkedFreeCells();
zone->arenas.mergeArenasFromCollectingLists();
zone->clearGCSliceThresholds();
+#ifdef DEBUG
+ zone->cellsToAssertNotGray().clearAndFree();
+#endif
}
for (SweepGroupCompartmentsIter comp(rt); !comp.done(); comp.next()) {
@@ -1224,7 +1227,6 @@ IncrementalProgress GCRuntime::beginMarkingSweepGroup(JS::GCContext* gcx,
for (auto& marker : markers) {
MOZ_ASSERT(marker->markColor() == MarkColor::Black);
}
- MOZ_ASSERT(cellsToAssertNotGray.ref().empty());
#endif
gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::MARK);
@@ -1234,6 +1236,8 @@ IncrementalProgress GCRuntime::beginMarkingSweepGroup(JS::GCContext* gcx,
// will be marked through, as they are not marked with
// TraceCrossCompartmentEdge.
for (SweepGroupZonesIter zone(this); !zone.done(); zone.next()) {
+ MOZ_ASSERT_IF(!zone->isGCMarkingBlackAndGray(),
+ zone->cellsToAssertNotGray().empty());
zone->changeGCState(zone->initialMarkingState(), Zone::MarkBlackAndGray);
}
@@ -1653,14 +1657,14 @@ IncrementalProgress GCRuntime::beginSweepingSweepGroup(JS::GCContext* gcx,
if (zone->isAtomsZone()) {
sweepingAtoms = true;
}
- }
#ifdef DEBUG
- for (const auto* cell : cellsToAssertNotGray.ref()) {
- JS::AssertCellIsNotGray(cell);
- }
- cellsToAssertNotGray.ref().clearAndFree();
+ for (const auto* cell : zone->cellsToAssertNotGray()) {
+ JS::AssertCellIsNotGray(cell);
+ }
+ zone->cellsToAssertNotGray().clearAndFree();
#endif
+ }
// Updating the atom marking bitmaps. This marks atoms referenced by
// uncollected zones so cannot be done in parallel with the other sweeping
diff --git a/js/src/gc/Zone.h b/js/src/gc/Zone.h
@@ -451,10 +451,6 @@ class Zone : public js::ZoneAllocator, public js::gc::GraphNodeBase<JS::Zone> {
js::MainThreadData<js::StringStats> previousGCStringStats;
js::MainThreadData<js::StringStats> stringStats;
-#ifdef DEBUG
- js::MainThreadData<unsigned> gcSweepGroupIndex;
-#endif
-
js::gc::PretenuringZone pretenuring;
private:
@@ -554,6 +550,16 @@ class Zone : public js::ZoneAllocator, public js::gc::GraphNodeBase<JS::Zone> {
using ObjectVector = js::GCVector<JSObject*, 0, js::SystemAllocPolicy>;
js::MainThreadOrGCTaskData<ObjectVector> objectsWithWeakPointers;
+#ifdef DEBUG
+ js::MainThreadData<unsigned> gcSweepGroupIndex;
+
+ // During gray marking, delay AssertCellIsNotGray checks by
+ // recording the cell pointers here and checking after marking has
+ // finished.
+ js::MainThreadData<js::Vector<const js::gc::Cell*, 0, js::SystemAllocPolicy>>
+ cellsToAssertNotGray_;
+#endif
+
public:
#ifdef JS_GC_ZEAL
// Must come after weakCaches_ above.
@@ -956,6 +962,8 @@ class Zone : public js::ZoneAllocator, public js::gc::GraphNodeBase<JS::Zone> {
// For testing purposes, return the index of the sweep group which this zone
// was swept in in the last GC.
unsigned lastSweepGroupIndex() { return gcSweepGroupIndex; }
+
+ auto& cellsToAssertNotGray() { return cellsToAssertNotGray_.ref(); }
#endif
// Support for invalidating fuses
