[rb] fix silent hang on oversized WebSocket frames

[aguspe]

💥 What does this PR do?

The Ruby devtools/bidi websocket listener can hang when it gets a frame bigger than WebSocket.max_frame_size (websocket-ruby defaults to 20MB).

Websocket-ruby raises TooLong internally but rescues it unless you set should_raise, so incoming_frame.next just returns nil, the oversized bytes stay buffered, and the loop spins forever at ~100% cpu.

builds on #17286, thanks @Chandan25sharma. left the track_cancelled part out, that's separate.

🔧 Implementation Notes

the bump happens in WebSocketConnection#initialize, not at load time, and only when the current limit is lower, so non-devtools users or anyone who already set a bigger value aren't affected. websocket-ruby only
exposes max_frame_size globally so there's no per-instance option. didn't touch should_raise, detection uses the per-instance incoming_frame.error? instead.

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: Partially the description description and RBS signatures
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

Review Summary by Qodo

Fix silent hang on oversized WebSocket frames

🐞 Bug fix

Walkthroughs

Description

• Raise WebSocket frame size limit to 100MB for DevTools connections
• Surface dropped frames as logged errors instead of silent hangs
• Add frame error detection to stop listener gracefully
• Include unit tests for frame size and error handling

Diagram

flowchart LR
  A["WebSocket Frame Received"] --> B["Check Frame Size"]
  B --> C{"Exceeds Limit?"}
  C -->|No| D["Process Frame"]
  C -->|Yes| E["frame_dropped? Detects Error"]
  E --> F["Log Error Message"]
  F --> G["Stop Listener Gracefully"]
  D --> H["Continue Processing"]

Loading

File Changes

1. rb/lib/selenium/webdriver/common/websocket_connection.rb 🐞 Bug fix +39/-9

Implement frame size limit and error detection

• Added MAX_FRAME_SIZE constant set to 100MB
• Introduced apply_frame_size_limit method to conditionally raise global WebSocket limit
• Extracted frame processing logic into process_incoming_frames method
• Added frame_dropped? method to detect and log frame decoding errors
• Modified attach_socket_listener to break on dropped frames instead of spinning

rb/lib/selenium/webdriver/common/websocket_connection.rb

---

2. rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb 🧪 Tests +75/-0

Add unit tests for frame handling

• New unit test file for WebSocketConnection
• Tests apply_frame_size_limit raises low limits and preserves higher ones
• Tests frame_dropped? returns false for valid frames and true for oversized frames
• Uses frame allocation to test logic without socket connections

rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb

---

3. rb/sig/gems/websocket/websocket.rbs 📝 Documentation +3/-0

Add WebSocket max_frame_size type signatures

• Added type signatures for WebSocket.max_frame_size getter and setter methods

rb/sig/gems/websocket/websocket.rbs

---

View more (1)

4. rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs 📝 Documentation +8/-0

Update WebSocketConnection type signatures

• Added MAX_FRAME_SIZE constant type signature
• Added type signatures for process_incoming_frames, frame_dropped?, and
 apply_frame_size_limit methods

rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

1. Listener stops without closing ✓ Resolved 🐞 Bug ☼ Reliability

Description

When frame_dropped? becomes true, attach_socket_listener breaks out of its read loop without
closing the socket or setting @closing, leaving the connection in a “dead listener, open socket”
state. Subsequent send_cmd calls can hang until Wait#until times out because no thread is
reading responses anymore.

Code

rb/lib/selenium/webdriver/common/websocket_connection.rb[R141-145]

+            process_incoming_frames

-              @messages_mtx.synchronize { callbacks[message['method']].dup }.each do |callback|
-                @callback_threads.add(callback_thread(message['params'], &callback))
-              end
-            end
+            # Stop instead of spinning forever on a frame that can never be parsed.
+            break if frame_dropped?
          end

Evidence

The listener loop breaks on frame_dropped? without closing the socket or setting @closing, but
send_cmd depends on the listener thread to read and populate messages; without it, Wait#until
will only raise after its timeout, appearing as a hang.

rb/lib/selenium/webdriver/common/websocket_connection.rb[132-149]
rb/lib/selenium/webdriver/common/websocket_connection.rb[164-175]
rb/lib/selenium/webdriver/common/websocket_connection.rb[105-119]
rb/lib/selenium/webdriver/common/wait.rb[51-77]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When an oversized/undecodable frame is detected, the listener thread exits via `break if frame_dropped?`, but the connection is not transitioned to a closed/closing state and the socket is not closed. This leaves `send_cmd` able to write, but with no listener to ever populate `messages`, causing callers to block until timeout.

## Issue Context
- `send_cmd` waits for a response ID to appear in `messages` via `Wait#until`.
- The listener thread is the only mechanism that reads from the socket and calls `process_frame` to populate `messages`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[105-119]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[132-149]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[164-179]

## Proposed fix
1. When `frame_dropped?` is true, transition the connection to a closed/closing state and close the socket.
  - Avoid calling `close` directly from the socket thread (it joins threads); instead introduce a helper like `shutdown_socket!(reason)` that:
    - sets `@closing = true` under `@closing_mtx`
    - closes `socket` (rescuing `CONNECTION_ERRORS`)
    - optionally stores `@listener_error` for later reporting.
2. Make `send_cmd` fail fast when the listener has terminated due to a decode error (e.g., if `@listener_error` is set, or if `@socket_thread` is not alive and `@closing` is false), raising a `WebDriverError` with an actionable message rather than waiting 30s.
3. Consider emitting a debug log when the listener exits normally via this path, so user logs show a clear lifecycle event.

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

---

2. around block lacks ensure ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The spec mutates global WebSocket.max_frame_size but restores it only after example.call, so an
exception/failing expectation can skip restoration and contaminate subsequent tests. This violates
the requirement to restore mutated global process state in an ensure/finally block.

Code

rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[R29-33]

+      around do |example|
+        original = WebSocket.max_frame_size
+        example.call
+        WebSocket.max_frame_size = original
+      end

Evidence

PR Compliance ID 9 requires restoring mutated global state using ensure/finally. The added
around hook sets WebSocket.max_frame_size, runs example.call, and restores afterward without
an ensure, so restoration can be skipped on error.

rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The spec changes global `WebSocket.max_frame_size` but does not guarantee it is restored if the example raises/fails, which can leak state into later tests.

## Issue Context
RSpec expectation failures raise exceptions; without `ensure`, the cleanup line may not run.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]

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

---

3. Incomplete error log hint 🐞 Bug ◔ Observability ⭐ New

Description

frame_dropped? logs an error message ending with Raise WebSocket.max_frame_size= but provides no
suggested value (despite having a known MAX_FRAME_SIZE constant), making the remediation unclear
when a connection is terminated due to an oversized frame. This slows diagnosis of a production
failure mode where the listener closes itself on decode errors.

Code

rb/lib/selenium/webdriver/common/websocket_connection.rb[R170-171]

+        WebDriver.logger.error("WebSocket frame dropped (#{incoming_frame.error}): exceeds max_frame_size " \
+                               "(#{WebSocket.max_frame_size} bytes). Raise WebSocket.max_frame_size=", id: :ws)

Evidence

The log message currently ends with Raise WebSocket.max_frame_size= without providing any value;
the class defines MAX_FRAME_SIZE (100MB), which can be referenced to make the log actionable.

rb/lib/selenium/webdriver/common/websocket_connection.rb[38-42]
rb/lib/selenium/webdriver/common/websocket_connection.rb[170-172]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The error log emitted when a frame is dropped ends with `Raise WebSocket.max_frame_size=` but does not include a recommended value (or example assignment), so it’s not actionable.

### Issue Context
`MAX_FRAME_SIZE` is defined (100MB) and the log already prints the current `WebSocket.max_frame_size`; operators need a concrete next step when this terminal condition occurs.

### Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[170-172]

### Suggested change
Update the log string to include a recommended value, e.g.:
- `... Raise WebSocket.max_frame_size=#{MAX_FRAME_SIZE} (or higher).`
- Or `... Set WebSocket.max_frame_size to >= #{MAX_FRAME_SIZE}.`
Also remove the trailing `=` if no assignment example is included.

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

---

4. Global frame size persists 🐞 Bug ⛨ Security

Description

apply_frame_size_limit permanently raises the process-wide WebSocket.max_frame_size when it is
below 100MB, and close never restores the previous value. This can unexpectedly override a user’s
intentionally-lower global limit for the remainder of the process after the first DevTools/BiDi
connection.

Code

rb/lib/selenium/webdriver/common/websocket_connection.rb[R177-179]

+      def apply_frame_size_limit
+        WebSocket.max_frame_size = MAX_FRAME_SIZE if WebSocket.max_frame_size < MAX_FRAME_SIZE
+      end

Evidence

The code calls apply_frame_size_limit in initialization and conditionally sets the global
WebSocket.max_frame_size, but no restoration occurs in close, so the process-wide setting
persists beyond the connection’s lifecycle.

rb/lib/selenium/webdriver/common/websocket_connection.rb[43-57]
rb/lib/selenium/webdriver/common/websocket_connection.rb[177-179]
rb/lib/selenium/webdriver/common/websocket_connection.rb[59-80]
rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`WebSocket.max_frame_size` is global module state; `WebSocketConnection#initialize` conditionally increases it, but no code restores the prior value when the connection is closed. If an application intentionally configured a smaller limit (e.g., for memory/DoS constraints), using Selenium DevTools/BiDi will override that policy for the remainder of the process.

## Issue Context
- The test suite explicitly saves/restores `WebSocket.max_frame_size`, which highlights that this state is global and sticky.
- Because it’s global, multiple concurrent connections need careful handling to avoid restoring too early.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[43-57]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[59-80]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[177-179]
- rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]

## Proposed fix
1. Track whether this instance modified the global value:
  - Store `@original_max_frame_size` and a flag like `@bumped_frame_size_limit` when raising it.
2. Restore safely on `close`:
  - Prefer a class-level refcount + mutex (e.g., `@@frame_size_bumps`) so restoration happens only when the last connection that bumped the value closes.
  - Restore only if `WebSocket.max_frame_size` is still equal to `MAX_FRAME_SIZE` (or the value you set) to avoid clobbering user changes made after initialization.
3. Add/update specs to validate restoration behavior (including multiple connections bumping concurrently if your test suite supports it).

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

---

Previous review results

Review updated until commit 510ee4e

Results up to commit ce6fe48

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. around block lacks ensure ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The spec mutates global WebSocket.max_frame_size but restores it only after example.call, so an
exception/failing expectation can skip restoration and contaminate subsequent tests. This violates
the requirement to restore mutated global process state in an ensure/finally block.

Code

rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[R29-33]

+      around do |example|
+        original = WebSocket.max_frame_size
+        example.call
+        WebSocket.max_frame_size = original
+      end

Evidence

PR Compliance ID 9 requires restoring mutated global state using ensure/finally. The added
around hook sets WebSocket.max_frame_size, runs example.call, and restores afterward without
an ensure, so restoration can be skipped on error.

rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The spec changes global `WebSocket.max_frame_size` but does not guarantee it is restored if the example raises/fails, which can leak state into later tests.

## Issue Context
RSpec expectation failures raise exceptions; without `ensure`, the cleanup line may not run.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]

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

---

2. Listener stops without closing ✓ Resolved 🐞 Bug ☼ Reliability

Description

When frame_dropped? becomes true, attach_socket_listener breaks out of its read loop without
closing the socket or setting @closing, leaving the connection in a “dead listener, open socket”
state. Subsequent send_cmd calls can hang until Wait#until times out because no thread is
reading responses anymore.

Code

rb/lib/selenium/webdriver/common/websocket_connection.rb[R141-145]

+            process_incoming_frames

-              @messages_mtx.synchronize { callbacks[message['method']].dup }.each do |callback|
-                @callback_threads.add(callback_thread(message['params'], &callback))
-              end
-            end
+            # Stop instead of spinning forever on a frame that can never be parsed.
+            break if frame_dropped?
          end

Evidence

The listener loop breaks on frame_dropped? without closing the socket or setting @closing, but
send_cmd depends on the listener thread to read and populate messages; without it, Wait#until
will only raise after its timeout, appearing as a hang.

rb/lib/selenium/webdriver/common/websocket_connection.rb[132-149]
rb/lib/selenium/webdriver/common/websocket_connection.rb[164-175]
rb/lib/selenium/webdriver/common/websocket_connection.rb[105-119]
rb/lib/selenium/webdriver/common/wait.rb[51-77]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When an oversized/undecodable frame is detected, the listener thread exits via `break if frame_dropped?`, but the connection is not transitioned to a closed/closing state and the socket is not closed. This leaves `send_cmd` able to write, but with no listener to ever populate `messages`, causing callers to block until timeout.

## Issue Context
- `send_cmd` waits for a response ID to appear in `messages` via `Wait#until`.
- The listener thread is the only mechanism that reads from the socket and calls `process_frame` to populate `messages`.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[105-119]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[132-149]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[164-179]

## Proposed fix
1. When `frame_dropped?` is true, transition the connection to a closed/closing state and close the socket.
  - Avoid calling `close` directly from the socket thread (it joins threads); instead introduce a helper like `shutdown_socket!(reason)` that:
    - sets `@closing = true` under `@closing_mtx`
    - closes `socket` (rescuing `CONNECTION_ERRORS`)
    - optionally stores `@listener_error` for later reporting.
2. Make `send_cmd` fail fast when the listener has terminated due to a decode error (e.g., if `@listener_error` is set, or if `@socket_thread` is not alive and `@closing` is false), raising a `WebDriverError` with an actionable message rather than waiting 30s.
3. Consider emitting a debug log when the listener exits normally via this path, so user logs show a clear lifecycle event.

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

---

3. Global frame size persists 🐞 Bug ⛨ Security

Description

apply_frame_size_limit permanently raises the process-wide WebSocket.max_frame_size when it is
below 100MB, and close never restores the previous value. This can unexpectedly override a user’s
intentionally-lower global limit for the remainder of the process after the first DevTools/BiDi
connection.

Code

rb/lib/selenium/webdriver/common/websocket_connection.rb[R177-179]

+      def apply_frame_size_limit
+        WebSocket.max_frame_size = MAX_FRAME_SIZE if WebSocket.max_frame_size < MAX_FRAME_SIZE
+      end

Evidence

The code calls apply_frame_size_limit in initialization and conditionally sets the global
WebSocket.max_frame_size, but no restoration occurs in close, so the process-wide setting
persists beyond the connection’s lifecycle.

rb/lib/selenium/webdriver/common/websocket_connection.rb[43-57]
rb/lib/selenium/webdriver/common/websocket_connection.rb[177-179]
rb/lib/selenium/webdriver/common/websocket_connection.rb[59-80]
rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`WebSocket.max_frame_size` is global module state; `WebSocketConnection#initialize` conditionally increases it, but no code restores the prior value when the connection is closed. If an application intentionally configured a smaller limit (e.g., for memory/DoS constraints), using Selenium DevTools/BiDi will override that policy for the remainder of the process.

## Issue Context
- The test suite explicitly saves/restores `WebSocket.max_frame_size`, which highlights that this state is global and sticky.
- Because it’s global, multiple concurrent connections need careful handling to avoid restoring too early.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[43-57]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[59-80]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[177-179]
- rb/spec/unit/selenium/webdriver/common/websocket_connection_spec.rb[29-33]

## Proposed fix
1. Track whether this instance modified the global value:
  - Store `@original_max_frame_size` and a flag like `@bumped_frame_size_limit` when raising it.
2. Restore safely on `close`:
  - Prefer a class-level refcount + mutex (e.g., `@@frame_size_bumps`) so restoration happens only when the last connection that bumped the value closes.
  - Restore only if `WebSocket.max_frame_size` is still equal to `MAX_FRAME_SIZE` (or the value you set) to avoid clobbering user changes made after initialization.
3. Add/update specs to validate restoration behavior (including multiple connections bumping concurrently if your test suite supports it).

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

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 510ee4e