[java]: add websocket-port test and --connect-existing check

[navin772]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

Java fix for #15451.

Java already uses a free websocket port for new firefox webdriver sessions. This PR adds a check for the --connect-existing flag and a BiDi based test for multiple Firefox driver sessions.

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

Bug fix, Tests

---

Description

• Added a check for the --connect-existing flag in GeckoDriverService to avoid websocket port conflicts.

• Enhanced logic to allocate websocket ports only when not connecting to an existing Firefox instance.

• Introduced a BiDi-based test to verify multiple Firefox driver sessions with unique websocket ports.

• Ensured proper validation of BiDi websocket URLs and port uniqueness in the new test.

---

Changes walkthrough 📝

	Relevant files
Bug fix	GeckoDriverService.java Add `--connect-existing` check and websocket port allocation logic java/src/org/openqa/selenium/firefox/GeckoDriverService.java Added logic to check for the --connect-existing flag. Allocated websocket ports only when not connecting to an existing instance. Updated origin handling logic to avoid conflicts with multiple Firefox instances. +19/-6
Tests	FirefoxDriverConcurrentTest.java Add BiDi test for multiple Firefox instances                          java/test/org/openqa/selenium/firefox/FirefoxDriverConcurrentTest.java Added a new test for multiple Firefox instances with BiDi enabled. Verified unique websocket ports for each Firefox instance. Validated BiDi websocket URLs and port extraction logic. +32/-0

---

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:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Bug The code checks for "--connect-existing" in the args list that's being built in the same method, but at this point the list only contains the port argument. The check should examine the builder's arguments instead. for (String arg : args) { if (arg.contains("--connect-existing")) { connectExisting = true; break; } } Resource Leak The test creates two Firefox driver instances but doesn't properly clean them up in a finally block, unlike other tests in the same file. @Test void multipleFirefoxInstancesWithBiDiEnabledCanRunSimultaneously() { // Create two Firefox instances with BiDi enabled FirefoxOptions options1 = new FirefoxOptions().enableBiDi(); FirefoxOptions options2 = new FirefoxOptions().enableBiDi(); FirefoxDriver driver1; FirefoxDriver driver2; // Start the first Firefox instance driver1 = new FirefoxDriver(options1); BiDi biDi1 = driver1.getBiDi(); assertThat(biDi1).isNotNull(); // Extract the BiDi websocket URL and port for the first instance String webSocketUrl1 = (String) driver1.getCapabilities().getCapability("webSocketUrl"); String port1 = webSocketUrl1.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1"); // Start the second Firefox instance driver2 = new FirefoxDriver(options2); BiDi biDi2 = driver2.getBiDi(); assertThat(biDi2).isNotNull(); // Extract the BiDi websocket URL and port for the second instance String webSocketUrl2 = (String) driver2.getCapabilities().getCapability("webSocketUrl"); String port2 = webSocketUrl2.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1"); // Verify that the ports are different assertThat(port1).isNotEqualTo(port2); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Fix incorrect arguments check Suggestion Impact: The suggestion identified a logical error in the code. Instead of fixing it with the suggested approach, the commit completely refactored the logic by removing the problematic loop and replacing it with a cleaner implementation using the marionettePort field to determine connection behavior. code diff: - // Check if we're connecting to an existing Firefox instance - boolean connectExisting = false; - for (String arg : args) { - if (arg.contains("--connect-existing")) { - connectExisting = true; - break; + // Check if marionette port is specified via connectToExisting method + if (marionettePort != null) { + args.add("--connect-existing"); + args.add("--marionette-port"); + args.add(String.valueOf(marionettePort)); + } else { + // Configure websocket port for BiDi communication + if (websocketPort != null) { + args.add("--websocket-port"); + args.add(String.valueOf(websocketPort)); + + args.add("--allow-origins"); + args.add(String.format("http://127.0.0.1:%d", websocketPort)); + args.add(String.format("http://localhost:%d", websocketPort)); + args.add(String.format("http://[::1]:%d", websocketPort)); + } else { + // Use 0 to auto-allocate a free port + args.add("--websocket-port"); + args.add("0"); } - } - - // Only allocate a free port for the websocket when not connecting to an existing instance - // This avoids conflicts when multiple Firefox instances are started - if (!connectExisting) { - int wsPort = PortProber.findFreePort(); - args.add(String.format("--websocket-port=%d", wsPort)); - - args.add("--allow-origins"); - args.add(String.format("http://127.0.0.1:%d", wsPort)); - args.add(String.format("http://localhost:%d", wsPort)); - args.add(String.format("http://[::1]:%d", wsPort)); } The code is checking for the "--connect-existing" flag in the args list before it's fully populated. The check should be performed on the command line arguments passed to the service, not on the args list that's being built in this method. java/src/org/openqa/selenium/firefox/GeckoDriverService.java [225-232] // Check if we're connecting to an existing Firefox instance boolean connectExisting = false; -for (String arg : args) { +for (String arg : getArgs()) { if (arg.contains("--connect-existing")) { connectExisting = true; break; } } Apply this suggestion Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical logical error where the code is checking for "--connect-existing" in the args list that's currently being built, rather than in the already-provided arguments. This would cause the check to always return false, breaking the conditional logic for websocket port allocation.	High
General	✅ Add resource cleanup Suggestion Impact: The commit implemented the suggested resource cleanup pattern by adding a try-finally block and ensuring both drivers are properly quit. The implementation went beyond the suggestion by also changing the driver types from FirefoxDriver to WebDriver and using WebDriverBuilder instead of direct instantiation. code diff: + WebDriver driver1 = null; + WebDriver driver2 = null; + + try { + driver1 = new WebDriverBuilder().get(options1); + BiDi biDi1 = ((FirefoxDriver) driver1).getBiDi(); + assertThat(biDi1).isNotNull(); + + // Extract the BiDi websocket URL and port for the first instance + String webSocketUrl1 = + (String) ((FirefoxDriver) driver1).getCapabilities().getCapability("webSocketUrl"); + String port1 = webSocketUrl1.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1"); + + driver2 = new WebDriverBuilder().get(options2); + BiDi biDi2 = ((FirefoxDriver) driver2).getBiDi(); + assertThat(biDi2).isNotNull(); + + // Extract the BiDi websocket URL and port for the second instance + String webSocketUrl2 = + (String) ((FirefoxDriver) driver2).getCapabilities().getCapability("webSocketUrl"); + String port2 = webSocketUrl2.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1"); + + // Verify that the ports are different + assertThat(port1).isNotEqualTo(port2); + } finally { + // Clean up + if (driver1 != null) { + driver1.quit(); + } + if (driver2 != null) { + driver2.quit(); + } + } The test doesn't properly clean up resources in case of test failure. Add a try-finally block to ensure both drivers are properly quit, similar to the pattern used in other tests in this class. java/test/org/openqa/selenium/firefox/FirefoxDriverConcurrentTest.java [169-181] @Test void multipleFirefoxInstancesWithBiDiEnabledCanRunSimultaneously() { // Create two Firefox instances with BiDi enabled FirefoxOptions options1 = new FirefoxOptions().enableBiDi(); FirefoxOptions options2 = new FirefoxOptions().enableBiDi(); - FirefoxDriver driver1; - FirefoxDriver driver2; + FirefoxDriver driver1 = null; + FirefoxDriver driver2 = null; - // Start the first Firefox instance - driver1 = new FirefoxDriver(options1); - BiDi biDi1 = driver1.getBiDi(); - assertThat(biDi1).isNotNull(); + try { + // Start the first Firefox instance + driver1 = new FirefoxDriver(options1); + BiDi biDi1 = driver1.getBiDi(); + assertThat(biDi1).isNotNull(); [Suggestion has been applied] Suggestion importance[1-10]: 7 __ Why: The suggestion addresses an important resource leak issue by adding proper cleanup in a try-finally block, ensuring Firefox driver instances are terminated even if the test fails. This follows the pattern used in other tests and prevents resource leaks that could affect subsequent tests.	Medium
	Add null checks The test should handle the case where the webSocketUrl might be null, which could happen if BiDi initialization fails. Add null checks before attempting to extract the port from the URL. java/test/org/openqa/selenium/firefox/FirefoxDriverConcurrentTest.java [183-194] // Extract the BiDi websocket URL and port for the first instance String webSocketUrl1 = (String) driver1.getCapabilities().getCapability("webSocketUrl"); +assertThat(webSocketUrl1).isNotNull(); String port1 = webSocketUrl1.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1"); // Start the second Firefox instance driver2 = new FirefoxDriver(options2); BiDi biDi2 = driver2.getBiDi(); assertThat(biDi2).isNotNull(); // Extract the BiDi websocket URL and port for the second instance String webSocketUrl2 = (String) driver2.getCapabilities().getCapability("webSocketUrl"); +assertThat(webSocketUrl2).isNotNull(); String port2 = webSocketUrl2.replaceAll("^ws://[^:]+:(\\d+)/.*$", "$1"); Apply this suggestion Suggestion importance[1-10]: 6 __ Why: The suggestion adds important null checks before processing the webSocketUrl values, which prevents potential NullPointerExceptions if BiDi initialization fails. This improves test robustness and provides clearer error messages when failures occur.	Low
Update

[titusfortner]

I don't think the args can contain that flag. The service instances in Java creates args by each method in the builder, not by passing in an array of args directly.

[navin772]

@titusfortner yes, that's true, do you know where and how should I access the builder args?

Also, java bindings already used a different websocket port in case of BiDi, but it didn't had a check for --connect-existing

[titusfortner]

So, the current code doesn't allow specifying either marionette-port or websocket-port.
I think we'd need to add a method to the builder for connectToExisting(int marionettePort)

To support --connect-existing we'd want the user to also specify marionette-port (so the driver knows which existing Firefox instance to connect to)

Then in the createArgs if the user defined the marionette port you pass that in instead of a websocket port

[diemol]

@navin772, what is left to do here? I see many open conversations.

[navin772]

@diemol I think there were some issues in accessing the passed args and need to add a new method connectToExisting as Titus suggested above. I will take a deeper look at this on Monday and see what works best here.

[navin772]

I have added 2 methods - connectToExisting and withWebSocketPort to pass the respective ports as suggested by @titusfortner. Also, addressed the comments by @joerg1985.