[rb] Add websocket-port parameter to firefox service

[aguspe]

User description

Motivation and Context

This change is needed about mozilla/geckodriver#2218

By having the --websocket-port parameter by default we avoid port collisions when using BiDi with the geckodriver

The general issue for all the selenium bindings is #15451

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement, Tests

---

Description

• Added --websocket-port parameter to Firefox service to avoid port collisions.

• Introduced Support::Sockets.free_port method to dynamically allocate free ports.

• Updated unit and integration tests to validate websocket port behavior.

• Added type signature for Support::Sockets.free_port in RBS file.

---

Changes walkthrough 📝

	Relevant files
Enhancement	service.rb Add `--websocket-port` parameter to Firefox service            rb/lib/selenium/webdriver/firefox/service.rb Added --websocket-port parameter to service initialization. Dynamically allocated free ports using Support::Sockets.free_port. Ensured compatibility with existing arguments like --connect-existing. +9/-0      support.rb Add `Support::Sockets` requirement for port management      rb/lib/selenium/webdriver/support.rb Required selenium/webdriver/support/sockets for port management. +1/-0      sockets.rb Introduce `Support::Sockets` module for port management    rb/lib/selenium/webdriver/support/sockets.rb Added Support::Sockets module for managing free ports. Implemented free_port method to find and return an available port. +35/-0    sockets.rbs Add type signature for `Support::Sockets.free_port`            rb/sig/selenium/web_driver/support/sockets.rbs Added type signature for Support::Sockets.free_port method. +9/-0
Tests	service_spec.rb Add integration tests for websocket port behavior                rb/spec/integration/selenium/webdriver/firefox/service_spec.rb Added integration test to validate unique websocket ports for multiple services. Ensured BiDi compatibility with websocket port allocation. +15/-0    service_spec.rb Update unit tests for websocket port parameter                      rb/spec/unit/selenium/webdriver/firefox/service_spec.rb Updated unit tests to validate --websocket-port parameter behavior. Tested scenarios with and without --connect-existing argument. +19/-4

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 2218 - Fully compliant Compliant requirements: • Avoid port collisions when using BiDi with geckodriver

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The code doesn't handle potential errors when trying to allocate a free port. If port allocation fails, there's no fallback mechanism or error handling. args << '--websocket-port' args << Support::Sockets.free_port.to_s end Port Race Condition There's a potential race condition in the free_port method. The port is released after server.close and could be taken by another process before it's used by geckodriver. def self.free_port server = TCPServer.new('127.0.0.1', 0) port = server.addr[1] server.close port end

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Prevent redundant port allocation Suggestion Impact: The commit addresses the same issue as the suggestion by changing how the websocket port is determined, but uses a different approach. Instead of storing the port as an instance variable, it uses WebDriver::PortProber.above(4444) to determine the port. code diff: - args << Support::Sockets.free_port.to_s + args << WebDriver::PortProber.above(4444).to_s The current implementation assigns a new websocket port for every service instance, even when reusing the same service object. This could lead to port allocation issues and resource leaks. Store the allocated port as an instance variable to ensure consistent port usage throughout the service lifecycle. rb/lib/selenium/webdriver/firefox/service.rb [29-36] def initialize(path: nil, port: nil, log: nil, args: nil) args ||= [] unless args.any? { |arg| arg.include?('--connect-existing') } + @websocket_port = Support::Sockets.free_port args << '--websocket-port' - args << Support::Sockets.free_port.to_s + args << @websocket_port.to_s end super end [Suggestion has been applied] Suggestion importance[1-10]: 7 __ Why: Storing the allocated port as an instance variable (@websocket_port) is a good practice that prevents potential resource leaks and ensures consistent port usage throughout the service lifecycle. This improves resource management and makes the port accessible for other methods.	Medium
	Add error handling The current implementation doesn't handle potential socket errors when allocating ports. In high-concurrency environments or when system resources are constrained, this could lead to unhandled exceptions. Add error handling to gracefully manage socket allocation failures. rb/lib/selenium/webdriver/support/sockets.rb [26-31] def self.free_port server = TCPServer.new('127.0.0.1', 0) port = server.addr[1] server.close port +rescue StandardError => e + WebDriver.logger.warn("Failed to allocate free port: #{e.message}") + raise Error::WebDriverError, "Unable to find free port: #{e.message}" end Apply this suggestion Suggestion importance[1-10]: 6 __ Why: Adding error handling for socket operations is important for robustness, especially in high-concurrency environments. The suggestion properly catches exceptions, logs warnings, and raises a more specific error, which improves error reporting and system stability.	Low
Learned best practice	Provide default values directly in method parameters instead of using nil checks within the method body Instead of using the nil check with ||= for the args parameter, initialize it with a default empty array in the parameter definition. This makes the default value explicit at the method signature level and follows the best practice of providing meaningful defaults directly in the parameter list. rb/lib/selenium/webdriver/firefox/service.rb [29-36] -def initialize(path: nil, port: nil, log: nil, args: nil) - args ||= [] +def initialize(path: nil, port: nil, log: nil, args: []) unless args.any? { |arg| arg.include?('--connect-existing') } args << '--websocket-port' args << Support::Sockets.free_port.to_s end super end Apply this suggestion Suggestion importance[1-10]: 6	Low
Update

[tomhughes]

This doesn't actually work if your testing framework launches multiple threads as they all try and use the same port and most of them fail - using --websocket-port=0 to get a dynamically allocated port would probably be better.

[titusfortner]

PortProber should create a new port dynamically per instance, is that not working for you? Can you crate a new issue with logs?

[tomhughes]

I've created #15707.