[grid] Fix latent bugs in WebSocket proxy#17429
Open
shs96c wants to merge 5 commits intoSeleniumHQ:trunkfrom
Open
[grid] Fix latent bugs in WebSocket proxy#17429shs96c wants to merge 5 commits intoSeleniumHQ:trunkfrom
shs96c wants to merge 5 commits intoSeleniumHQ:trunkfrom
Conversation
MessageInboundConverter handled orphan continuation frames and unknown frame types by calling ctx.write(frame), which sends the frame back out to the peer. They should travel forward through the inbound pipeline so the upgrade/keepalive handlers can deal with them.
The converter was calling ctx.writeAndFlush(frame) and dropping the incoming ChannelPromise, so any caller awaiting the original future would never see it complete. Pipe the promise through ctx.write(..., promise) and rely on the upstream writeAndFlush() to trigger the flush.
BinaryMessage(ByteBuffer) sized its array by capacity(), which works only because Netty's ByteBuf.nioBuffer() happens to return a slice where capacity == remaining. A caller passing a flipped ByteBuffer backed by a larger array would either pad zero bytes onto the message or hit a BufferUnderflowException. Use remaining() so the contract matches the comment "data to use".
A failing forward currently fires the exception through the pipeline but leaves upstreamClosing unset, so any frames already queued behind this one re-attempt the same failing send before the close handshake runs. Set the flag on first failure so subsequent frames short-circuit to the drop path immediately.
The factory passed to WebSocketUpgradeHandler may have already opened an upstream WebSocket and acquired a connection slot by the time the WS handshake itself fails (unsupported Sec-WebSocket-Version, or the handshake future completing exceptionally). The previous code dropped the produced consumer without invoking it, so the upstream and the slot leaked. Drive each failure path through the consumer's CloseMessage cleanup so existing consumer logic frees the resources.
selenium-ci
added
C-java
Contributor
Review Summary by QodoFix five latent bugs in Grid WebSocket proxy implementation
WalkthroughsDescription• Forward orphan WebSocket frames inbound through pipeline, not outbound to peer • Propagate ChannelPromise through MessageOutboundConverter to complete futures • Size BinaryMessage by readable bytes instead of buffer capacity • Latch upstreamClosing flag on WebSocketFrameProxy send failure • Release WebSocket consumer on handshake failure to prevent resource leaks Diagramflowchart LR
A["Orphan WebSocket Frames"] -->|fireChannelRead| B["Forward Inbound"]
C["ChannelPromise"] -->|ctx.write| D["MessageOutboundConverter"]
E["ByteBuffer"] -->|remaining| F["BinaryMessage Sizing"]
G["Send Failure"] -->|set flag| H["upstreamClosing Latch"]
I["Handshake Failure"] -->|CloseMessage| J["Consumer Cleanup"]
File Changes1. java/src/org/openqa/selenium/netty/server/MessageInboundConverter.java
|
Contributor
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five small, independent bug fixes in the Grid WebSocket proxy code, each on its own commit with a focused unit test. No behaviour change for callers; each commit explains the failure mode it closes.
MessageInboundConverter). Orphan continuation frames and unknown-type frames went out viactx.writeinstead of forward through the pipeline viactx.fireChannelRead— a malformed peer would be echoed instead of dropped.ChannelPromisethroughMessageOutboundConverter. The converter calledctx.writeAndFlush(frame)and dropped the supplied promise, so any caller awaiting the original future would never see it complete.BinaryMessage(ByteBuffer)by readable bytes, not buffer capacity. The constructor sized its array bycapacity(); works only because Netty'snioBuffer()happens to beslice()-backed. A flipped buffer over a larger backing array would either pad zeros onto the message or hitBufferUnderflowException.upstreamClosingonWebSocketFrameProxysend failure. A failing forward fired the exception through the pipeline but left the flag unset, so any frames already queued behind it re-attempted the same failing send before the close handshake ran.WebSocketUpgradeHandler). The factory passed in may have already opened an upstream WebSocket and acquired a connection slot before the WS handshake itself fails (unsupportedSec-WebSocket-Version, or handshake-future failure). The previous code dropped the produced consumer without invoking it, leaking both the upstream and the slot. Both failure paths now drive the consumer through itsCloseMessagecleanup so the existing consumer logic frees the resources.Test plan
🤖 Generated with Claude Code