Revert "Move wrapper handle to shim" and add a regression test

This reverts commit 2d08fd1.

Author: ObsidianMinor

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<Wrappable>
-   - 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 Ref<T>s exist (one would root the wrapper)
+   - No strong Ref<T>s 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

src/workerd/jsg/condemned-wrapper-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<Child>
+// 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): child(kj::mv(child)) {}
+
+  ~RefHolder() noexcept(false) {
+    ++holderDestructions;
+  }
+
+  void visitForGc(GcVisitor& visitor) {
+    visitor.visit(child);
+  }
+
+  JSG_RESOURCE_TYPE(RefHolder) {}
+
+ private:
+  Ref<Child> child;
+};
+
+struct ShutdownContext: public ContextGlobalObject {
+  Ref<RefHolder> makeHolder(Lock& js) {
+    return js.alloc<RefHolder>(js.alloc<Child>());
+  }
+
+  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<IsolateObserver>());
+    isolate.runInLockScope([&](ShutdownIsolate::Lock& lock) {
+      JSG_WITHIN_CONTEXT_SCOPE(
+          lock, lock.newContext<ShutdownContext>().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

src/workerd/jsg/isolate-shutdown-test.c++

@@ -1668,7 +1668,7 @@ Ref<T> _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<T>. 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<Ref<T>> 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.