commit 19e5fe9057c4e735bfb93f991823b16023ddfaf5
parent 95112dc0345dc697fccc7839db9678af6bba5424
Author: Jon Coppeard <jcoppeard@mozilla.com>
Date: Wed, 15 Oct 2025 07:29:23 +0000
Bug 1994023 - Rework nsCycleCollector::FixGrayBits to avoid unnecessary work r=sfink,mccr8
Currently this can fix up the gray bits before doing a GC (which will calculate
them from scratch), as well as fix them up after a GC (which will do nothing).
Differential Revision: https://phabricator.services.mozilla.com/D268433
Diffstat:
6 files changed, 85 insertions(+), 49 deletions(-)
diff --git a/js/public/HeapAPI.h b/js/public/HeapAPI.h
@@ -806,6 +806,8 @@ static MOZ_ALWAYS_INLINE Zone* GetStringZone(JSString* str) {
extern JS_PUBLIC_API Zone* GetObjectZone(JSObject* obj);
+// Check whether a GC thing is gray. If the gray marking state is unknown
+// (e.g. due to OOM during gray unmarking) this returns false.
static MOZ_ALWAYS_INLINE bool GCThingIsMarkedGray(GCCellPtr thing) {
js::gc::Cell* cell = thing.asCell();
if (IsInsideNursery(cell)) {
@@ -825,13 +827,8 @@ static MOZ_ALWAYS_INLINE bool GCThingIsMarkedGrayInCC(GCCellPtr thing) {
}
auto* tenuredCell = reinterpret_cast<js::gc::TenuredCell*>(cell);
- if (!js::gc::detail::TenuredCellIsMarkedGray(tenuredCell)) {
- return false;
- }
-
MOZ_ASSERT(js::gc::detail::CanCheckGrayBits(tenuredCell));
-
- return true;
+ return js::gc::detail::TenuredCellIsMarkedGray(tenuredCell);
}
extern JS_PUBLIC_API JS::TraceKind GCThingTraceKind(void* thing);
diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp
@@ -3265,6 +3265,13 @@ static bool NondeterministicGetWeakMapKeys(JSContext* cx, unsigned argc,
return true;
}
+static bool SetGrayBitsInvalid(JSContext* cx, unsigned argc, Value* vp) {
+ CallArgs args = CallArgsFromVp(argc, vp);
+ cx->runtime()->gc.setGrayBitsInvalid();
+ args.rval().setUndefined();
+ return true;
+}
+
class HasChildTracer final : public JS::CallbackTracer {
RootedValue child_;
bool found_;
@@ -10399,6 +10406,10 @@ gc::ZealModeHelpText),
"nondeterministicGetWeakMapKeys(weakmap)",
" Return an array of the keys in the given WeakMap."),
+ JS_FN_HELP("setGrayBitsInvalid", SetGrayBitsInvalid, 0, 0,
+"setGrayBitsInvalid()",
+" Set the gray bits state to invalid."),
+
JS_FN_HELP("internalConst", InternalConst, 1, 0,
"internalConst(name)",
" Query an internal constant for the engine. See InternalConst source for\n"
diff --git a/xpcom/base/CycleCollectedJSRuntime.cpp b/xpcom/base/CycleCollectedJSRuntime.cpp
@@ -1755,6 +1755,8 @@ bool CycleCollectedJSRuntime::UsefulToMergeZones() const { return false; }
void CycleCollectedJSRuntime::FixWeakMappingGrayBits() const {
MOZ_ASSERT(!JS::IsIncrementalGCInProgress(mJSRuntime),
"Don't call FixWeakMappingGrayBits during a GC.");
+ MOZ_ASSERT(AreGCGrayBitsValid());
+
FixWeakMappingGrayBitsTracer fixer(mJSRuntime);
fixer.FixAll();
}
diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp
@@ -3456,12 +3456,31 @@ void nsCycleCollector::CheckThreadSafety() {
#endif
}
+static void SendNeedGCTelemetry(bool needGC) {
+ if (NS_IsMainThread()) {
+ glean::cycle_collector::need_gc
+ .EnumGet(static_cast<glean::cycle_collector::NeedGcLabel>(needGC))
+ .Add();
+ return;
+ }
+
+ glean::cycle_collector::worker_need_gc
+ .EnumGet(static_cast<glean::cycle_collector::WorkerNeedGcLabel>(needGC))
+ .Add();
+}
+
// The cycle collector uses the mark bitmap to discover what JS objects are
-// reachable only from XPConnect roots that might participate in cycles. We ask
-// the JS runtime whether we need to force a GC before this CC. It should only
-// be true when UnmarkGray has run out of stack. We also force GCs on shutdown
-// to collect cycles involving both DOM and JS, and in WantAllTraces CCs to
-// prevent hijinks from ForgetSkippable and compartmental GCs.
+// 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.
+//
+// We also force GCs on shutdown to collect cycles involving both DOM and JS,
+// and in WantAllTraces CCs to prevent hijinks from ForgetSkippable and
+// compartmental GCs.
+//
+// If we don't need to GC we still need to fix up the gray bits since gray
+// unmarking doesn't trace through weakmaps. We don't need to do this after GC.
void nsCycleCollector::FixGrayBits(bool aIsShutdown, TimeLog& aTimeLog) {
CheckThreadSafety();
@@ -3469,52 +3488,34 @@ void nsCycleCollector::FixGrayBits(bool aIsShutdown, TimeLog& aTimeLog) {
return;
}
- // If we're not forcing a GC anyways due to shutdown or an all traces CC,
- // check to see if we still need to do one to fix the gray bits.
- if (!(aIsShutdown || (mLogger && mLogger->IsAllTraces()))) {
- mCCJSRuntime->FixWeakMappingGrayBits();
- aTimeLog.Checkpoint("FixWeakMappingGrayBits");
+ bool grayBitsInvalid = !mCCJSRuntime->AreGCGrayBitsValid();
- bool needGC = !mCCJSRuntime->AreGCGrayBitsValid();
- // Only do a telemetry ping for non-shutdown CCs.
- if (NS_IsMainThread()) {
- glean::cycle_collector::need_gc
- .EnumGet(static_cast<glean::cycle_collector::NeedGcLabel>(needGC))
- .Add();
- } else {
- glean::cycle_collector::worker_need_gc
- .EnumGet(
- static_cast<glean::cycle_collector::WorkerNeedGcLabel>(needGC))
- .Add();
- }
+ bool wantAllTraces = mLogger && mLogger->IsAllTraces();
- if (!needGC) {
- return;
- }
+ if (!aIsShutdown && !wantAllTraces) {
+ SendNeedGCTelemetry(grayBitsInvalid);
}
- mResults.mForcedGC = true;
+ if (!aIsShutdown && !wantAllTraces && !grayBitsInvalid) {
+ // No need to GC. We only need to fix up the gray bits.
+ mCCJSRuntime->FixWeakMappingGrayBits();
+ aTimeLog.Checkpoint("FixGrayBits::FixWeakMappingGrayBits");
+ return;
+ }
- uint32_t count = 0;
- do {
- if (aIsShutdown) {
- mCCJSRuntime->GarbageCollect(JS::GCOptions::Shutdown,
- JS::GCReason::SHUTDOWN_CC);
- } else {
- mCCJSRuntime->GarbageCollect(JS::GCOptions::Normal,
- JS::GCReason::CC_FORCED);
- }
+ mResults.mForcedGC = true;
- mCCJSRuntime->FixWeakMappingGrayBits();
+ JS::GCOptions options = JS::GCOptions::Normal;
+ JS::GCReason reason = JS::GCReason::CC_FORCED;
+ if (aIsShutdown) {
+ options = JS::GCOptions::Shutdown;
+ reason = JS::GCReason::SHUTDOWN_CC;
+ }
- // It's possible that FixWeakMappingGrayBits will hit OOM when unmarking
- // gray and we will have to go round again. The second time there should not
- // be any weak mappings to fix up so the loop body should run at most twice.
- MOZ_RELEASE_ASSERT(count < 2);
- count++;
- } while (!mCCJSRuntime->AreGCGrayBitsValid());
+ mCCJSRuntime->GarbageCollect(options, reason);
+ MOZ_ASSERT(mCCJSRuntime->AreGCGrayBitsValid());
- aTimeLog.Checkpoint("FixGrayBits");
+ aTimeLog.Checkpoint("FixGrayBits::GarbageCollect");
}
bool nsCycleCollector::IsIncrementalGCInProgress() {
diff --git a/xpcom/tests/unit/test_invalidGrayBits.js b/xpcom/tests/unit/test_invalidGrayBits.js
@@ -0,0 +1,23 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Check that the CC handles invalid gray bits without triggering an
+// assertion. This condition arises due to OOM and rare in actual use.
+
+function run_test() {
+ // Create some weak map entries that may need to have their gray marking
+ // fixed.
+ let wm = new WeakMap();
+ let key = {};
+ wm.set(key, {});
+ Cu.getJSTestingFunctions().minorgc();
+
+ // Mark gray bits as invalid.
+ Cu.getJSTestingFunctions().setGrayBitsInvalid();
+
+ // Cycle collect.
+ Cu.forceCC();
+}
diff --git a/xpcom/tests/unit/xpcshell.toml b/xpcom/tests/unit/xpcshell.toml
@@ -57,6 +57,8 @@ fail-if = ["os == 'android'"]
["test_iniParser.js"]
+["test_invalidGrayBits.js"]
+
["test_ioutil.js"]
["test_localfile.js"]
