← All reports

[JSC] Redesign WeakGCSet to match WeakGCMap

Component: JavaScriptCore Heap | 7692eec

Source/JavaScriptCore/runtime/WeakGCSet.h

-template<typename T>
-struct WeakGCSetHash {
- static unsigned hash(const Weak<T>& p) { return PtrHash<T*>::hash(p.get()); }
- static bool equal(const Weak<T>& a, const Weak<T>& b) {
- if (!a || !b) return false;
- return a.get() == b.get();
- }
-};
AddResult add(ValueArg* value)
{
- AssertNoGC assertNoGC;
- return m_set.add(key);
+ AddResult result = m_set.add(value);
+ markDirty(m_vm);
+ return result;
}

Source/JavaScriptCore/runtime/WeakGCSetInlines.h

-inline void WeakGCSet<...>::reconcileWeakReferencesAtGCEnd(VM&, CollectionScope collectionScope)
-{
- if (collectionScope == CollectionScope::Full)
- pruneStaleEntries();
-}
-NEVER_INLINE void WeakGCSet<...>::pruneStaleEntries()
-{
- m_set.removeIf([](auto& entry) { return !entry; });
-}
+NEVER_INLINE void WeakGCSet<...>::reconcileWeakReferencesAtGCEnd(VM& vm, CollectionScope)
+{
+ // A set entry is its own key, so unlike WeakGCMap there is no value to null out and leave for
+ // the next full collection: both scopes remove.
+ m_set.removeIf([&](ValueArg* value) { return !vm.heap.isMarked(value); });
+}

WeakGCSet is a weak hash-set used to memoize JS objects without keeping them alive across GC — JSGlobalObject uses it to cache the JSCustomGetterFunction/JSCustomSetterFunction objects built for accessors like RegExp's $&, $_, and input, so repeated Object.getOwnPropertyDescriptor() calls return the same function object. This commit ports the WeakGCMap redesign (320942@main) to it: buckets now hold raw ValueArg* pointers instead of Weak<T> handles, dead entries are pruned by a single vm.heap.isMarked() sweep in reconcileWeakReferencesAtGCEnd, and add()/ensure() call markDirty(m_vm) so the set is guaranteed to be revisited at GC end. Callers — the custom getter/setter cache, WasmTable::grow, and ScriptExecutionContext's microtask global object cache — are updated to dereference raw pointers directly instead of null-checking a Weak<T>.

Before:                                          After:
bucket ── Weak<T> handle ──► T cell               bucket ── raw T* (unguarded)
  hash()/equal()/isWeakNullValue()                  hash()/equal() dereference directly
  null-check the handle before use                  add()/ensure() call markDirty(m_vm)
  pruneStaleEntries() only on Full GC:              reconcileWeakReferencesAtGCEnd() runs
    removeIf(!entry)                                 after EVERY collection:
  (eden-only deaths linger to next full GC)           removeIf(!vm.heap.isMarked(T*))

Pruning now happens after every collection instead of being deferred to full GCs, so a dead bucket is never observably present — which is what lets hash() and equal() drop their null checks. This is a design port for consistency with the already-updated WeakGCMap, not a response to a discovered vulnerability, and it removes per-entry Weak<> handle overhead along with the null-check boilerplate at every call site.

Because hash()/equal() no longer guard their bucket pointers, correctness now depends entirely on reconcileWeakReferencesAtGCEnd running and completing before any lookup, iteration, or hash computation can observe an unmarked cell. Narrow: verify every mutating path reliably calls markDirty(m_vm) — a missed markDirty could leave the set unvisited at a GC end phase, letting a dead raw pointer survive its cell's collection into an unguarded dereference — and check the safeToCompareToEmptyOrDeleted=false contract on WeakCustomGetterOrSetterHash in JSGlobalObject.h, whose new comment notes that WeakCustomGetterOrSetterHashTranslator::equal() unconditionally dereferences whatever bucket it is handed, making that flag load-bearing. Wider: enumerate the other JSC weak-collection call sites still using the older deferred-Weak<> pattern this commit replaces — each carries the same reconciliation-timing dependency once migrated, so the migration order is itself the risk. Widest: any data structure that trades per-entry validity checks for a global "sweep runs before anyone looks" invariant has this shape; the audit question to carry is what code can run between the collection and the sweep. Code-review tell: a removed null check in a hash or equality functor, paired with a new markDirty-style notification in the mutators.