diff --git a/docs/jsg.md b/docs/jsg.md index 40a5e6a7d98..a8a44908135 100644 --- a/docs/jsg.md +++ b/docs/jsg.md @@ -2241,31 +2241,25 @@ if (jsg::HeapTracer::isInCppgcDestructor()) { ### Wrapper Lifecycle ``` -1. Wrappable created (no JS wrapper yet) +1. C++ object created (no JS wrapper yet) | -2. Wrappable passed to JavaScript +2. Object passed to JavaScript | -3. attachWrapper() attaches the JS wrapper, allocating a cppgc shim - (or reusing one from the freelist) +3. attachWrapper() creates JS wrapper | -4. wrapper -> shim -> Wrappable: - - the wrapper keeps the shim alive, via V8's CppHeap pointer table - - the shim holds a strong kj::Own - - so the Wrappable cannot be destroyed while a wrapper exists +4. JS wrapper and C++ object linked | -5. GC may collect the wrapper if: +5. GC may collect wrapper if: - No JS references exist - - No strong Refs exist (one would root the wrapper) + - No strong Refs exist - Wrapper is "unmodified" | -6. detachWrapper() runs when the wrapper goes away, from: - - ~CppgcShim, after a major GC collected the wrapper - - ResetRoot(), when V8 drops an unmodified droppable wrapper - - clearWrappers(), at isolate shutdown - It releases the shim's reference to the Wrappable. +6. If wrapper collected but C++ object still alive: + - New wrapper created on next JS access | -7. If other C++ references remain, the Wrappable lives on and a new - wrapper is created on the next JS access. Otherwise it is destroyed. +7. When C++ object destroyed: + - detachWrapper() called + - JS wrapper becomes empty shell ``` ### Async Destructor Safety diff --git a/src/workerd/jsg/condemned-wrapper-test.c++ b/src/workerd/jsg/condemned-wrapper-test.c++ deleted file mode 100644 index 37afa57869e..00000000000 --- a/src/workerd/jsg/condemned-wrapper-test.c++ +++ /dev/null @@ -1,162 +0,0 @@ -// Copyright (c) 2026 Cloudflare, Inc. -// Licensed under the Apache 2.0 license found in the LICENSE file or at: -// https://opensource.org/licenses/Apache-2.0 - -// Tests for the window in which a Wrappable's wrapper has been collected by a major GC but the -// ~CppgcShim that releases the Wrappable has not run yet. In that window the Wrappable is -// condemned: it is still addressable, and its WeakRef anchor still reports alive, but promoting a -// WeakRef to a strong Ref would resurrect a doomed object. See Wrappable::isCondemned(). -// -// A forced GC normally sweeps atomically and runs ~CppgcShim before returning, so the window never -// opens. Lock::requestGcWithDeferredSweepForTesting() leaves the sweep pending, which is what a -// natural major GC does, and makes the window observable without relying on allocation pressure to -// be handed a GC at the right moment. - -#include "jsg-test.h" - -namespace workerd::jsg::test { -namespace { - -// A pending sweep stays pending for the rest of the test without any special GC flags, because -// cppgc only ever runs finalizers on the mutator thread: a concurrent sweep task merely collects -// unfinalized objects (DeferredFinalizationBuilder) and SweepFinalizer drains that list on the -// mutator thread. Nothing between requestGcWithDeferredSweepForTesting() and the assertions -// allocates or pumps the foreground task runner, so ~CppgcShim cannot run until -// finishDeferredSweepForTesting() asks for it. -V8System v8System({"--expose-gc"_kj}); - -class ContextGlobalObject: public Object, public ContextGlobal {}; - -// Set by DeferredSweepContext::makeBox(). A namespace-scope variable rather than a member because -// Evaluator::run() does not hand the caller a reference to the context object. -kj::Maybe> weakBox; - -struct DeferredSweepContext: public ContextGlobalObject { - // Allocates a NumberBox and remembers a weak ref to it. Returning the Ref is what gives the - // object a JS wrapper, which is the thing the GC later collects. - Ref makeBox(Lock& js) { - auto box = js.alloc(42); - weakBox = box.getWeakRef(js); - return box; - } - - JSG_RESOURCE_TYPE(DeferredSweepContext) { - JSG_NESTED_TYPE(NumberBox); - JSG_METHOD(makeBox); - } -}; -JSG_DECLARE_ISOLATE_TYPE(DeferredSweepIsolate, DeferredSweepContext, NumberBox); - -// Creates a wrapped NumberBox reachable only from JS, then drops the JS reference, leaving the -// wrapper collectable. The handle scope keeps the script's own handles from rooting it. -void makeCollectableBox(Lock& js) { - js.withinHandleScope([&] { - auto source = "let b = makeBox(); b = null;"_kj; - auto script = check(v8::Script::Compile(js.v8Context(), js.str(source))); - check(script->Run(js.v8Context())); - }); - KJ_ASSERT(weakBox != kj::none); -} - -KJ_TEST("deferred sweep: WeakRef refuses to promote a condemned target") { - setPredictableModeForTest(); - Evaluator e(v8System); - e.run([](Lock& js) { - // Reset even if an assertion fails, so a WeakRef cannot outlive the isolate or - // leak into the next test. - KJ_DEFER(weakBox = kj::none); - auto& tracer = HeapTracer::getTracer(js.v8Isolate); - auto countBefore = tracer.getCondemnedWrapperCount(); - - makeCollectableBox(js); - auto& weak = KJ_ASSERT_NONNULL(weakBox); - - js.requestGcWithDeferredSweepForTesting(); - - // The Wrappable is still addressable: ~CppgcShim, which is what drops the shim's owning - // reference and runs ~Wrappable, has not been given a chance to run. - auto& box = KJ_ASSERT_NONNULL(weak.tryGet()); - KJ_ASSERT(box.value == 42); - - // Promotion must fail rather than resurrect it, and must record that it did so. The counter - // is what establishes that the refusal came from the condemned check: tryAddRef() only bumps - // it on the isCondemned() branch, so this is equivalent to asserting isCondemned() directly, - // which the test cannot do because Object inherits Wrappable privately. - KJ_ASSERT(weak.tryAddRef(js) == kj::none); - KJ_ASSERT(tracer.getCondemnedWrapperCount() == countBefore + 1); - - // Having refused once, the anchor is invalidated permanently. - KJ_ASSERT(!weak.isAlive()); - - js.finishDeferredSweepForTesting(); - }); -} - -KJ_TEST("deferred sweep: finishing the sweep releases the Wrappable") { - setPredictableModeForTest(); - Evaluator e(v8System); - e.run([](Lock& js) { - // Reset even if an assertion fails, so a WeakRef cannot outlive the isolate or - // leak into the next test. - KJ_DEFER(weakBox = kj::none); - makeCollectableBox(js); - auto& weak = KJ_ASSERT_NONNULL(weakBox); - - js.requestGcWithDeferredSweepForTesting(); - KJ_ASSERT(weak.isAlive()); - - // Running the deferred ~CppgcShim drops the last reference to the Wrappable, which invalidates - // the anchor through ~Wrappable() rather than through the condemned check. - js.finishDeferredSweepForTesting(); - KJ_ASSERT(!weak.isAlive()); - }); -} - -KJ_TEST("deferred sweep: isolate shutdown handles a condemned Wrappable") { - setPredictableModeForTest(); - KJ_DEFER(weakBox = kj::none); - - { - DeferredSweepIsolate isolate(v8System, kj::heap()); - isolate.runInLockScope([&](DeferredSweepIsolate::Lock& lock) { - JSG_WITHIN_CONTEXT_SCOPE( - lock, lock.newContext().getHandle(lock), [&](Lock& js) { - makeCollectableBox(js); - js.requestGcWithDeferredSweepForTesting(); - KJ_ASSERT(KJ_ASSERT_NONNULL(weakBox).isAlive()); - }); - }); - // The isolate is destroyed with ~CppgcShim still pending. - } - - KJ_ASSERT(!KJ_ASSERT_NONNULL(weakBox).isAlive()); -} - -// Contrast with the above: this is the behaviour the deferred-sweep hook exists to change. If this -// ever starts reporting a condemned wrapper, a forced GC has stopped sweeping atomically and the -// hook is no longer buying anything. -KJ_TEST("forced GC sweeps atomically, so the condemned window never opens") { - setPredictableModeForTest(); - Evaluator e(v8System); - e.run([](Lock& js) { - // Reset even if an assertion fails, so a WeakRef cannot outlive the isolate or - // leak into the next test. - KJ_DEFER(weakBox = kj::none); - auto& tracer = HeapTracer::getTracer(js.v8Isolate); - auto countBefore = tracer.getCondemnedWrapperCount(); - - makeCollectableBox(js); - auto& weak = KJ_ASSERT_NONNULL(weakBox); - - js.requestGcForTesting(); - - // ~CppgcShim ran inside the collection, so the anchor was invalidated by ~Wrappable() and - // there was never a condemned Wrappable for tryAddRef() to catch. - KJ_ASSERT(!weak.isAlive()); - KJ_ASSERT(weak.tryAddRef(js) == kj::none); - KJ_ASSERT(tracer.getCondemnedWrapperCount() == countBefore); - }); -} - -} // namespace -} // namespace workerd::jsg::test diff --git a/src/workerd/jsg/isolate-shutdown-test.c++ b/src/workerd/jsg/isolate-shutdown-test.c++ new file mode 100644 index 00000000000..32ce8fe4b8a --- /dev/null +++ b/src/workerd/jsg/isolate-shutdown-test.c++ @@ -0,0 +1,108 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +#include "jsg-test.h" + +namespace workerd::jsg::test { +namespace { + +// This test covers a shutdown failure involving a wrapped parent that owns an unwrapped child. +// RefHolder is returned to JavaScript and therefore has a JS wrapper and a cppgc shim. Child is +// never exposed to JavaScript, so it has no wrapper of its own, but tracing RefHolder propagates +// the isolate pointer to it through RefHolder::visitForGc(). +// +// A major GC can condemn RefHolder's wrapper while leaving its cppgc finalizer pending. Isolate +// teardown must finalize RefHolder while the isolate lock is held so that releasing its Ref +// can destroy Child immediately. If RefHolder instead remains owned by its condemned shim until +// v8::Isolate::Dispose(), its finalizer runs after the deferred-destruction queue has transitioned +// to DROPPED. Child still remembers the isolate and sees that the isolate lock is not held, so it +// tries to enqueue its destruction and fails the queueState == ACTIVE requirement. +// +// Production stacks reached the same path through container objects such as Response and R2 +// GetResult releasing ReadableStream references from CppgcShim::~CppgcShim(). +V8System v8System({"--expose-gc"_kj}); + +class ContextGlobalObject: public Object, public ContextGlobal {}; + +uint childDestructions = 0; +uint holderDestructions = 0; + +class Child final: public Object { + public: + ~Child() noexcept(false) { + ++childDestructions; + } + + JSG_RESOURCE_TYPE(Child) {} +}; + +class RefHolder final: public Object { + public: + explicit RefHolder(Ref child): child(kj::mv(child)) {} + + ~RefHolder() noexcept(false) { + ++holderDestructions; + } + + void visitForGc(GcVisitor& visitor) { + visitor.visit(child); + } + + JSG_RESOURCE_TYPE(RefHolder) {} + + private: + Ref child; +}; + +struct ShutdownContext: public ContextGlobalObject { + Ref makeHolder(Lock& js) { + return js.alloc(js.alloc()); + } + + JSG_RESOURCE_TYPE(ShutdownContext) { + JSG_NESTED_TYPE(Child); + JSG_NESTED_TYPE(RefHolder); + JSG_METHOD(makeHolder); + } +}; +JSG_DECLARE_ISOLATE_TYPE(ShutdownIsolate, ShutdownContext, Child, RefHolder); + +KJ_TEST("isolate shutdown finalizes condemned wrappers containing unwrapped children") { + setPredictableModeForTest(); + childDestructions = 0; + holderDestructions = 0; + + { + ShutdownIsolate isolate(v8System, kj::heap()); + isolate.runInLockScope([&](ShutdownIsolate::Lock& lock) { + JSG_WITHIN_CONTEXT_SCOPE( + lock, lock.newContext().getHandle(lock), [&](Lock& js) { + js.withinHandleScope([&] { + // Drop the only JS reference while the nested handle scope ensures no temporary V8 + // handle keeps RefHolder's wrapper alive. + auto source = "let holder = makeHolder(); holder = null;"_kj; + auto script = check(v8::Script::Compile(js.v8Context(), js.str(source))); + check(script->Run(js.v8Context())); + }); + + // Forced test GCs normally sweep atomically. Leave sweeping pending to reproduce the state + // created by a natural major GC immediately before isolate shutdown. + js.requestGcWithDeferredSweepForTesting(); + // Prove that neither cppgc finalization nor child destruction happened during the GC. This + // keeps the test focused on shutdown rather than the ordinary atomic-sweep path. + KJ_ASSERT(holderDestructions == 0, holderDestructions); + KJ_ASSERT(childDestructions == 0, childDestructions); + }); + }); + + // The isolate is destroyed with RefHolder's cppgc finalizer still pending. Isolate teardown + // must finalize the holder and release its unwrapped child safely. + } + + KJ_ASSERT(holderDestructions == 1, holderDestructions); + KJ_ASSERT(childDestructions == 1, childDestructions); +} + +} // namespace +} // namespace workerd::jsg::test diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 75b30fbfc1c..982f6c08a53 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -1668,7 +1668,7 @@ Ref _jsgThis(T* obj) { // use-after-free. // // - tryAddRef(js) answers "is the object still usable from JS?". It requires the isolate -// lock and returns kj::none for condemned objects (see Wrappable::isCondemned()). +// lock and returns kj::none for condemned objects (see Wrappable::wasTracedInLastGc()). // Any JS-facing work through a WeakRef must go through tryAddRef(). // // Use operator->() for convenient single-expression access that asserts liveness: @@ -1772,7 +1772,7 @@ class WeakRef { // Try to promote to a strong Ref. Returns kj::none if the target has been destroyed, // or if the target's V8 wrapper died in a major GC whose deferred cleanup has not yet - // released the target (detected via Wrappable::isCondemned(); + // released the target (detected via the GC epoch check in Wrappable::wasTracedInLastGc(); // see the implementation in setup.h). In the latter case the target is condemned and this // WeakRef is permanently invalidated. kj::Maybe> tryAddRef(Lock&) const; @@ -3115,16 +3115,13 @@ class Lock { void requestGcForTesting() const; // Like requestGcForTesting(), but leaves cppgc's sweep pending rather than running it inside the - // collection. On return, wrappers unreachable at the start of the GC have been collected and - // their Wrappables condemned (see Wrappable::isCondemned()), but the ~CppgcShim that releases - // each Wrappable has not run yet. This is the state a natural major GC leaves behind, and the - // only state in which the condemned-wrapper hazard is observable. + // collection. This reproduces the delayed finalization performed by a natural major GC. // // Pair with finishDeferredSweepForTesting() to close the window. Testing only. void requestGcWithDeferredSweepForTesting() const; // Completes a sweep left pending by requestGcWithDeferredSweepForTesting(), running the deferred - // ~CppgcShim finalizers. Testing only. + // cppgc finalizers. Testing only. void finishDeferredSweepForTesting() const; // Runs the given function synchronously with a v8::HandleScope on the stack. diff --git a/src/workerd/jsg/setup.c++ b/src/workerd/jsg/setup.c++ index 406b23b39dd..ae5e9d31006 100644 --- a/src/workerd/jsg/setup.c++ +++ b/src/workerd/jsg/setup.c++ @@ -314,7 +314,21 @@ HeapTracer::HeapTracer(v8::Isolate* isolate) // after the fact will trigger a spurious ASAN failure. self.clearFreelistedShims(); } - }, this, v8::GCType::kGCTypeMarkSweepCompact); + // Advance the GC epoch at the start of a major GC cycle, exactly once per cycle. An + // incremental cycle fires kGCTypeIncrementalMarking at marking start and then + // kGCTypeMarkSweepCompact again at the atomic pause; a non-incremental major GC fires + // only the latter. activeGcEpoch == completedGcEpoch identifies "no cycle in flight" + // (the mark-compact epilogue restores that equality), so the second prologue of the + // same cycle is a no-op. traceFromV8() stamps the active epoch into each wrapper it + // visits, and wasTracedInLastGc() compares against the last *completed* epoch — so + // objects not yet traced by an in-progress cycle are correctly treated as alive (they + // still carry the previous completed epoch). + if (self.activeGcEpoch == self.completedGcEpoch) { + ++self.activeGcEpoch; + } + }, this, + static_cast( + v8::GCType::kGCTypeMarkSweepCompact | v8::GCType::kGCTypeIncrementalMarking)); isolate->AddGCEpilogueCallback( [](v8::Isolate* isolate, v8::GCType type, v8::GCCallbackFlags flags, void* data) { @@ -323,6 +337,13 @@ HeapTracer::HeapTracer(v8::Isolate* isolate) wrappable->detachWrapper(true); } self.detachLater.clear(); + if (type == v8::GCType::kGCTypeMarkSweepCompact) { + // Promote the active epoch to completed. V8 has already zapped dead traced nodes + // (ResetDeadNodes runs during the atomic pause), so from this point — still before + // control returns to JavaScript — wasTracedInLastGc() reports false for any wrapper + // not traced during this cycle. + self.completedGcEpoch = self.activeGcEpoch; + } }, this, v8::GCType::kGCTypeAll); } @@ -335,7 +356,30 @@ HeapTracer& HeapTracer::getTracer(v8::Isolate* isolate) { return IsolateBase::from(isolate).heapTracer; } -// Note: ResetRoot() lives in wrappable.c++, where Wrappable::CppgcShim is a complete type. +void HeapTracer::ResetRoot(const v8::TracedReference& handle) { + // V8 calls this to tell us when our wrapper can be dropped. See comment about droppable + // references in Wrappable::attachWrapper() for details. + v8::HandleScope scope(isolate); + + // V8 can only hand this polymorphic callback an object that is in one of the + // sandbox-external tables, so it's genuinely one of our objects. Sandbox-internal corruption + // can still substitute another shim, so resolve through the exact wrapper identity check. + auto object = handle.As().Get(isolate); + auto& wrappable = + *Wrappable::unwrapFromShimInRangeOrAbort(isolate, object, kJsgWrappableTagRange); + auto& backReference = KJ_ASSERT_NONNULL(wrappable.wrapper); + + // V8 gets angry if we do not EXPLICITLY call `Reset()` on the wrapper. If we merely destroy it + // (which is what `detachWrapper()` will do) it is not satisfied, and will come back and try to + // visit the reference again, but it will DCHECK-fail on that second attempt because the + // reference is in an inconsistent state at that point. + backReference.Reset(); + + // We don't want to call `detachWrapper()` now because it may create new handles (specifically, + // if the wrappable has strong references, which means that its outgoing references need to be + // upgraded to strong). + detachLater.add(&wrappable); +} bool HeapTracer::TryResetRoot(const v8::TracedReference& handle) { // This method is potentially called on a separate thread. Our ResetRoot() implementation, diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index f3f71a65351..26aefca6315 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -1124,13 +1124,15 @@ template kj::Maybe> WeakRef::tryAddRef(Lock&) const { KJ_IF_SOME(i, impl) { if (!i.anchor->isAlive()) return kj::none; - // A major GC may have collected the target's wrapper while the ~CppgcShim that would - // release the target Wrappable (running ~Wrappable(), which invalidates the anchor) is - // still deferred, so the anchor keeps reporting isAlive(). Promoting a Ref in that state - // would call addStrongRef() on a doomed Wrappable. cppgc tells us directly: it cleared the - // Wrappable's weak reference to its shim during the collecting GC's atomic pause. + // After a major GC, V8's ResetDeadNodes zaps a dead droppable TracedReference without + // calling ResetRoot(). The CppgcShim destructor that would release the object (running + // ~Wrappable(), which invalidates the anchor) can be deferred past the end of the GC + // cycle, so the anchor still reports isAlive() while the TracedReference dangles. + // Promoting a Ref in that state would call addStrongRef(), which copies the dangling + // reference via TracedReference::Get() — a use-after-free. Detect it instead: a wrapper + // that exists but was not traced in the last completed major GC cycle is dead. auto& target = static_cast(i.target); - if (target.isCondemned()) { + if (!target.wasTracedInLastGc()) { // The object is condemned: its wrapper died in a completed major GC, which also means // no strong refs exist (they would have rooted the wrapper) and no live wrappable // holds a traced ref to it (that would have marked it) — anything still referencing diff --git a/src/workerd/jsg/wrappable.c++ b/src/workerd/jsg/wrappable.c++ index c3f42854e1b..5935fed2fe9 100644 --- a/src/workerd/jsg/wrappable.c++ +++ b/src/workerd/jsg/wrappable.c++ @@ -17,6 +17,9 @@ #include #include +#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__) +#include +#endif namespace workerd::jsg { @@ -49,15 +52,7 @@ void HeapTracer::clearWrappers() { // V8 operations. Without this, objects that outlive their isolate (like WebSockets // stored in a HibernationManager) would have a dangling isolate pointer and crash // when trying to check v8::Locker::IsLocked() or create V8 handles. - if (ownWrappable.get() == nullptr) { - // cppgc has cleared the weak shim but has not run its deferred destructor yet. The shim - // will release the Wrappable later, after isolate teardown has unlinked it here. - KJ_DASSERT(wrappable.isCondemned()); - wrappers.remove(wrappable); - wrappable.isolate = nullptr; - } else { - ownWrappable->isolate = nullptr; - } + ownWrappable->isolate = nullptr; } clearFreelistedShims(); } @@ -95,8 +90,8 @@ class Wrappable::CppgcShim final: public v8::Object::Wrappable { CppgcShim(JSGWrappable& wrappable, v8::CppHeapPointerTag tag) : tag(tag), state(Active{kj::addRef(wrappable)}) { - KJ_DASSERT(!wrappable.hasWrapper()); - wrappable.weakShim = this; + KJ_DASSERT(wrappable.cppgcShim == kj::none); + wrappable.cppgcShim = *this; } ~CppgcShim() { @@ -109,13 +104,9 @@ class Wrappable::CppgcShim final: public v8::Object::Wrappable { KJ_SWITCH_ONEOF(state) { KJ_CASE_ONEOF(active, Active) { - // A collected wrapper implies nothing was pinning it, i.e. strongRefcount == 0. This is - // what makes it safe for cppgc to clear `weakShim` without a GC visitation; see the - // comment on Wrappable::strongRefcount. + KJ_DASSERT(&KJ_ASSERT_NONNULL(active.wrappable->cppgcShim) == this); KJ_DASSERT(active.wrappable->strongWrapper.IsEmpty()); - // Can't go through detachWrapper(): on the major-GC path cppgc already cleared the - // Wrappable's `weakShim`, so it can no longer find us. Hand it the shim directly. - active.wrappable->detachFromShim(*this, false); + active.wrappable->detachWrapper(false); } KJ_CASE_ONEOF(freelisted, Freelisted) { KJ_DASSERT(&KJ_ASSERT_NONNULL(*freelisted.prev) == this); @@ -132,8 +123,6 @@ class Wrappable::CppgcShim final: public v8::Object::Wrappable { void Trace(cppgc::Visitor* visitor) const override { KJ_SWITCH_ONEOF(state) { KJ_CASE_ONEOF(active, Active) { - // Trace the handle this shim owns, which is what cppgc expects of a GC-managed object. - visitor->Trace(KJ_ASSERT_NONNULL(wrapper)); active.wrappable->traceFromV8(*visitor); } KJ_CASE_ONEOF(freelisted, Freelisted) { @@ -181,9 +170,12 @@ class Wrappable::CppgcShim final: public v8::Object::Wrappable { JSGWrappable& resolve(v8::Local object) { KJ_IF_SOME(active, state.tryGet()) { - // An Active shim always holds its wrapper; see the invariant on `wrapper` below. - if (KJ_ASSERT_NONNULL(wrapper) == object) { - return *active.wrappable; + KJ_IF_SOME(wrapper, active.wrappable->wrapper) { + if (wrapper == object) { + return *active.wrappable; + } + } else { + KJ_FAIL_ASSERT("active CppgcShim has no wrapper"); } } reportWrapperIdentityMismatch(); @@ -194,17 +186,6 @@ class Wrappable::CppgcShim final: public v8::Object::Wrappable { // lifetime and identifies which freelist bucket it belongs to. v8::CppHeapPointerTag tag; - // Handle to the JS wrapper this shim was Wrapped into. Non-empty exactly while `state` is - // Active; addToFreelist() asserts it was cleared, and Trace() ignores it in the other states. - // - // This lives here rather than in the Wrappable so that the handle and the shim cppgc collects - // are one and the same: once the shim dies, nothing can observe the handle, which is the - // arrangement cppgc's contract assumes. It is cleared by dropping the kj::Maybe rather than by - // Reset(), because a full GC may already have freed the underlying traced node -- - // ~TracedReference is trivial and writes nothing, whereas Reset() would touch freed memory. The - // minor-GC path is the exception and must Reset() explicitly; see HeapTracer::ResetRoot(). - kj::Maybe> wrapper; - mutable kj::OneOf state; // This is `mutable` because `Trace()` is const. We configure V8 to perform traces atomically in // the main thread so concurrency is not a concern. @@ -219,9 +200,6 @@ kj::Maybe& HeapTracer::freelistHeadFor(v8::CppHeapPoin } void HeapTracer::addToFreelist(JSGWrappable::CppgcShim& shim) { - // Trace() deliberately ignores the handle once a shim is no longer Active, so a freelisted shim - // holding one would silently drop a live edge. detachFromShim() clears it before we get here. - KJ_DASSERT(shim.wrapper == kj::none); auto& head = freelistHeadFor(shim.tag); auto& freelisted = shim.state.init(); freelisted.next = head; @@ -247,8 +225,8 @@ JSGWrappable::CppgcShim* HeapTracer::allocateShim( } KJ_DASSERT(shim.tag == tag); shim.state = JSGWrappable::CppgcShim::Active{kj::addRef(wrappable)}; - KJ_DASSERT(!wrappable.hasWrapper()); - wrappable.weakShim = &shim; + KJ_DASSERT(wrappable.cppgcShim == kj::none); + wrappable.cppgcShim = shim; return &shim; } } @@ -271,34 +249,6 @@ void HeapTracer::clearFreelistedShims() { } } -void HeapTracer::ResetRoot(const v8::TracedReference& handle) { - // V8 calls this to tell us when our wrapper can be dropped. See comment about droppable - // references in Wrappable::attachWrapper() for details. - v8::HandleScope scope(isolate); - - // V8 can only hand this polymorphic callback a wrapper that is in one of the sandbox-external - // tables, so it is genuinely one of ours. Sandbox-internal corruption can still substitute - // another shim, so resolve through the exact wrapper identity check. - auto object = handle.As().Get(isolate); - auto& wrappable = - *JSGWrappable::unwrapFromShimInRangeOrAbort(isolate, object, kJsgWrappableTagRange); - auto& shim = KJ_ASSERT_NONNULL(wrappable.getShim()); - - // V8 gets angry if we do not EXPLICITLY call `Reset()` on the wrapper. If we merely destroy it - // (which is what `detachWrapper()` will do) it is not satisfied, and will come back and try to - // visit the reference again, but it will DCHECK-fail on that second attempt because the - // reference is in an inconsistent state at that point. - // - // Unlike the major-GC path, the node is definitely still live here: V8 is telling us it is - // dropping a droppable root it has decided to reclaim, not reporting one it already zapped. - KJ_ASSERT_NONNULL(shim.wrapper).Reset(); - - // We don't want to call `detachWrapper()` now because it may create new handles (specifically, - // if the wrappable has strong references, which means that its outgoing references need to be - // upgraded to strong). - detachLater.add(&wrappable); -} - void HeapTracer::jsgGetMemoryInfo(jsg::MemoryTracker& tracker) const { for (const auto& wrapper: wrappers) { tracker.trackField("wrapper", wrapper); @@ -374,65 +324,58 @@ Wrappable* Wrappable::unwrapFromShimInRangeOrAbort( abort(); } -kj::Maybe JSGWrappable::getShim() const { - auto* shim = weakShim.Get(); - if (shim == nullptr) return kj::none; - // Only CppgcShim instances are ever stored in `weakShim`. - return *static_cast(shim); -} - -kj::Maybe> JSGWrappable::tryGetHandle(v8::Isolate* isolate) { - KJ_IF_SOME(shim, getShim()) { - return KJ_ASSERT_NONNULL(shim.wrapper).Get(isolate); - } - return kj::none; -} - kj::Own JSGWrappable::detachWrapper(bool shouldFreelistShim) { - KJ_IF_SOME(shim, getShim()) { - return detachFromShim(shim, shouldFreelistShim); - } else { - return {}; - } -} - -kj::Own JSGWrappable::detachFromShim( - JSGWrappable::CppgcShim& shim, bool shouldFreelistShim) { - auto result = - kj::mv(KJ_ASSERT_NONNULL(shim.state.tryGet()).wrappable); - // Drop the handle without Reset()ing it: the traced node may already have been freed by a - // full GC. See the comment on CppgcShim::wrapper. - shim.wrapper = kj::none; - if (shouldFreelistShim) { - KJ_ASSERT(isolate != nullptr); - HeapTracer::getTracer(isolate).addToFreelist(shim); - } else { - shim.state = JSGWrappable::CppgcShim::Dead{}; - } - // Already null when cppgc cleared it for us on the major-GC path; a no-op then. - weakShim.Clear(); - strongWrapper.Reset(); - // Note: weak refs are deliberately NOT invalidated here. Detaching the wrapper does not - // imply the Wrappable is dying: this method also runs when V8 drops an unmodified droppable - // wrapper via ResetRoot() (the Wrappable stays alive through C++ refs and the wrapper is - // recreated on demand the next time it is passed to JS) and at isolate shutdown via - // clearWrappers() (Wrappables like hibernatable WebSockets outlive the isolate). A - // jsg::WeakRef tracks the Wrappable's lifetime, not the wrapper's; invalidation happens in - // ~Wrappable(), or eagerly in WeakRef::tryAddRef() when isCondemned() proves the Wrappable - // is doomed. - if (isolate != nullptr) { - HeapTracer::getTracer(isolate).removeWrapper({}, *this); + KJ_IF_SOME(shim, cppgcShim) { +#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__) + // There's a possibility that the CppgcShim has already been found to be unreachable by a GC + // pass, but has not actually been destroyed yet. For some reason, cppgc likes to delay the + // calling of actual destructors. However, in ASAN builds, cppgc will poison the memory in the + // meantime, because it figures that we "shouldn't" be accessing unreachable memory. This + // assumption makes sense in the abstract, but not for our specific use case, where we are + // essentially maintaining a weak pointer to the CppgcShim. If the destructor had been called, + // then `cppgcShim` here would have been nulled out at that time. We're expecting that until + // the destructor is called, we can still safely access the object to detach the wrapper. + // + // So to work around cppgc's incorrect assumption, we manually unpoison the memory. + // + // Note: An alternative strategy could have been for CppgcShim itself to allocate a separate + // C++ heap object to store its own state in, so that that state could be modified even while + // the CppgcShim object itself is poisoned. In this case `Wrappable::cppgcShim` would change to + // point at this state object, not to the `CppgcShim` itself. However, this approach would + // require extra heap allocation for everyone, just to satisfy ASAN, which seems undesirable. + ASAN_UNPOISON_MEMORY_REGION(&shim, sizeof(shim)); +#endif + + auto& tracer = HeapTracer::getTracer(isolate); + auto result = + kj::mv(KJ_ASSERT_NONNULL(shim.state.tryGet()).wrappable); + if (shouldFreelistShim) { + tracer.addToFreelist(shim); + } else { + shim.state = JSGWrappable::CppgcShim::Dead{}; + } + wrapper = kj::none; + cppgcShim = kj::none; + strongWrapper.Reset(); + // Note: weak refs are deliberately NOT invalidated here. Detaching the wrapper does not + // imply the object is dying: this method also runs when V8 drops an unmodified droppable + // wrapper via ResetRoot() (the object stays alive through C++ refs and the wrapper is + // recreated on demand the next time it is passed to JS) and at isolate shutdown via + // clearWrappers() (objects like hibernatable WebSockets outlive the isolate). A + // jsg::WeakRef tracks the object's lifetime, not the wrapper's; invalidation happens in + // ~Wrappable(), or eagerly in WeakRef::tryAddRef() when a zapped wrapper proves the + // object is condemned. + tracer.removeWrapper({}, *this); if (strongRefcount > 0) { // Need to visit child references in order to convert them to strong references, since we // no longer have an intervening wrapper. GcVisitor visitor(*this, kj::none); jsgVisitForGc(visitor); } + return result; } else { - KJ_DASSERT(!link.isLinked()); - KJ_DASSERT(strongRefcount == 0); + return {}; } - return result; } v8::Local Wrappable::getHandle(v8::Isolate* isolate) { @@ -446,10 +389,10 @@ void Wrappable::addStrongRef() { "referencing wrapper without isolate lock"); if (strongRefcount++ == 0) { // This object previously had no strong references, but now it has one. - KJ_IF_SOME(shim, getShim()) { + KJ_IF_SOME(w, wrapper) { // Copy the traced reference into the strong reference. v8::HandleScope scope(isolate); - strongWrapper.Reset(isolate, KJ_ASSERT_NONNULL(shim.wrapper).Get(isolate)); + strongWrapper.Reset(isolate, w.Get(isolate)); } else { // Since we have no JS wrapper, we're forced to recursively mark all references reachable // through this wrapper as strong. @@ -463,7 +406,7 @@ void Wrappable::removeStrongRef() { "destroying wrapper without isolate lock"); if (--strongRefcount == 0) { // This was the last strong reference. - if (!hasWrapper()) { + if (wrapper == kj::none) { // We have no wrapper. We need to mark all references held by this object as weak. if (isolate != nullptr) { // But only if the current isolate isn't null. If strong ref count is zero, @@ -497,7 +440,8 @@ void Wrappable::maybeDeferDestruction(bool strong, kj::Own ownSelf, Wrappa } void Wrappable::traceFromV8(cppgc::Visitor& cppgcVisitor) { - // The wrapper handle is traced by our CppgcShim, which owns it. + tracedEpoch.store(HeapTracer::getTracer(isolate).getActiveGcEpoch(), std::memory_order_relaxed); + cppgcVisitor.Trace(KJ_ASSERT_NONNULL(wrapper)); GcVisitor visitor(*this, cppgcVisitor); jsgVisitForGc(visitor); } @@ -508,7 +452,7 @@ void Wrappable::attachWrapper(v8::Isolate* isolate, v8::CppHeapPointerTag tag) { auto& tracer = HeapTracer::getTracer(isolate); - KJ_REQUIRE(!hasWrapper()); + KJ_REQUIRE(wrapper == kj::none); KJ_REQUIRE(strongWrapper.IsEmpty()); // The C++ Wrappable object must hold a TracedReference to its own JavaScript wrapper, while @@ -538,7 +482,19 @@ void Wrappable::attachWrapper(v8::Isolate* isolate, // our wrapper and recreated it, the property would be gone. Luckily, V8 already handles this // for us! V8 knows not to drop our wrapper if the application has done anything with it such // that a recreated wrapper would no longer be equivalent. + wrapper.emplace(isolate, object, v8::TracedReference::IsDroppable()); this->isolate = isolate; + // Stamp the last *completed* GC epoch so that wasTracedInLastGc() returns true for newly + // attached wrappers (otherwise a wrapper attached after any completed major GC would read as + // dead). Deliberately NOT the active epoch: creating a TracedReference is an initializing + // store, which V8 does not black-allocate, so a wrapper attached during an in-flight marking + // cycle whose JS object dies before the atomic pause is zapped by that same cycle. Stamping + // the completed epoch keeps such wrappers detectable as dead; if the wrapper instead survives + // the in-flight cycle, traceFromV8() re-stamps it with the active epoch during the pause. + tracedEpoch.store(tracer.getCompletedGcEpoch(), std::memory_order_relaxed); + + // Add to list of objects to force-clean at isolate shutdown. + tracer.addWrapper({}, *this); // Set up internal fields for a newly-allocated object. KJ_REQUIRE(object->InternalFieldCount() == Wrappable::INTERNAL_FIELD_COUNT); @@ -554,14 +510,8 @@ void Wrappable::attachWrapper(v8::Isolate* isolate, // never be driven by different pointers, closing the wrapper type-confusion / use-after-free // class of bugs. The shim carries the same tag so it lands in the matching freelist bucket. auto* shim = tracer.allocateShim(*this, tag); - shim->wrapper.emplace(isolate, object, v8::TracedReference::IsDroppable()); v8::Object::Wrap(isolate, object, shim, tag); - // Add to the list of Wrappables to force-clean at isolate shutdown. Done after the shim - // exists, so this Wrappable is never briefly linked-but-wrapperless, which is the state - // isCondemned() looks for. - tracer.addWrapper({}, *this); - if (strongRefcount > 0) { strongWrapper.Reset(isolate, object); @@ -575,7 +525,7 @@ void Wrappable::attachWrapper(v8::Isolate* isolate, } void Wrappable::jsgGetMemoryInfo(jsg::MemoryTracker& tracker) const { - tracker.trackField("cppgcshim", getShim()); + tracker.trackField("cppgcshim", cppgcShim); } v8::Local Wrappable::attachOpaqueWrapper( @@ -640,7 +590,7 @@ void Wrappable::visitRef(GcVisitor& visitor, kj::Maybe& refParent, b } // Make ref strength match the parent. - if (visitor.parent.strongRefcount > 0 && !visitor.parent.hasWrapper()) { + if (visitor.parent.strongRefcount > 0 && visitor.parent.wrapper == kj::none) { // This reference should be strong, because the parent has strong refs and does not have its // own wrapper that will be traced. @@ -668,13 +618,8 @@ void Wrappable::visitRef(GcVisitor& visitor, kj::Maybe& refParent, b KJ_IF_SOME(cgv, visitor.cppgcVisitor) { // We're visiting for the purpose of a GC trace. - KJ_IF_SOME(shim, getShim()) { - // Reaching the handle through the weak persistent is safe inside a trace: cppgc clears - // weak persistents only in MarkerBase::ProcessWeakness(), at the end of the atomic pause - // and after every Trace() callback has run. So a live wrapper is always visible here, and - // a null shim means this Wrappable was condemned by an earlier GC -- handled below, since - // a condemned Wrappable must be traced through transitively just like an unwrapped one. - cgv.Trace(KJ_ASSERT_NONNULL(shim.wrapper)); + KJ_IF_SOME(w, wrapper) { + cgv.Trace(w); } else { // This object doesn't currently have a wrapper, so traces must transitively trace through // it. However, as an optimization, we can skip the trace if we've already been traced in @@ -688,7 +633,7 @@ void Wrappable::visitRef(GcVisitor& visitor, kj::Maybe& refParent, b void GcVisitor::visit(Data& value) { if (!value.handle.IsEmpty()) { // Make ref strength match the parent. - if (parent.strongRefcount > 0 && !parent.hasWrapper()) { + if (parent.strongRefcount > 0 && parent.wrapper == kj::none) { // This is directly reachable by a strong ref, so mark the handle strong. if (value.tracedHandle != kj::none) { // Convert the handle back to strong and discard the traced reference. @@ -729,7 +674,7 @@ void GcVisitor::visit(v8::Global& strong, v8::TracedReference 0 && !parent.hasWrapper()) { + if (parent.strongRefcount > 0 && parent.wrapper == kj::none) { // Parent has strong Rust refs and no JS wrapper — keep handle strong, // discard any traced ref. if (!traced.IsEmpty()) { diff --git a/src/workerd/jsg/wrappable.h b/src/workerd/jsg/wrappable.h index 6b9a39c77be..3d78c4eda0d 100644 --- a/src/workerd/jsg/wrappable.h +++ b/src/workerd/jsg/wrappable.h @@ -10,7 +10,6 @@ #include -#include #include #include #include @@ -21,6 +20,7 @@ #include #include +#include #include // Niche value optimization for v8::TracedReference. This teaches kj::Maybe to use @@ -180,35 +180,22 @@ class Wrappable: public kj::Refcounted { &WORKERD_WRAPPABLE_TAG; } - // Does a live JS wrapper currently exist for this Wrappable? - // - // False both before the first attachWrapper() and after detachWrapper(), and also during the - // window in which a major GC has collected the wrapper but the deferred ~CppgcShim has not run - // yet -- see isCondemned(). - bool hasWrapper() const { - return weakShim.Get() != nullptr; - } - - // True when a completed major GC collected this Wrappable's wrapper and cleared `weakShim`, - // but the ~CppgcShim that will release this Wrappable has not run yet. The Wrappable is still - // alive and its weak-ref anchor still reports alive, yet it is doomed: the shim holds the last - // reference to it, and the shim's destructor is only deferred, not cancelled. - // - // A Wrappable is a member of HeapTracer::wrappers exactly between attachWrapper() and - // detachWrapper(), so being linked while having no wrapper isolates precisely the case where - // cppgc nulled `weakShim` behind our back. That link test is load-bearing, because a null - // `weakShim` has two producers: cppgc on major GC (this case), and detachWrapper() itself -- - // including the minor-GC path, where V8 drops a droppable wrapper while the Wrappable lives - // on. - bool isCondemned() const { - return link.isLinked() && !hasWrapper(); - } - - // Invalidate all outstanding jsg::WeakRefs pointing at this Wrappable. Called lazily from - // WeakRef::tryAddRef() when a condemned Wrappable is detected; ~Wrappable() performs the same - // invalidation for ordinary destruction. Deliberately NOT called from detachWrapper(): that - // also runs when V8 drops an unmodified droppable wrapper via ResetRoot() and at isolate - // shutdown, while the Wrappable remains alive and usable — a WeakRef tracks Wrappable + // Returns true if this object's V8 wrapper was traced by GC during the most recent + // *completed* major GC cycle, or if no wrapper (i.e. no v8::TracedReference) currently + // exists. A false return means the TracedReference was zapped by V8's ResetDeadNodes and + // is no longer safe to dereference, even though the C++ object (and its weak-ref anchor) + // may still be alive: V8 zaps dead droppable traced nodes during a full GC without calling + // ResetRoot(), and the CppgcShim destructor that would release the object can be deferred + // past the end of the cycle. Comparing against the completed epoch (not the in-flight + // epoch) ensures that objects not yet traced during an in-progress incremental marking + // cycle are not falsely reported as dead. + inline bool wasTracedInLastGc() const; + + // Invalidate all outstanding jsg::WeakRefs pointing at this object. Called lazily from + // WeakRef::tryAddRef() when a zapped wrapper is detected; ~Wrappable() performs the same + // invalidation for ordinary destruction. Deliberately NOT called from detachWrapper(): + // that also runs when V8 drops an unmodified droppable wrapper via ResetRoot() and at + // isolate shutdown, while the object remains alive and usable — a WeakRef tracks object // lifetime, not wrapper lifetime. void invalidateWeakRefs() { KJ_IF_SOME(a, weakRefAnchor) { @@ -216,9 +203,11 @@ class Wrappable: public kj::Refcounted { } } - // Act on a condemned Wrappable (see isCondemned()): invalidate outstanding weak refs and bump - // the isolate's condemned counter (see HeapTracer::getCondemnedWrapperCount()). Called from - // WeakRef::tryAddRef(), which must refuse to promote such a Wrappable. + // Mark this object as condemned: its wrapper's TracedReference was zapped by a completed + // major GC, but the CppgcShim destructor that will release the object has not run yet. + // Invalidates outstanding weak refs and bumps the isolate's condemned counter (see + // HeapTracer::getCondemnedWrapperCount()). Called from WeakRef::tryAddRef() on the sole + // path that can observe the condition. inline void condemn(); void addStrongRef(); @@ -234,10 +223,9 @@ class Wrappable: public kj::Refcounted { v8::Local getHandle(v8::Isolate* isolate); - // This Wrappable's JS wrapper, or null if none currently exists. Reaches the handle through - // `weakShim`, so a wrapper the GC has already collected reads as absent rather than as a - // zapped handle. - kj::Maybe> tryGetHandle(v8::Isolate* isolate); + kj::Maybe> tryGetHandle(v8::Isolate* isolate) { + return wrapper.map([&](v8::TracedReference& ref) { return ref.Get(isolate); }); + } // Visits a Ref pointing at this Wrappable. `refParent` and `refStrong` are the members of // `Ref`, and this method is invoked on the object the ref points at. (This avoids the need @@ -347,68 +335,55 @@ class Wrappable: public kj::Refcounted { private: class CppgcShim; - // The shim owning this Wrappable's wrapper handle, or null when no live wrapper exists. - // Downcasts `weakShim`; see its comment for why that is stored as the cppgc base type. - kj::Maybe getShim() const; - - // The body of detachWrapper(), taking the shim explicitly. ~CppgcShim must use this: on the - // major-GC path cppgc has already cleared `weakShim`, so getShim() would find nothing. - kj::Own detachFromShim(CppgcShim& shim, bool shouldFreelistShim); + // If a JS wrapper is currently allocated, this point to the cppgc shim object. + kj::Maybe cppgcShim; - // The cppgc shim owning this Wrappable's JS wrapper, or null when no live wrapper exists. - // - // The wrapper handle itself lives in the shim, not here. The wrapper is created lazily when the - // Wrappable is first exported to JavaScript; until then this is null. - // - // If the wrapper is "unmodified" from its original creation state, then V8 may choose to - // collect it even when the Wrappable could still technically be reached from C++. The idea here - // is that if the Wrappable is returned to JavaScript again later, the wrapper can be - // reconstructed at that time. However, if the wrapper is modified by the application (e.g. - // monkey-patched with a new property), then collecting and recreating it won't work. The logic - // to decide if a wrapper has been "modified" is internal to V8 and baked into its use of - // EmbedderRootsHandler. - // - // The reference is weak so that the GC, not us, decides when it goes away. When a major GC - // collects the wrapper, cppgc nulls this in MarkerBase::ProcessWeakness() -- during the same - // atomic pause that zaps the wrapper's traced node, and long before the deferred ~CppgcShim - // runs. Every reader therefore observes "no wrapper" as soon as the GC knows it, instead of - // reaching a zapped handle through a bare pointer. + // Handle to the JS wrapper object. The wrapper is created lazily when the object is first + // exported to JavaScript; until then, the wrapper is empty. // - // Declared as the cppgc base type because CppgcShim is defined in wrappable.c++ while - // cppgc::WeakPersistent needs a complete type. Only CppgcShim instances are ever stored here; - // getShim() does the downcast. - cppgc::WeakPersistent weakShim; - - // Whenever there are non-GC-traced references to this Wrappable (i.e. from other C++ objects, - // i.e. strongRefcount > 0) and a wrapper exists, `strongWrapper` contains a copy of the wrapper - // handle, to force the wrapper to stay alive. Otherwise, `strongWrapper` is empty. + // If the wrapper object is "unmodified" from its original creation state, then V8 may choose to + // collect it even when it could still technically be reached via C++ objects. The idea here is + // that if the object is returned to JavaScript again later, the wrapper can be reconstructed at + // that time. However, if the wrapper is modified by the application (e.g. monkey-patched with + // a new property), then collecting and recreating it won't work. The logic to decide if an + // object has been "modified" is internal to V8 and baked into its use of EmbedderRootsHandler. + kj::Maybe> wrapper; + + // Whenever there are non-GC-traced references to the object (i.e. from other C++ objects, i.e. + // strongRefcount > 0), and `wrapper` is non-null, then `strongWrapper` contains a copy of + // `wrapper`, to force it to stay alive. Otherwise, `strongWrapper` is empty. v8::Global strongWrapper; - // Will be non-null if a wrapper has ever been attached. + // Will be non-null if `wrapper` has ever been non-null. v8::Isolate* isolate = nullptr; - // How many strong Refs point at this Wrappable, forcing its wrapper to stay alive even if - // GC tracing doesn't find it? + // How many strong Refs point at this object, forcing the wrapper to stay alive even if GC + // tracing doesn't find it? // - // Whenever the value of the boolean expression (strongRefcount > 0 && !hasWrapper()) changes, a - // GC visitation is needed to update all outgoing refs. The four places that can change it -- - // attachWrapper(), detachWrapper(), addStrongRef() and removeStrongRef() -- each run one. - // - // cppgc clearing `weakShim` behind our back does not count, and needs no visitation: clearing - // requires the wrapper to have been collected, which requires `strongWrapper` to be empty (see - // the assert in ~CppgcShim), which means strongRefcount is 0 -- so the expression is false both - // before and after. In other words, a wrapper can only be condemned while strongRefcount == 0. + // Whenever the value of the boolean expression (strongRefcount > 0 && wrapper.IsEmpty()) changes, + // a GC visitation is needed to update all outgoing refs. uint strongRefcount = 0; - // While a wrapper is attached, the Wrappable is a member of the list `HeapTracer::wrappers`. + // When `wrapperRef` is non-empty, the Wrappable is a member of the list `HeapTracer::wrappers`. kj::ListLink link; - // Lazy-allocated shared state for jsg::WeakRef. Zero overhead for Wrappables that never + // Stamped with the active GC epoch in traceFromV8() each time V8 traces this wrapper, and + // with the completed epoch in attachWrapper(). wasTracedInLastGc() compares this against + // the last *completed* GC epoch to detect wrappers whose TracedReference was zapped by a + // full GC (see that method's comment). + // + // The CppHeap is configured with atomic marking (see newCppHeap() in setup.c++), so + // traceFromV8() only runs on the main thread during the atomic pause and no concurrent + // access occurs today; the atomic is defensive hardening in case that configuration ever + // changes. Relaxed ordering suffices: reads happen under the isolate lock on the same + // thread that runs the GC callbacks. + std::atomic tracedEpoch{0}; + + // Lazy-allocated shared state for jsg::WeakRef. Zero overhead for objects that never // have weak references taken. Created on first call to getOrCreateWeakRefAnchor(). kj::Maybe> weakRefAnchor; - // Returns (or creates) the shared WeakRefAnchor for this Wrappable. Used by - // Ref::getWeakRef(). + // Returns (or creates) the shared WeakRefAnchor for this object. Used by Ref::getWeakRef(). kj::Rc getOrCreateWeakRefAnchor() { KJ_IF_SOME(a, weakRefAnchor) { return a.addRef(); @@ -466,8 +441,25 @@ class HeapTracer: public v8::EmbedderRootsHandler { kj::Maybe& freelistHeadFor(v8::CppHeapPointerTag tag); public: + // The epoch of the currently active (possibly in-flight) major GC cycle. Advanced once + // per cycle in whichever prologue fires first (incremental-marking start or the + // mark-compact prologue). Wrappable::traceFromV8() stamps this value into + // Wrappable::tracedEpoch. + uint64_t getActiveGcEpoch() const { + return activeGcEpoch; + } + + // The epoch of the last fully completed major GC cycle. Catches up to activeGcEpoch in + // the mark-compact epilogue, which runs after ResetDeadNodes() has zapped dead traced + // nodes but before control returns to JavaScript. Wrappable::wasTracedInLastGc() compares + // against this value, so objects not yet traced in an in-flight cycle are not falsely + // reported as dead. + uint64_t getCompletedGcEpoch() const { + return completedGcEpoch; + } + // Number of times WeakRef::tryAddRef() has detected a condemned target in this isolate, i.e. - // the number of times the dangling-wrapper hazard has actually been caught rather + // the number of times the dangling-TracedReference hazard has actually been caught rather // than merely guarded against. // // This exists so that the regression test can assert it reached the hazard. The window is @@ -526,6 +518,14 @@ class HeapTracer: public v8::EmbedderRootsHandler { // static_assert in TypeWrapper::wrappableTag(). kj::Maybe freelistedShimsByTag[kMaxWrappableTags] = {}; + // Major GC epoch counters; see getActiveGcEpoch()/getCompletedGcEpoch(). The two are equal + // exactly when no major cycle is in flight (the mark-compact epilogue restores equality), + // which is how the prologue advances the epoch exactly once per cycle: an incremental cycle + // fires prologues both at incremental-marking start and again at the atomic pause, and only + // the first of those observes equality. + uint64_t activeGcEpoch = 0; + uint64_t completedGcEpoch = 0; + // See getCondemnedWrapperCount(). Plain (non-atomic) because it is only ever touched from // Wrappable::condemn(), which runs under the isolate lock. uint64_t condemnedWrapperCount = 0; @@ -534,13 +534,31 @@ class HeapTracer: public v8::EmbedderRootsHandler { }; inline void Wrappable::condemn() { - // Only reachable from isCondemned() returning true, which implies this Wrappable is still - // linked into HeapTracer::wrappers, which implies attachWrapper() set `isolate`. + // Only reachable from wasTracedInLastGc() returning false, which implies a wrapper exists, + // which implies attachWrapper() set `isolate`. KJ_DASSERT(isolate != nullptr); ++HeapTracer::getTracer(isolate).condemnedWrapperCount; invalidateWeakRefs(); } +inline bool Wrappable::wasTracedInLastGc() const { + // The hazard being detected is a dangling v8::TracedReference, so the check applies only + // when one exists. `wrapper` reads as none both when no wrapper was ever attached and after + // detachWrapper() (including when V8 drops an unmodified droppable wrapper via ResetRoot() + // while the object stays alive); in those states there is nothing to zap, and code paths + // that would touch the wrapper (e.g. addStrongRef()) already handle its absence. Note that + // `isolate` cannot be used to detect "never wrapped": GC visitation propagates it to + // wrapper-less children (see Wrappable::visitRef()). + // + // Reading the Maybe is safe even when the TracedReference dangles: its emptiness is a + // property of the local handle, not of the (possibly freed) node it points at. + if (wrapper == kj::none) return true; + // `wrapper` is only ever set in attachWrapper(), which also sets `isolate`. + KJ_DASSERT(isolate != nullptr); + return tracedEpoch.load(std::memory_order_relaxed) >= + HeapTracer::getTracer(isolate).getCompletedGcEpoch(); +} + void substituteCppgcShimForTest( v8::Isolate* isolate, v8::Local target, v8::Local source); void detachWrapperForTest(v8::Isolate* isolate, v8::Local object);