I see that ReadableImpl::enqueue at line 1127 also has a JSG_REQUIRE(canCloseOrEnqueue(), ...). So without the new check in ReadableStreamDefaultController::enqueue, the code would reach impl.enqueue and then JSG_REQUIRE would throw a TypeError. This wouldn't be a crash, but the additional check at the outer level is cleaner and avoids the unexpected throw.

Now let me think about whether there are any issues with the PR:

1. auto self = JSG_THIS in ReadableStreamDefaultController::enqueue (line 2286): Correct. Keeps this alive through the size callback. The kj::mv(self) on line 2304 passes it to impl.enqueue which needs it.

2. auto readableControllerRef = kj::addRef(readableController) (line 4349): This uses kj::addRef which returns kj::Own<T>. This is a jsg::Object — using kj::Own directly rather than jsg::Ref is fine for a stack-local keep-alive guard. Both increment the ref count on Wrappable. However, jsg::Ref<T> is the conventional way to hold references to jsg::Object subclasses. Let me check if kj::Own<T> is actually equivalent here.

Looking at _jsgThis at line 1395-1397: Ref<T>(kj::addRef(*obj)) -- so jsg::Ref<T> wraps a kj::Own<T>. Using kj::addRef directly is equivalent from a lifetime perspective. The difference is that jsg::Ref<T> could participate in GC tracing via visitForGc, but since this is a local variable, that's not relevant. This is fine.

3. !errored && impl.canCloseOrEnqueue() check (line 2303): Correct. The size callback can error the controller through a side-channel (calling transformController.error()) without throwing. In that case, errored is still false (the tryCatch didn't catch anything), but the impl state has transitioned to Errored. The added check prevents enqueueing into an errored controller.

4. auto self = JSG_THIS in ReadableByteStreamController::enqueue (line 2564): This is a "defensive" change as mentioned in the PR title. It addresses C++ argument evaluation order (unspecified) where JSG_THIS as a function argument to impl.enqueue might not be evaluated before chunk.detach(js). By capturing it upfront, the order is guaranteed. This is a legitimate fix for a potential issue.

5. Early return at line 4360-4362: Correct. After the size callback errors the controller, hasBackpressure() and the backpressure update logic shouldn't run.

6. WPT expected failure removals: Two expected failures are removed matching the two cases now fixed. The remaining expected failures are for different scenarios (close, cancel, read inside size).

7. Test coverage: Two regression tests added, one where size callback calls error() and one where it also throws. Both good.

The changes look solid. This is a well-targeted fix for a use-after-free vulnerability in the streams implementation, with good tests and proper WPT conformance improvements. I don't see any actionable issues.

LGTM

github run

Merging this PR will degrade performance by 28.25%

❌ 1 regressed benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

_{Comparing erikcorry/size-callback (d9f6586) with main (4e71c55)}