commit 83ce0f20261e56e178598a403efdc3d269b7f79f
parent 0e8962595bfe71884fac46dd4eaddee778efba82
Author: Jon Coppeard <jcoppeard@mozilla.com>
Date: Thu, 30 Oct 2025 12:55:38 +0000
Bug 1997118 - Mark gray bits as invalid if we abort GC during marking r=sfink
We depend on GC marking to propagate gray umarking through zones that are being
marked incrementally. Aborting this raises the possibility of creating black to
gray edges as previously happened with this testcase.
Marking the gray bits as invalid will cause the CC to perform another GC to
redo the marking. Aborting GC is pretty rare in practice so I don't expect this
to have a performance impact.
Differential Revision: https://phabricator.services.mozilla.com/D270546
Diffstat:
5 files changed, 110 insertions(+), 2 deletions(-)
diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp
@@ -3264,6 +3264,12 @@ static bool NondeterministicGetWeakMapKeys(JSContext* cx, unsigned argc,
return true;
}
+static bool GrayBitsValid(JSContext* cx, unsigned argc, Value* vp) {
+ CallArgs args = CallArgsFromVp(argc, vp);
+ args.rval().setBoolean(cx->runtime()->gc.areGrayBitsValid());
+ return true;
+}
+
static bool SetGrayBitsInvalid(JSContext* cx, unsigned argc, Value* vp) {
CallArgs args = CallArgsFromVp(argc, vp);
cx->runtime()->gc.setGrayBitsInvalid();
@@ -10407,6 +10413,10 @@ gc::ZealModeHelpText),
"nondeterministicGetWeakMapKeys(weakmap)",
" Return an array of the keys in the given WeakMap."),
+ JS_FN_HELP("grayBitsValid", GrayBitsValid, 0, 0,
+"grayBitsValid()",
+" Return whether the gray bits state is valid."),
+
JS_FN_HELP("setGrayBitsInvalid", SetGrayBitsInvalid, 0, 0,
"setGrayBitsInvalid()",
" Set the gray bits state to invalid."),
diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp
@@ -3803,6 +3803,11 @@ GCRuntime::IncrementalResult GCRuntime::resetIncrementalGC(
resetGrayList(c);
}
+ // The gray marking state may not be valid. We depend on the mark stack to
+ // do gray unmarking in zones that are being marked by the GC and we've
+ // just cancelled that part way through.
+ setGrayBitsInvalid();
+
// Wait for sweeping of nursery owned sized allocations to finish.
nursery().joinSweepTask();
diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp
@@ -2929,6 +2929,10 @@ void UnmarkGrayTracer::onChild(JS::GCCellPtr thing, const char* name) {
void UnmarkGrayTracer::unmark(JS::GCCellPtr cell) {
MOZ_ASSERT(stack.empty());
+ // TODO: We probably don't need to do anything if the gray bits are
+ // invalid. However an early return here causes ExposeGCThingToActiveJS to
+ // fail because it asserts that something gets unmarked.
+
onChild(cell, "unmarking root");
while (!stack.empty() && !oom) {
@@ -2940,7 +2944,6 @@ void UnmarkGrayTracer::unmark(JS::GCCellPtr cell) {
// GC again before the next CC.
stack.clear();
runtime()->gc.setGrayBitsInvalid();
- return;
}
}
diff --git a/js/src/jit-test/tests/gc/gray-unmarking.js b/js/src/jit-test/tests/gc/gray-unmarking.js
@@ -0,0 +1,89 @@
+// Test interaction of gray marking / cross zone pointers / aborted GC marking.
+
+gczeal(0);
+
+gc();
+assertEq(grayBitsValid(), true);
+
+// Create some globals in different zones.
+let g1 = newGlobal({newCompartment: true});
+let g2 = newGlobal({newCompartment: true});
+let g3 = newGlobal({newCompartment: true});
+
+// Set up a linked list of objects in different zones: a --> b --> c
+g1.eval('var a = {}');
+g2.eval('var b = {}');
+g3.eval('var c = {}');
+g1.a.next = g2.b;
+g2.b.next = g3.c;
+
+// Observe mark state of the objects and remove extra references from the globals.
+g1.eval('addMarkObservers([a])');
+g2.eval('addMarkObservers([b])');
+g3.eval('addMarkObservers([c])');
+g2.b = undefined;
+g3.c = undefined;
+
+function checkMarks(a, b, c) {
+ assertEq(getMarks().join(", "), [a, b, c].join(", "));
+}
+
+// Check GC initially marks everything black.
+gc();
+checkMarks("black", "black", "black");
+
+// Replace root with a gray one and check GC marks everything gray.
+g1.eval('grayRoot()[0] = a');
+g1.a = undefined;
+gc();
+checkMarks("gray", "gray", "gray");
+
+// Read the gray root and check gray unmarking marks everything black again.
+g1.eval('grayRoot()[0]');
+checkMarks("black", "black", "black");
+
+// Reset everything to gray.
+gc();
+checkMarks("gray", "gray", "gray");
+
+// Start marking zone 2.
+schedulezone(g2);
+startgc(10);
+while (gcstate() === "Prepare" || gcstate() === "MarkRoots") {
+ gcslice(10);
+}
+assertEq(gcstate(), "Mark");
+assertEq(gcstate(g1), "NoGC");
+assertEq(gcstate(g2), "MarkBlackOnly");
+assertEq(gcstate(g3), "NoGC");
+
+// Check zone 2's mark bits have been cleared.
+checkMarks("gray", "unmarked", "gray");
+
+// Gray unmarking stops at the zone that is being mraked
+g1.eval('grayRoot()[0]');
+checkMarks("black", "black", "gray");
+
+// GC marking handles the gray unmarking by propagaing b's mark state.
+finishgc();
+assertEq(grayBitsValid(), true);
+checkMarks("black", "black", "black");
+
+// Reset everything to gray.
+gc();
+checkMarks("gray", "gray", "gray");
+
+// Repeat the previous test but abort marking after unmarking a. c is
+// left as 'gray' (incorrect) but the gray marking state is marked as invalid.
+schedulezone(g2);
+startgc(10);
+while (gcstate() === "Prepare" || gcstate() === "MarkRoots") {
+ gcslice(10);
+}
+assertEq(gcstate(), "Mark");
+checkMarks("gray", "unmarked", "gray");
+g1.eval('grayRoot()[0]');
+assertEq(grayBitsValid(), true);
+abortgc();
+assertEq(grayBitsValid(), false);
+checkMarks("black", "black", "gray");
diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp
@@ -3470,7 +3470,8 @@ static void SendNeedGCTelemetry(bool needGC) {
// reachable only from XPConnect roots that might participate in cycles.
//
// That data might not currently be valid, requiring a GC to restore it. This is
-// rare in practice and is caused by an OOM during gray unmarking.
+// rare in practice and is caused by an OOM during gray unmarking or aborting GC
+// part way through marking.
//
// We also force GCs on shutdown to collect cycles involving both DOM and JS,
// and in WantAllTraces CCs to prevent hijinks from ForgetSkippable and
