CompressionStream: register a chunk's promise before its first codec step - #38970
CompressionStream: register a chunk's promise before its first codec step#38970robobun wants to merge 1 commit into
Conversation
…step Delivering a step can run user code: enqueueing the first piece resolves a pending read(), and resolving a promise with an object looks up its then. An abort or cancel issued from there found no promise to abandon, and the chunk was then parked in a state nothing would settle, so write() and abort() hung. Creating the promise up front makes the first call take the same path as a continuation and removes the separate settle switch.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: closed without merging. The only way to reach the hang this fixed is an |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks re-entrant promise/state lifecycle in the native streams layer, a maintainer familiar with #38695's stepping protocol should still give it a look.
What was reviewed:
- Traced Done/Failed/DoneSinkFull/Pending through
settlePendingChunk— each settles the pre-created promise the same way the old per-case switch did. - Checked that
takeCodecPromise→nativeTransformReleaseStateIfIdleinsidetransformChunkis safe:runNativeArmholdsm_nativeStateInUse, so the release defers to its epilogue. - Confirmed the abandoned-during-first-step path:
nativeCodecAbandonresolves and clearsm_codecPromise,stepChunkHerereturnsPending, andtransformChunkreturns the already-resolved promise. - Test restores
Object.prototype.thenin both the getter and afinally, and asserts the getter fired.
Extended reasoning...
Overview
This PR touches one C++ file (src/jsc/bindings/webcore/streams/JSCompressionStreamShared.cpp, +9/−22) and adds one test to test/js/web/streams/compression.test.ts. It is a follow-up to #38695: transformChunk now creates and stores m_codecPromise before the first codec step (previously only after stepChunkHere returned Pending), so a re-entrant nativeCodecAbandon reached from inside the first step's delivery finds a promise to settle. The per-case switch that used to create a fresh promise per outcome is replaced by the existing settlePendingChunk, unifying the first-call path with the continuation path. stepChunkHere drops its continuation flag since m_codecPromise is now always set at entry.
Security risks
None identified. The scenario requires a poisoned Object.prototype.then getter to reach; it's a hang, not a memory-safety issue. No untrusted input parsing, allocation sizing, or privilege boundary is involved.
Level of scrutiny
High. This is native JSC bindings code that manipulates a WriteBarrier<JSPromise> and interacts with re-entrant user JS during promise resolution — exactly the memory-safety / "user JS can synchronously free your state" territory the review guidelines flag as most-blocked. I traced each outcome case for behavioral equivalence and verified the new takeCodecPromise call inside transformChunk cannot free the coder early (nativeTransformReleaseStateIfIdle at JSTransformStreamDefaultController.cpp:406 short-circuits on m_nativeStateInUse, which runNativeArm holds around the arm). The RETURN_IF_EXCEPTION(scope, nullptr) early-return now leaves m_codecPromise set where it previously did not, but that path is VM termination only (runStepHere catches ordinary throws into step.thrown via a top exception scope).
Other factors
The change is a net simplification that makes the on-thread first call use the same settle path nativeCodecContinue and deliverAsync already use. The PR description reports the whole file, the WPT streams suite, and related tests passing under BUN_JSC_validateExceptionChecks=1, plus an ASAN check of the reader.cancel() variant. The new test properly restores the mutated Object.prototype in a finally and asserts the getter actually fired. Still, given the subtlety of re-entrant abandon during a step loop and this being native streams lifecycle code, deferring to a human familiar with the #38695 design is the safer call.
|
We can skip handling users overriding Object.prototype.then here. |
|
Understood, closing; the merged #38695 stands as is. |
2 participants
Follow-up to #38695, closing the last variant of the abort hang found in its review.
Problem
writer.abort()(orreader.cancel()) issued from inside a chunk's first codec step leaveswrite(),abort()andwriter.closedpending forever. Reaching that point takes anObject.prototype.thengetter: enqueueing the first piece resolves the pendingread(), and resolving a promise with an object looks up itsthen, so that getter runs synchronously inside the step loop. Not reachable from ordinary code, so this is about closing the class rather than a user report.transformChunk(src/jsc/bindings/webcore/streams/JSCompressionStreamShared.cpp) only createdm_codecPromiseafterstepChunkHerereturnedPending. An abandon issued during the first steps (writableStreamStartErroring, the source cancel algorithm) found nothing to settle,stepChunkHereonly checked for that on continuations, and the chunk was then parked on a writable already inerroring, which nothing settles unless the user happens to keep reading.Fix
transformChunkcreates and stores the promise before the first step, for the on-thread path as the thread-pool path already did, and applies the outcome with the existingsettlePendingChunk. The first call now takes exactly the continuation path: an abandon from inside a delivery resolves the promise andstepChunkHerestops, as it already did for later steps; otherwise Done / Failed / DoneSinkFull settle the same promise that used to be created per case, so the separate switch goes away (9 lines added, 22 removed in the file).promiseFulfilledWith/promiseRejectedWithwere create-then-fulfill / create-then-reject of a fresh promise, which is whatsettleCodecChunkdoes to the pre-created one; the sink-full case moves the same promise intom_nativeSinkReadyPromiseas before.writer.abort() re-entered from the chunk's first step settles the writein test/js/web/streams/compression.test.ts times out on main (thewrite()never settles; a probe confirmsabort()was called and the first 64 KiB piece was delivered) and passes with the change; the whole file (66 tests), the vendored WPT streams suite, streams.test.js, the TextEncoderStream/TextDecoderStream tests and the node webstreams compression tests pass underBUN_JSC_validateExceptionChecks=1; a re-entrantreader.cancel()variant for brotli/gzip/zstd settles cleanly under ASAN.Background
CompressionStream/DecompressionStreamemits each input chunk in steps of at mosthighWaterMarkbytes. When the consumer is full, the chunk's transform promise (m_codecPromise) stays pending, which keeps the write in flight; the consumer's pull (or the native sink's onReady) runs further steps, and whichever side goes away first (writable erroring, readable cancel, sink detach) callsnativeCodecAbandon, which resolves that promise so the writable can finish erroring. That protocol assumed the promise exists whenever an abandon can happen; this change makes that true for the first step as well.