Fix early GC collection of Reader

[erikcorry]

Adds a refcounted reference to the Reader object to stop it going away too early.

This bug was detected with a gc-stress mode on a test in streams-test.ts:
"ReadableStream start should be able to return a promise and reject it"

The test is as follows:

promise_test(() => {
  const theError = new Error('rejected!');
  const rs = new ReadableStream({
    start() {
      return delay(1).then(() => { throw theError; });
    }
  });
  return rs.getReader().closed.then(() => {
    assert_unreached('closed promise should be rejected');
  }, e => {
    assert_equals(e, theError, 'promise should be rejected with the same error');
  });
}, 'ReadableStream start should be able to return a promise and reject it');

The root cause is that the Reader has a reference to the Stream, but the Stream has no reference to the Reader. If the user does not keep a reference to the Reader, it can be collected early, which causes a destructor to remove promises from the queue and never resolve them.

When the Reader is GC'd, the following cascade occurs:

1. V8 GC collects the Reader's JS wrapper (unreachable from JS roots)

2. CppGC collects the shim → detachWrapper() on the Reader's Wrappable

3. Reader C++ object is destroyed:

   • ReaderImpl::~ReaderImpl() destroys closedPromise (the C++ handle; the JS promise object survives, held by the .then() chain)
   • ReaderImpl::~ReaderImpl() destroys state, which is Attached
   • Attached::~Attached() destroys jsg::Ref<ReadableStream> — the last ref to the stream (the rs local variable went out of scope when the test function returned)

4. ReadableStream is destroyed:

   • ReadableStreamJsController (member) is destroyed
   • kj::Own<ValueReadable> state is destroyed
   • ValueReadable::~ValueReadable() destroys its Consumer
   • ~ConsumerImpl() calls queue.removeConsumer(*this) (queue.h:390) — removes itself from the Controller's queue
   • ReadableLockImpl (the lock member) is destroyed
   • ReaderLocked::~ReaderLocked() calls reader.detach() — but the Reader is already destroyed, so this is a use-after-free on the raw Reader& (common.h:815). In practice this likely doesn't crash because the memory hasn't been reused yet.
   • ReaderLocked::~ReaderLocked() then implicitly destroys closedFulfiller (the Resolver) without ever resolving or rejecting it

5. Later: delay(1) fires → start() promise rejects → onFailure lambda fires → ReadableImpl::doError() on the Controller (which is alive, held by the lambda's Ref<ReadableStreamDefaultController>) → queue.error() → iterates consumers → no consumers left (removed in step 4) → onConsumerError is never called → ReadableStreamJsController::doError is never called → lock.onError() is never called → closedFulfiller was already destroyed

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This adds a kj::Own<void> ref-counted reference from ReaderLocked back to the Reader to prevent premature GC collection, and removes the unsafe detach() call from the ReaderLocked destructor.

1. [MEDIUM] WriterLocked has the same vulnerability. WriterLocked::~WriterLocked() (common.h:906-910) calls w.detach() on a bare Writer& reference, which is the exact same use-after-free pattern that this PR fixes in ReaderLocked. WriterImpl::Attached holds a jsg::Ref<WritableStream>, so ws.getWriter().closed.then(...) creates the same unrooted-temporary hazard. Consider applying the same fix to WriterLocked in this PR, or at minimum filing a follow-up.

---

This review was generated by an AI code review assistant and may contain inaccuracies.

[ask-bonk]

I've posted the review on PR #6661. Here's a summary of my analysis:

The fix itself is correct and well-designed. The approach of using kj::Own<void> (via kj::addRef) rather than jsg::Ref<T> is the right choice -- it keeps the C++ Reader object alive without pinning the JS wrapper, and the cycle-breaking logic in visitForGc (releasing readerRef_ once closedFulfiller is consumed) is sound. I verified all paths that settle the closedFulfiller and confirmed they all call releaseReaderRef() or clear().

One medium-severity finding: WriterLocked has the exact same vulnerability -- its destructor calls w.detach() on a bare Writer& that could be destroyed during a GC cascade. I flagged this in the review for either same-PR fix or follow-up.

github run

[erikcorry]

Looks like this causes a leak, caught by the internal build. Trying to see how to fix it.