From f562aae0b7f3100dc614f6a8470d5c27c8828952 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Wed, 22 Oct 2025 02:16:24 -0500 Subject: [PATCH 1/9] [rb] add synchronization and error handling for socket interactions --- .../webdriver/common/websocket_connection.rb | 100 ++++++++++++------ .../selenium/webdriver/remote/bidi_bridge.rb | 5 +- rb/lib/selenium/webdriver/remote/bridge.rb | 11 +- 3 files changed, 75 insertions(+), 41 deletions(-) diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index 26f02ebc9bf1c..5e7299f6a4db0 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -24,7 +24,10 @@ module WebDriver class WebSocketConnection CONNECTION_ERRORS = [ Errno::ECONNRESET, # connection is aborted (browser process was killed) - Errno::EPIPE # broken pipe (browser process was killed) + Errno::EPIPE, # broken pipe (browser process was killed) + Errno::EBADF, # file descriptor already closed (double-close or GC) + IOError, # Ruby socket read/write after close + EOFError # socket reached EOF after remote closed cleanly ].freeze RESPONSE_WAIT_TIMEOUT = 30 @@ -34,7 +37,8 @@ class WebSocketConnection def initialize(url:) @callback_threads = ThreadGroup.new - + @mtx = Mutex.new + @closing = false @session_id = nil @url = url @@ -43,9 +47,26 @@ def initialize(url:) end def close - @callback_threads.list.each(&:exit) - @socket_thread.exit - socket.close + @mtx.synchronize do + return if @closing + + @closing = true + end + + begin + socket.close + rescue *CONNECTION_ERRORS => e + WebDriver.logger.debug "WebSocket listener closed: #{e.class}: #{e.message}", id: :ws + # already closed + end + + # Let threads unwind instead of calling exit + @socket_thread&.join(0.5) + @callback_threads.list.each do |thread| + thread.join(0.5) + rescue StandardError + nil + end end def callbacks @@ -53,62 +74,72 @@ def callbacks end def add_callback(event, &block) - callbacks[event] << block - block.object_id + @mtx.synchronize do + callbacks[event] << block + block.object_id + end end def remove_callback(event, id) - return if callbacks[event].reject! { |callback| callback.object_id == id } + removed = @mtx.synchronize { callbacks[event].reject! { |cb| cb.object_id == id } } + return if removed || @closing - ids = callbacks[event]&.map(&:object_id) + ids = @mtx.synchronize { callbacks[event]&.map(&:object_id) } raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}" end def send_cmd(**payload) id = next_id data = payload.merge(id: id) - WebDriver.logger.debug "WebSocket -> #{data}"[...MAX_LOG_MESSAGE_SIZE], id: :bidi + WebDriver.logger.debug "WebSocket -> #{data}"[...MAX_LOG_MESSAGE_SIZE], id: :ws data = JSON.generate(data) out_frame = WebSocket::Frame::Outgoing::Client.new(version: ws.version, data: data, type: 'text') - socket.write(out_frame.to_s) - wait.until { messages.delete(id) } + begin + socket.write(out_frame.to_s) + rescue *CONNECTION_ERRORS => e + raise Error::WebDriverError, "WebSocket is closed (#{e.class}: #{e.message})" + end + + wait.until do + @mtx.synchronize { messages.delete(id) } + end end private - # We should be thread-safe to use the hash without synchronization - # because its keys are WebSocket message identifiers and they should be - # unique within a devtools session. def messages @messages ||= {} end def process_handshake socket.print(ws.to_s) - ws << socket.readpartial(1024) + ws << socket.readpartial(1024) until ws.finished? end def attach_socket_listener Thread.new do - Thread.current.abort_on_exception = true Thread.current.report_on_exception = false - until socket.eof? + loop do + break if @closing + incoming_frame << socket.readpartial(1024) while (frame = incoming_frame.next) + break if @closing + message = process_frame(frame) next unless message['method'] params = message['params'] - callbacks[message['method']].each do |callback| + @mtx.synchronize { callbacks[message['method']].dup }.each do |callback| @callback_threads.add(callback_thread(params, &callback)) end end end - rescue *CONNECTION_ERRORS - Thread.stop + rescue *CONNECTION_ERRORS, WebSocket::Error => e + WebDriver.logger.debug "WebSocket listener closed: #{e.class}: #{e.message}", id: :ws end end @@ -122,27 +153,26 @@ def process_frame(frame) # Firefox will periodically fail on unparsable empty frame return {} if message.empty? - message = JSON.parse(message) - messages[message['id']] = message - WebDriver.logger.debug "WebSocket <- #{message}"[...MAX_LOG_MESSAGE_SIZE], id: :bidi + msg = JSON.parse(message) + @mtx.synchronize { messages[msg['id']] = msg if msg.key?('id') } - message + WebDriver.logger.debug "WebSocket <- #{msg}"[...MAX_LOG_MESSAGE_SIZE], id: :ws + msg end def callback_thread(params) Thread.new do - Thread.current.abort_on_exception = true - - # We might end up blocked forever when we have an error in event. - # For example, if network interception event raises error, - # the browser will keep waiting for the request to be proceeded - # before returning back to the original thread. In this case, - # we should at least print the error. - Thread.current.report_on_exception = true + Thread.current.abort_on_exception = false + Thread.current.report_on_exception = false + return if @closing yield params - rescue Error::WebDriverError, *CONNECTION_ERRORS - Thread.stop + rescue Error::WebDriverError, *CONNECTION_ERRORS => e + WebDriver.logger.debug "Callback aborted: #{e.class}: #{e.message}", id: :ws + rescue StandardError => e + # Unexpected handler failure; log with a short backtrace. + bt = Array(e.backtrace).first(5).join("\n") + WebDriver.logger.error "Callback error: #{e.class}: #{e.message}\n#{bt}", id: :ws end end diff --git a/rb/lib/selenium/webdriver/remote/bidi_bridge.rb b/rb/lib/selenium/webdriver/remote/bidi_bridge.rb index c95ddec538f85..9c576d7e1f54f 100644 --- a/rb/lib/selenium/webdriver/remote/bidi_bridge.rb +++ b/rb/lib/selenium/webdriver/remote/bidi_bridge.rb @@ -46,9 +46,10 @@ def refresh end def quit - super - ensure bidi.close + super + rescue *QUIT_ERRORS + nil end def close diff --git a/rb/lib/selenium/webdriver/remote/bridge.rb b/rb/lib/selenium/webdriver/remote/bridge.rb index d4318a25b0088..82f4772437e15 100644 --- a/rb/lib/selenium/webdriver/remote/bridge.rb +++ b/rb/lib/selenium/webdriver/remote/bridge.rb @@ -206,13 +206,16 @@ def switch_to_default_content switch_to_frame nil end - QUIT_ERRORS = [IOError].freeze + QUIT_ERRORS = [IOError, EOFError, WebSocket::Error].freeze def quit execute :delete_session - http.close - rescue *QUIT_ERRORS - nil + ensure + begin + http.close + rescue *QUIT_ERRORS + nil + end end def close From 430068f709a10196cd2e6963127b5df673045f94 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Wed, 22 Oct 2025 08:22:46 -0500 Subject: [PATCH 2/9] fix when errors get bubbled up --- .../webdriver/common/websocket_connection.rb | 7 +++++-- rb/lib/selenium/webdriver/remote/bridge.rb | 15 ++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index 5e7299f6a4db0..edca3a522887c 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -170,9 +170,12 @@ def callback_thread(params) rescue Error::WebDriverError, *CONNECTION_ERRORS => e WebDriver.logger.debug "Callback aborted: #{e.class}: #{e.message}", id: :ws rescue StandardError => e - # Unexpected handler failure; log with a short backtrace. + # Do not allow an error raised during bidi interception to prevent potential deadlock + Thread.main.raise(e) unless @closing || params['isBlocked'] + + condition = @closing ? 'on closing' : 'during interception' bt = Array(e.backtrace).first(5).join("\n") - WebDriver.logger.error "Callback error: #{e.class}: #{e.message}\n#{bt}", id: :ws + WebDriver.logger.error "Callback error #{condition}: #{e.class}: #{e.message}\n#{bt}", id: :ws end end diff --git a/rb/lib/selenium/webdriver/remote/bridge.rb b/rb/lib/selenium/webdriver/remote/bridge.rb index 82f4772437e15..bacc3e338d804 100644 --- a/rb/lib/selenium/webdriver/remote/bridge.rb +++ b/rb/lib/selenium/webdriver/remote/bridge.rb @@ -209,13 +209,18 @@ def switch_to_default_content QUIT_ERRORS = [IOError, EOFError, WebSocket::Error].freeze def quit - execute :delete_session - ensure begin - http.close - rescue *QUIT_ERRORS - nil + execute :delete_session + rescue *QUIT_ERRORS => e + WebDriver.logger.debug "delete_session failed during quit: #{e.class}: #{e.message}", id: :ws + ensure + begin + http.close + rescue *QUIT_ERRORS => e + WebDriver.logger.debug "http.close failed during quit: #{e.class}: #{e&.message}", id: :ws + end end + nil end def close From 069bdae96744594a30e08b51fee25142fbe2c0c1 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Wed, 22 Oct 2025 09:49:54 -0500 Subject: [PATCH 3/9] do not override the connection error type --- rb/lib/selenium/webdriver/common/websocket_connection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index edca3a522887c..13c974a194f40 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -98,7 +98,7 @@ def send_cmd(**payload) begin socket.write(out_frame.to_s) rescue *CONNECTION_ERRORS => e - raise Error::WebDriverError, "WebSocket is closed (#{e.class}: #{e.message})" + raise e, "WebSocket is closed (#{e.class}: #{e.message})" end wait.until do From cfddb1fdae00bc31cf830fcfa989331869cac02e Mon Sep 17 00:00:00 2001 From: titusfortner Date: Wed, 22 Oct 2025 16:12:11 -0500 Subject: [PATCH 4/9] unguard passing test likely has been failing for the wrong reason --- rb/spec/integration/selenium/webdriver/remote/driver_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rb/spec/integration/selenium/webdriver/remote/driver_spec.rb b/rb/spec/integration/selenium/webdriver/remote/driver_spec.rb index bd6a5a200fa82..614870bee456e 100644 --- a/rb/spec/integration/selenium/webdriver/remote/driver_spec.rb +++ b/rb/spec/integration/selenium/webdriver/remote/driver_spec.rb @@ -83,8 +83,7 @@ module Remote end end - it 'errors when not set', {except: {browser: :firefox, reason: 'grid always sets true and firefox returns it'}, - exclude: {browser: :safari, reason: 'grid hangs'}} do + it 'errors when not set', exclude: {browser: :safari, reason: 'grid hangs'} do reset_driver!(enable_downloads: false) do |driver| expect { driver.downloadable_files From f1e7c26bb721b762d06b303ca950d2e5d47653b7 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Wed, 22 Oct 2025 17:39:57 -0500 Subject: [PATCH 5/9] fix debug logging id names --- rb/lib/selenium/webdriver/remote/bridge.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rb/lib/selenium/webdriver/remote/bridge.rb b/rb/lib/selenium/webdriver/remote/bridge.rb index bacc3e338d804..94d4cf263a110 100644 --- a/rb/lib/selenium/webdriver/remote/bridge.rb +++ b/rb/lib/selenium/webdriver/remote/bridge.rb @@ -212,12 +212,12 @@ def quit begin execute :delete_session rescue *QUIT_ERRORS => e - WebDriver.logger.debug "delete_session failed during quit: #{e.class}: #{e.message}", id: :ws + WebDriver.logger.debug "delete_session failed during quit: #{e.class}: #{e.message}", id: :quit ensure begin http.close rescue *QUIT_ERRORS => e - WebDriver.logger.debug "http.close failed during quit: #{e.class}: #{e&.message}", id: :ws + WebDriver.logger.debug "http.close failed during quit: #{e.class}: #{e&.message}", id: :quit end end nil From 5f175e352b042aa6cc7ce5a98cf13351e0f19194 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Thu, 23 Oct 2025 12:06:46 -0500 Subject: [PATCH 6/9] add suggested fixes --- .../webdriver/common/websocket_connection.rb | 32 ++++++++++++------- .../selenium/webdriver/remote/bidi_bridge.rb | 3 +- rb/lib/selenium/webdriver/remote/bridge.rb | 2 +- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index 13c974a194f40..0cd953a82a653 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -37,7 +37,11 @@ class WebSocketConnection def initialize(url:) @callback_threads = ThreadGroup.new - @mtx = Mutex.new + + @callbacks_mtx = Mutex.new + @messages_mtx = Mutex.new + @closing_mtx = Mutex.new + @closing = false @session_id = nil @url = url @@ -47,7 +51,7 @@ def initialize(url:) end def close - @mtx.synchronize do + @closing_mtx.synchronize do return if @closing @closing = true @@ -64,8 +68,8 @@ def close @socket_thread&.join(0.5) @callback_threads.list.each do |thread| thread.join(0.5) - rescue StandardError - nil + rescue StandardError => e + WebDriver.logger.debug "Failed to join thread during close: #{e.class}: #{e.message}", id: :ws end end @@ -74,18 +78,22 @@ def callbacks end def add_callback(event, &block) - @mtx.synchronize do + @callbacks_mtx.synchronize do callbacks[event] << block block.object_id end end def remove_callback(event, id) - removed = @mtx.synchronize { callbacks[event].reject! { |cb| cb.object_id == id } } - return if removed || @closing + @callbacks_mtx.synchronize do + return if @closing + + callbacks_for_event = callbacks[event] + return if callbacks_for_event.reject! { |cb| cb.object_id == id } - ids = @mtx.synchronize { callbacks[event]&.map(&:object_id) } - raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}" + ids = callbacks[event]&.map(&:object_id) + raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}" + end end def send_cmd(**payload) @@ -102,7 +110,7 @@ def send_cmd(**payload) end wait.until do - @mtx.synchronize { messages.delete(id) } + @messages_mtx.synchronize { messages.delete(id) } end end @@ -133,7 +141,7 @@ def attach_socket_listener next unless message['method'] params = message['params'] - @mtx.synchronize { callbacks[message['method']].dup }.each do |callback| + @messages_mtx.synchronize { callbacks[message['method']].dup }.each do |callback| @callback_threads.add(callback_thread(params, &callback)) end end @@ -154,7 +162,7 @@ def process_frame(frame) return {} if message.empty? msg = JSON.parse(message) - @mtx.synchronize { messages[msg['id']] = msg if msg.key?('id') } + @messages_mtx.synchronize { messages[msg['id']] = msg if msg.key?('id') } WebDriver.logger.debug "WebSocket <- #{msg}"[...MAX_LOG_MESSAGE_SIZE], id: :ws msg diff --git a/rb/lib/selenium/webdriver/remote/bidi_bridge.rb b/rb/lib/selenium/webdriver/remote/bidi_bridge.rb index 9c576d7e1f54f..fe5d3e8cc6677 100644 --- a/rb/lib/selenium/webdriver/remote/bidi_bridge.rb +++ b/rb/lib/selenium/webdriver/remote/bidi_bridge.rb @@ -47,9 +47,10 @@ def refresh def quit bidi.close - super rescue *QUIT_ERRORS nil + ensure + super end def close diff --git a/rb/lib/selenium/webdriver/remote/bridge.rb b/rb/lib/selenium/webdriver/remote/bridge.rb index 94d4cf263a110..ca35dc42ab89c 100644 --- a/rb/lib/selenium/webdriver/remote/bridge.rb +++ b/rb/lib/selenium/webdriver/remote/bridge.rb @@ -217,7 +217,7 @@ def quit begin http.close rescue *QUIT_ERRORS => e - WebDriver.logger.debug "http.close failed during quit: #{e.class}: #{e&.message}", id: :quit + WebDriver.logger.debug "http.close failed during quit: #{e.class}: #{e.message}", id: :quit end end nil From 7333da60d200312bff5178666a9108bd82c0b384 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Thu, 23 Oct 2025 13:19:03 -0500 Subject: [PATCH 7/9] fix bad logic about when to bubble up exceptions in callbacks --- rb/lib/selenium/webdriver/common/websocket_connection.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index 0cd953a82a653..ab40319628a61 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -178,12 +178,12 @@ def callback_thread(params) rescue Error::WebDriverError, *CONNECTION_ERRORS => e WebDriver.logger.debug "Callback aborted: #{e.class}: #{e.message}", id: :ws rescue StandardError => e - # Do not allow an error raised during bidi interception to prevent potential deadlock - Thread.main.raise(e) unless @closing || params['isBlocked'] + # Main thread needs to know that it can stop waiting + Thread.main.raise(e) if params['isBlocked'] + return if @closing - condition = @closing ? 'on closing' : 'during interception' bt = Array(e.backtrace).first(5).join("\n") - WebDriver.logger.error "Callback error #{condition}: #{e.class}: #{e.message}\n#{bt}", id: :ws + WebDriver.logger.error "Callback error: #{e.class}: #{e.message}\n#{bt}", id: :ws end end From 481e0d7fda2799d8197471c3241ea53b6ad5a225 Mon Sep 17 00:00:00 2001 From: titusfortner Date: Thu, 23 Oct 2025 21:47:50 -0500 Subject: [PATCH 8/9] keep error propagation for devtools but mark deprecated --- rb/lib/selenium/webdriver/bidi.rb | 2 +- .../webdriver/common/websocket_connection.rb | 28 +++++++++++++------ rb/lib/selenium/webdriver/devtools.rb | 2 +- .../webdriver/common/websocket_connection.rbs | 8 +++++- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/rb/lib/selenium/webdriver/bidi.rb b/rb/lib/selenium/webdriver/bidi.rb index 80fc5b92fbe22..f5444451bc789 100644 --- a/rb/lib/selenium/webdriver/bidi.rb +++ b/rb/lib/selenium/webdriver/bidi.rb @@ -32,7 +32,7 @@ class BiDi autoload :InterceptedItem, 'selenium/webdriver/bidi/network/intercepted_item' def initialize(url:) - @ws = WebSocketConnection.new(url: url) + @ws = WebSocketConnection.new(url: url, protocol: :bidi) end def close diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index ab40319628a61..f6f5cb094ea26 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -35,7 +35,7 @@ class WebSocketConnection MAX_LOG_MESSAGE_SIZE = 9999 - def initialize(url:) + def initialize(url:, protocol: nil) @callback_threads = ThreadGroup.new @callbacks_mtx = Mutex.new @@ -45,6 +45,7 @@ def initialize(url:) @closing = false @session_id = nil @url = url + @protocol = protocol process_handshake @socket_thread = attach_socket_listener @@ -91,7 +92,7 @@ def remove_callback(event, id) callbacks_for_event = callbacks[event] return if callbacks_for_event.reject! { |cb| cb.object_id == id } - ids = callbacks[event]&.map(&:object_id) + ids = callbacks_for_event.map(&:object_id) raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}" end end @@ -109,9 +110,7 @@ def send_cmd(**payload) raise e, "WebSocket is closed (#{e.class}: #{e.message})" end - wait.until do - @messages_mtx.synchronize { messages.delete(id) } - end + wait.until { @messages_mtx.synchronize { messages.delete(id) } } end private @@ -140,9 +139,8 @@ def attach_socket_listener message = process_frame(frame) next unless message['method'] - params = message['params'] @messages_mtx.synchronize { callbacks[message['method']].dup }.each do |callback| - @callback_threads.add(callback_thread(params, &callback)) + @callback_threads.add(callback_thread(message['params'], &callback)) end end end @@ -178,10 +176,14 @@ def callback_thread(params) rescue Error::WebDriverError, *CONNECTION_ERRORS => e WebDriver.logger.debug "Callback aborted: #{e.class}: #{e.message}", id: :ws rescue StandardError => e - # Main thread needs to know that it can stop waiting - Thread.main.raise(e) if params['isBlocked'] return if @closing + if devtools? + # Async thread exceptions are not deterministic and should not be relied on; we should stop + WebDriver.logger.deprecate('propogating errors from DevTools callbacks') + Thread.main.raise(e) + end + bt = Array(e.backtrace).first(5).join("\n") WebDriver.logger.error "Callback error: #{e.class}: #{e.message}\n#{bt}", id: :ws end @@ -208,6 +210,14 @@ def ws @ws ||= WebSocket::Handshake::Client.new(url: @url) end + def devtools? + @protocol == :devtools + end + + def bidi? + @protocol == :bidi + end + def next_id @id ||= 0 @id += 1 diff --git a/rb/lib/selenium/webdriver/devtools.rb b/rb/lib/selenium/webdriver/devtools.rb index fdc1a194ceee9..187ff01959fb7 100644 --- a/rb/lib/selenium/webdriver/devtools.rb +++ b/rb/lib/selenium/webdriver/devtools.rb @@ -29,7 +29,7 @@ class DevTools autoload :Response, 'selenium/webdriver/devtools/response' def initialize(url:, target_type:) - @ws = WebSocketConnection.new(url: url) + @ws = WebSocketConnection.new(url: url, protocol: :devtools) @session_id = nil start_session(target_type: target_type) end diff --git a/rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs b/rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs index 98ac289f081ed..acc3c7bfefaee 100644 --- a/rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs +++ b/rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs @@ -5,6 +5,8 @@ module Selenium @callback_threads: untyped + @protocol: Symbol? + @session_id: untyped @url: untyped @@ -33,7 +35,7 @@ module Selenium MAX_LOG_MESSAGE_SIZE: Integer - def initialize: (url: untyped) -> void + def initialize: (url: String, ?protocol?: Symbol) -> void def add_callback: (untyped event) { () -> void } -> untyped @@ -47,6 +49,10 @@ module Selenium private + def bidi?: -> bool + + def devtools?: -> bool + def messages: () -> untyped def process_handshake: () -> untyped From 472d170000b6fa5a043a88a114a9b4ea95fe909f Mon Sep 17 00:00:00 2001 From: titusfortner Date: Sat, 25 Oct 2025 16:12:35 -0500 Subject: [PATCH 9/9] just switch to logging errors instead of propagating them --- rb/lib/selenium/webdriver/bidi.rb | 2 +- .../webdriver/common/websocket_connection.rb | 17 +---------------- rb/lib/selenium/webdriver/devtools.rb | 2 +- .../selenium/webdriver/devtools_spec.rb | 9 ++++----- rb/spec/rspec_matchers.rb | 2 +- 5 files changed, 8 insertions(+), 24 deletions(-) diff --git a/rb/lib/selenium/webdriver/bidi.rb b/rb/lib/selenium/webdriver/bidi.rb index f5444451bc789..80fc5b92fbe22 100644 --- a/rb/lib/selenium/webdriver/bidi.rb +++ b/rb/lib/selenium/webdriver/bidi.rb @@ -32,7 +32,7 @@ class BiDi autoload :InterceptedItem, 'selenium/webdriver/bidi/network/intercepted_item' def initialize(url:) - @ws = WebSocketConnection.new(url: url, protocol: :bidi) + @ws = WebSocketConnection.new(url: url) end def close diff --git a/rb/lib/selenium/webdriver/common/websocket_connection.rb b/rb/lib/selenium/webdriver/common/websocket_connection.rb index f6f5cb094ea26..585089fcc3cd0 100644 --- a/rb/lib/selenium/webdriver/common/websocket_connection.rb +++ b/rb/lib/selenium/webdriver/common/websocket_connection.rb @@ -35,7 +35,7 @@ class WebSocketConnection MAX_LOG_MESSAGE_SIZE = 9999 - def initialize(url:, protocol: nil) + def initialize(url:) @callback_threads = ThreadGroup.new @callbacks_mtx = Mutex.new @@ -45,7 +45,6 @@ def initialize(url:, protocol: nil) @closing = false @session_id = nil @url = url - @protocol = protocol process_handshake @socket_thread = attach_socket_listener @@ -178,12 +177,6 @@ def callback_thread(params) rescue StandardError => e return if @closing - if devtools? - # Async thread exceptions are not deterministic and should not be relied on; we should stop - WebDriver.logger.deprecate('propogating errors from DevTools callbacks') - Thread.main.raise(e) - end - bt = Array(e.backtrace).first(5).join("\n") WebDriver.logger.error "Callback error: #{e.class}: #{e.message}\n#{bt}", id: :ws end @@ -210,14 +203,6 @@ def ws @ws ||= WebSocket::Handshake::Client.new(url: @url) end - def devtools? - @protocol == :devtools - end - - def bidi? - @protocol == :bidi - end - def next_id @id ||= 0 @id += 1 diff --git a/rb/lib/selenium/webdriver/devtools.rb b/rb/lib/selenium/webdriver/devtools.rb index 187ff01959fb7..fdc1a194ceee9 100644 --- a/rb/lib/selenium/webdriver/devtools.rb +++ b/rb/lib/selenium/webdriver/devtools.rb @@ -29,7 +29,7 @@ class DevTools autoload :Response, 'selenium/webdriver/devtools/response' def initialize(url:, target_type:) - @ws = WebSocketConnection.new(url: url, protocol: :devtools) + @ws = WebSocketConnection.new(url: url) @session_id = nil start_session(target_type: target_type) end diff --git a/rb/spec/integration/selenium/webdriver/devtools_spec.rb b/rb/spec/integration/selenium/webdriver/devtools_spec.rb index 4506ecbbed5a1..a552a05e81c65 100644 --- a/rb/spec/integration/selenium/webdriver/devtools_spec.rb +++ b/rb/spec/integration/selenium/webdriver/devtools_spec.rb @@ -45,13 +45,12 @@ module WebDriver }.to yield_control end - it 'propagates errors in events' do + it 'logs errors in events' do + driver.devtools.page.enable + driver.devtools.page.on(:load_event_fired) { raise 'This is fine!' } expect { - driver.devtools.page.enable - driver.devtools.page.on(:load_event_fired) { raise 'This is fine!' } driver.navigate.to url_for('xhtmlTest.html') - sleep 0.5 - }.to raise_error(RuntimeError, 'This is fine!') + }.to have_error(:ws, /This is fine!/) end describe '#target' do diff --git a/rb/spec/rspec_matchers.rb b/rb/spec/rspec_matchers.rb index 88ba20289cd8b..e668368475beb 100644 --- a/rb/spec/rspec_matchers.rb +++ b/rb/spec/rspec_matchers.rb @@ -17,7 +17,7 @@ # specific language governing permissions and limitations # under the License. -LEVELS = %w[warning info deprecated].freeze +LEVELS = %w[error warning info deprecated].freeze LEVELS.each do |level| RSpec::Matchers.define "have_#{level}" do |entry|