Skip to content

[rb] fix silent hang on large WebSocket frame #17286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Chandan25sharma wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

[rb] fix silent hang on large WebSocket frame #17286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions rb/lib/selenium/webdriver/common/driver_extensions/has_network_interception.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ module HasNetworkInterception
# @yieldparam [Proc] continue block which proceeds with the request and optionally yields response
#

def intercept(&block)
def intercept(**options, &block)
if browser == :firefox
WebDriver.logger.deprecate(
'Driver#intercept on Firefox',
Expand All @@ -68,7 +68,7 @@ def intercept(&block)
)
end
@interceptor ||= DevTools::NetworkInterceptor.new(devtools)
@interceptor.intercept(&block)
@interceptor.intercept(**options, &block)
end
end # HasNetworkInterception
end # DriverExtensions
Expand Down
14 changes: 12 additions & 2 deletions rb/lib/selenium/webdriver/common/websocket_connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

require 'websocket'

WebSocket.should_raise = true
WebSocket.max_frame_size = 100 * 1024 * 1024

Comment on lines +22 to +24
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Global websocket config override 🐞 Bug ☼ Reliability

websocket_connection.rb sets WebSocket.should_raise and WebSocket.max_frame_size at file load
time, overriding process-wide websocket-ruby configuration for every Selenium user (even those not
using DevTools/BiDi). This can unexpectedly change behavior and memory limits for other WebSocket
clients in the same Ruby process and makes the new 100MB frame size an unconditional global default.
Agent Prompt 
### Issue description
`WebSocket.should_raise` and `WebSocket.max_frame_size` are set as global websocket-ruby configuration at file load time. Because this file is required by default, it changes behavior and memory limits process-wide for all Selenium users and potentially any other WebSocket usage in the same app.

### Issue Context
This is a library-level side effect: merely requiring Selenium modifies global WebSocket behavior.

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[20-24]
- rb/lib/selenium/webdriver/common.rb[70-103]

### What to change
- Avoid unconditional top-level global assignments. Prefer an explicit Selenium-owned configuration surface (e.g., `Selenium::WebDriver::WebSocketConnection.configure(max_frame_size:, should_raise:)`) or, at minimum, only apply defaults if the user hasn’t configured them and document the side effect.
- If websocket-ruby supports per-instance/per-frame configuration, set it on the connection/frame objects instead of mutating global module state.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

module Selenium
module WebDriver
class WebSocketConnection
Expand Down Expand Up @@ -109,7 +112,10 @@ def send_cmd(**payload)
raise e, "WebSocket is closed (#{e.class}: #{e.message})"
end

wait.until { @messages_mtx.synchronize { messages.delete(id) } }
wait.until do
@messages_mtx.synchronize { messages.delete(id) } ||
(raise Error::WebDriverError, "WebSocket listener thread is dead" unless @socket_thread&.alive?)
end
end

private
Expand Down Expand Up @@ -144,7 +150,11 @@ def attach_socket_listener
end
end
rescue *CONNECTION_ERRORS, WebSocket::Error => e
WebDriver.logger.debug "WebSocket listener closed: #{e.class}: #{e.message}", id: :ws
if e.is_a?(WebSocket::Error)
WebDriver.logger.warn "WebSocket listener closed due to error: #{e.class}: #{e.message}", id: :ws
else
WebDriver.logger.debug "WebSocket listener closed: #{e.class}: #{e.message}", id: :ws
end
end
end

Expand Down
6 changes: 3 additions & 3 deletions rb/lib/selenium/webdriver/devtools/network_interceptor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ def initialize(devtools)
@lock = Mutex.new
end

def intercept(&block)
devtools.network.on(:loading_failed) { |params| track_cancelled_request(params) }
def intercept(track_cancelled: true, &block)
devtools.network.on(:loading_failed) { |params| track_cancelled_request(params) } if track_cancelled
devtools.fetch.on(:request_paused) { |params| request_paused(params, &block) }

devtools.network.set_cache_disabled(cache_disabled: true)
devtools.network.enable
devtools.network.enable if track_cancelled
devtools.fetch.enable(patterns: [{requestStage: 'Request'}, {requestStage: 'Response'}])
Comment on lines +46 to 52
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Track_cancelled opt-out breaks 🐞 Bug ≡ Correctness

When NetworkInterceptor#intercept(track_cancelled: false) is used, Selenium stops recording
cancelled request IDs, but the cancellation-suppression logic still depends on that list to decide
whether to swallow INVALID_INTERCEPTION_ID errors. As a result, normal browser cancellations can
start raising WebDriverError exceptions and break interception for users who disable tracking.
Agent Prompt 
### Issue description
With `track_cancelled: false`, the interceptor can no longer mark cancellations, but `with_cancellable_request` still requires a cancellation marker to suppress `INVALID_INTERCEPTION_ID` errors. This makes expected browser cancellations surface as exceptions and can break request interception.

### Issue Context
- `cancelled_requests` is only populated by `track_cancelled_request`, which only runs if the `loading_failed` handler is installed.
- `with_cancellable_request` treats `INVALID_INTERCEPTION_ID` as cancellable only when `cancelled?(network_id)` returns true.

### Fix Focus Areas
- rb/lib/selenium/webdriver/devtools/network_interceptor.rb[46-53]
- rb/lib/selenium/webdriver/devtools/network_interceptor.rb[71-75]
- rb/lib/selenium/webdriver/devtools/network_interceptor.rb[77-89]
- rb/lib/selenium/webdriver/devtools/network_interceptor.rb[161-169]

### What to change
Implement a behavior for `track_cancelled: false` that does not rely on `cancelled_requests`:
- Option A: Store `@track_cancelled` in `intercept(...)` and update `with_cancellable_request` to swallow `INVALID_INTERCEPTION_ID` unconditionally when tracking is disabled.
- Option B: Derive cancellation from other available signals in `Fetch.requestPaused` (if feasible) so `cancelled?` can still be true without enabling Network events.
- Ensure the opt-out reduces bandwidth *without* turning expected cancellations into hard failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

end

Expand Down
Loading