tor-browser

commit 988b462ce07aa2d245372cb020ca5716e4cb5ebf
parent 117ed7db99075a445deec14d08d4c113810c7d5f
Author: Jon Coppeard <jcoppeard@mozilla.com>
Date:   Tue, 16 Dec 2025 08:55:40 +0000

Bug 2004443 - Don't mark symbols gray in zones that are not being marked r=sfink

When adding gray marking for symbols we needed marking to mark symbols black in
uncollected zones because GC marking handles gray unmarking in zones that are
being marked.

However that change also marked symbols gray in uncollected zones which leads
to this problem where you can have an unmarked object referencing an unmarked
symbol which later gets marked gray. When reading the symbol out of the object
gray unmarking doesn't unmark the symbol because the object was not itself
marked.

The fix is to only mark symbols in uncollected zones when black marking, which
handles the gray unmarking case without adding this problem.

Differential Revision: https://phabricator.services.mozilla.com/D276038

Diffstat:
M
js/src/gc/Marking.cpp
 | 
7
++++++-
A
js/src/jit-test/tests/gc/bug-2004443.js
 | 
39
+++++++++++++++++++++++++++++++++++++++
M
js/src/shell/js.cpp
 | 
2
+-
M
js/src/shell/jsshell.h
 | 
5
++++-

4 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp
@@ -1214,7 +1214,12 @@ bool js::GCMarker::mark(T* thing) {
   }
 
   if constexpr (std::is_same_v<T, JS::Symbol>) {
-    if (IsOwnedByOtherRuntime(runtime(), thing)) {
+    // Don't mark symbols owned by other runtimes. Mark symbols black in
+    // uncollected zones for gray unmarking, but don't mark symbols gray in
+    // uncollected zones.
+    if (IsOwnedByOtherRuntime(runtime(), thing) ||
+        (markColor() == MarkColor::Gray &&
+         !thing->zone()->isGCMarkingOrVerifyingPreBarriers())) {
       return false;
     }
   }
diff --git a/js/src/jit-test/tests/gc/bug-2004443.js b/js/src/jit-test/tests/gc/bug-2004443.js
@@ -0,0 +1,39 @@
+function checkMarks(expected) {
+  assertEq(getMarks().join(", "), expected.join(", "));
+}
+
+gczeal(0);
+gczeal('CheckGrayMarking');
+gc();
+
+let g1 = newGlobal({newCompartment: true});
+let g2 = newGlobal({newCompartment: true});
+let defineTestFunction = `
+  function makeTestObject(s) {
+    let o = {x: s};
+    addMarkObservers([o]);
+    grayRoot()[0] = o;
+    o = undefined;
+  }
+  `;
+g1.eval(defineTestFunction);
+g2.eval(defineTestFunction);
+
+let s = Symbol();
+g1.makeTestObject(s);
+g2.makeTestObject(s);
+addMarkObservers([s]);
+s = undefined;
+
+checkMarks(['unmarked', 'unmarked', 'unmarked']);
+
+schedulezone(g1);
+startgc();
+finishgc();
+checkMarks(['gray', 'unmarked', 'unmarked']);
+
+g2.eval('grayRoot()[0].x');
+checkMarks(['gray', 'unmarked', 'unmarked']);
+
+gc();
+checkMarks(['gray', 'gray', 'gray']);
diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
@@ -8944,7 +8944,7 @@ static bool AddMarkObservers(JSContext* cx, unsigned argc, Value* vp) {
       return false;
     }
 
-    if (!CanBeHeldWeakly(value)) {
+    if (!value.isObject() && !value.isSymbol()) {
       JS_ReportErrorASCII(cx, "Can only observe objects and symbols");
       return false;
     }
diff --git a/js/src/shell/jsshell.h b/js/src/shell/jsshell.h
@@ -169,7 +169,10 @@ class NonShrinkingValueVector
  public:
   bool traceWeak(JSTracer* trc) {
     for (HeapPtr<Value>& value : *this) {
-      TraceWeakEdge(trc, &value, "NonShrinkingValueVector element");
+      if (value.isGCThing() &&
+          value.toGCThing()->zoneFromAnyThread()->isGCSweeping()) {
+        TraceWeakEdge(trc, &value, "NonShrinkingValueVector element");
+      }
     }
     return true;
   }