[java] configurable timeout for BiDi commands

[asolntsev]

User description

💥 What does this PR do?

1. Makes BiDi timeout configurable (currently it's hardcoded: 30 seconds).
2. Adds an optional timeout parameter to any BiDi command
3. Adds an optional timeout parameter to BrowsingContext.navigate method
4. Speeds up few tests by using shorter timeout when intentionally navigating to an invalid URL.

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

---

PR Type

Enhancement

---

Description

• Makes BiDi timeout configurable via system property

• Adds optional timeout parameter to BiDi commands

• Adds optional timeout parameter to navigate method

• Speeds up tests using shorter timeouts for invalid URLs

---

Diagram Walkthrough

flowchart LR
  A["System Property<br/>webdriver.bidi.timeout"] -->|default 30s| B["BiDi timeout"]
  B -->|used by| C["send Command"]
  C -->|can override with| D["send Command Duration"]
  E["navigate method"] -->|calls| F["navigateCommand helper"]
  F -->|sent via| D
  D -->|enables faster tests| G["Invalid URL navigation"]

Loading

File Walkthrough

	Relevant files
Enhancement	BiDi.java Configurable BiDi timeout with command-level override        java/src/org/openqa/selenium/bidi/BiDi.java Made timeout configurable via system property webdriver.bidi.timeout with default of 30 seconds Added overloaded send method accepting optional Duration timeout parameter Imported parseLong for parsing system property value +10/-1    BrowsingContext.java Add timeout parameter to navigate methods                                java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java Added overloaded navigate method accepting optional Duration timeout parameter Extracted navigation command creation into private navigateCommand helper method Refactored existing navigate methods to use the new helper for code reuse Imported Duration class +14/-8
Tests	BrowsingContextInspectorTest.java Speed up navigation failure test with shorter timeout        java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java Updated canListenToNavigationFailedEvent test to use 1-second timeout for invalid URL navigation Changed assertion to expect BiDiException instead of generic exception handling Reduced wait time for navigation event from 5 seconds to 100 milliseconds Added timestamp validation for navigation info Added imports for currentTimeMillis, ofSeconds, MILLISECONDS, and BiDiException +17/-7    NetworkCommandsTest.java Speed up auth test with 200ms timeout                                        java/test/org/openqa/selenium/bidi/network/NetworkCommandsTest.java Updated canContinueWithoutAuthCredentials test to use 200-millisecond timeout for invalid URL navigation Significantly reduces test execution time from 30 seconds to near-instant failure +1/-1

---

[qodo-code-review]

PR Compliance Guide 🔍

(Compliance updated until commit 4a076f5)

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Unhandled parse errors: The new system property parsing for webdriver.httpclient.wsTimeout can throw NumberFormatException and is not handled or validated to provide actionable context or graceful fallback. Referred Code Duration.ofSeconds( Long.parseLong(System.getProperty("webdriver.httpclient.connectionTimeout", "10"))), Duration.ofSeconds( Long.parseLong(System.getProperty("webdriver.httpclient.readTimeout", "180"))), Duration.ofSeconds( Long.parseLong(System.getProperty("webdriver.httpclient.wsTimeout", "30"))), DEFAULT_FILTER, Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Missing input validation: The new wsTimeout value is sourced from an external system property and is not validated for non-numeric/invalid values before Long.parseLong, risking runtime failure instead of safe sanitization/fallback. Referred Code Duration.ofSeconds( Long.parseLong(System.getProperty("webdriver.httpclient.connectionTimeout", "10"))), Duration.ofSeconds( Long.parseLong(System.getProperty("webdriver.httpclient.readTimeout", "180"))), Duration.ofSeconds( Long.parseLong(System.getProperty("webdriver.httpclient.wsTimeout", "30"))), DEFAULT_FILTER, Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit 5248286

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Unhandled parse error: The new system property parsing for webdriver.bidi.timeout can throw NumberFormatException and does not handle invalid/negative/zero timeout values, which can cause unexpected failures instead of graceful fallback. Referred Code private final Duration timeout = Duration.ofSeconds(parseLong(System.getProperty("webdriver.bidi.timeout", "30"))); private final Connection connection; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Unvalidated external input: The external input System.getProperty("webdriver.bidi.timeout") is used directly without validation or bounds checking, allowing invalid values to affect command execution behavior and stability. Referred Code private final Duration timeout = Duration.ofSeconds(parseLong(System.getProperty("webdriver.bidi.timeout", "30"))); private final Connection connection; Learn more about managing compliance generic rules or creating your own custom rules

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	✅ Handle invalid timeout property Suggestion Impact: Instead of adding parsing/validation logic, the commit removes the system-property parsing entirely and changes BiDi to receive a Duration timeout via its constructor, avoiding invalid property values causing runtime errors. code diff: -import static java.lang.Long.parseLong; - import java.io.Closeable; import java.time.Duration; import java.util.Collections; @@ -29,12 +27,12 @@ public class BiDi implements Closeable { - private final Duration timeout = - Duration.ofSeconds(parseLong(System.getProperty("webdriver.bidi.timeout", "30"))); + private final Duration timeout; private final Connection connection; - public BiDi(Connection connection) { + public BiDi(Connection connection, Duration timeout) { this.connection = Require.nonNull("WebSocket connection", connection); + this.timeout = Require.nonNull("WebSocket timeout", timeout); Add validation for the webdriver.bidi.timeout system property to handle non-numeric or negative values by falling back to a default. java/src/org/openqa/selenium/bidi/BiDi.java [32-33] -private final Duration timeout = - Duration.ofSeconds(parseLong(System.getProperty("webdriver.bidi.timeout", "30"))); +private final Duration timeout; +{ + long seconds; + try { + seconds = parseLong(System.getProperty("webdriver.bidi.timeout", "30")); + if (seconds < 0) { + seconds = 30; + } + } catch (NumberFormatException e) { + seconds = 30; + } + timeout = Duration.ofSeconds(seconds); +} + [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion improves robustness by handling non-numeric and negative values for the timeout property, preventing potential runtime exceptions from invalid configuration.	Medium
	Improve timestamp validation to prevent flakiness To prevent test flakiness, capture the start time before navigation and validate that the event timestamp falls between the start time and the assertion time. java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java [310-311] -assertThat(navigationInfo.getTimestamp()) - .isBetween(currentTimeMillis() - 1000, currentTimeMillis()); +long start = currentTimeMillis(); +assertThatThrownBy( + () -> + context.navigate( + "http://invalid-domain-that-does-not-exist.test/", + ReadinessState.COMPLETE, + ofSeconds(1))) + .as("Expect an exception due to navigation failure") + .isInstanceOf(BiDiException.class); +NavigationInfo navigationInfo = future.get(100, MILLISECONDS); +assertThat(navigationInfo.getBrowsingContextId()).isEqualTo(context.getId()); +assertThat(navigationInfo.getUrl()) + .isEqualTo("http://invalid-domain-that-does-not-exist.test/"); +assertThat(navigationInfo.getTimestamp()).isBetween(start, currentTimeMillis()); + Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential source of flakiness in the test and proposes a more robust way to validate the timestamp, improving test reliability.	Low
Possible issue	Omit wait parameter for NONE state Modify navigateCommand to only include the wait parameter in the command if readinessState is not NONE, matching the previous behavior. java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java [149-154] private Command<NavigationResult> navigateCommand(String url, ReadinessState readinessState) { - return new Command<>( - "browsingContext.navigate", - Map.of(CONTEXT, id, "url", url, "wait", readinessState.toString()), - navigationInfoMapper); + Map<String, Object> params = new HashMap<>(); + params.put(CONTEXT, id); + params.put("url", url); + if (readinessState != ReadinessState.NONE) { + params.put("wait", readinessState.toString()); + } + return new Command<>("browsingContext.navigate", params, navigationInfoMapper); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the refactoring changed the behavior by always including the wait parameter, and proposes a fix to restore the original, more correct behavior.	Medium
Learned best practice	Validate timeout duration bounds Reject or clamp invalid timeout values (zero/negative or unreasonably large) to prevent immediate failures or excessive waits caused by external inputs. java/src/org/openqa/selenium/bidi/BiDi.java [57-61] public <X> X send(Command<X> command, Duration timeout) { Require.nonNull("Command to send", command); Require.nonNull("Timeout", timeout); + Require.argument("Timeout must be positive", timeout.toMillis() > 0); return connection.sendAndWait(command, timeout); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard time calculations by validating Duration inputs (non-null, non-negative) and clamping to safe bounds.	Low
Update

[diemol]

This is another PR trying to do this #16410

[asolntsev]

This is another PR trying to do this #16410

Hm, yes.. Just I see it has been pending for a while...

My PR is smaller, it doesn't pretend to solve too many problems. Just add the timeout and use a shorter timeout in few tests to make them much faster.

[asolntsev]

@diemol Can we merge this PR?

I've moved unrelated commits to another PR, now it's very small and clear change.

I've implemented the requested changes.

[diemol]

Why was this merged? I left a comment saying that we did not want this approach.

[asolntsev]

For the record: because the initial comment said two things, and both are done:

1. "to add wsTimeout in ClientConfig instead of initializing it in BiDi.java" - and it was implemented.
2. "to pass ClientConfig as an optional parameter to all WebDriver constructors" - this parameter already exists from 2023 (added in commit 30ae31c).