make augmentation of HasBiDi/HasDevTools lazy-loaded #16338

asolntsev · 2025-09-20T16:23:17Z

User description

When CDP url is not accessible, the command new Augmenter().augment(remoteWebDriver) fails immediately - even if I don't want to use CDP or BiDi.

🔗 Related Issues

This PR fixes the problem described in #9803, #10132, selenide/selenide#2797, selenide/selenide#3107 etc.

💥 What does this PR do?

This PR fixes DevToolsProvider and BiDiProvider, so that augmentation of HasBiDi and HasDevTools interfaces is now lazy-loaded.

Context:

We run a remote Chrome driver in TestContainer / Selenoid / Moon / Selenium Grid
The CDP url is not accessible (because it contains the port which is accessible only inside the docker)
BUT I don't want to use CDP or BiDi.
I use Augmenter to cast my webdriver, say, to JavascriptExecutor:

WebDriver wd = new Augmenter().augment(remoteWebDriver);  // FAILS
JavascriptExecutor js = (JavascriptExecutor) wd;

This code fails because of CDP connectivity issue during augmentation:

org.openqa.selenium.remote.http.ConnectionFailedException: JdkWebSocket initial request execution error
Build info: version: '4.35.0', revision: '1c58e5028b'
System info: os.name: 'Mac OS X', os.arch: 'aarch64', os.version: '26.0', java.version: '17.0.14'
Driver info: driver.version: unknown
	at org.openqa.selenium.remote.http.jdk.JdkHttpClient.openSocket(JdkHttpClient.java:253)
	at org.openqa.selenium.devtools.Connection.<init>(Connection.java:89)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.<init>(SeleniumCdpConnection.java:36)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.lambda$create$2(SeleniumCdpConnection.java:103)
	at java.base/java.util.Optional.map(Optional.java:260)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.create(SeleniumCdpConnection.java:103)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.create(SeleniumCdpConnection.java:49)
	at org.openqa.selenium.devtools.DevToolsProvider.getImplementation(DevToolsProvider.java:50)
	at org.openqa.selenium.devtools.DevToolsProvider.getImplementation(DevToolsProvider.java:29)
	at org.openqa.selenium.remote.Augmenter.augment(Augmenter.java:202)
	at org.openqa.selenium.remote.Augmenter.augment(Augmenter.java:173)

🔄 Types of changes

Bug fix (backwards compatible)

PR Type

Bug fix

Description

Make HasBiDi and HasDevTools augmentation lazy-loaded
Fix connection failures when CDP/BiDi URLs are inaccessible
Enable augmentation to succeed even without CDP/BiDi access
Implement double-checked locking pattern for thread safety

Diagram Walkthrough

flowchart LR
  A["Augmenter.augment()"] --> B["DevToolsProvider/BiDiProvider"]
  B --> C["Lazy Implementation"]
  C --> D["Connection Created Only When Accessed"]
  D --> E["No Immediate Connection Failure"]

File Walkthrough

Relevant files

Bug fix

BiDiProvider.java `Implement lazy BiDi connection initialization` java/src/org/openqa/selenium/bidi/BiDiProvider.java Replace immediate BiDi connection creation with lazy-loaded implementation Add double-checked locking pattern for thread-safe initialization Move connection logic inside `maybeGetBiDi()` method Make `getBiDiUrl()` method static	+20/-7
DevToolsProvider.java `Implement lazy DevTools connection initialization` java/src/org/openqa/selenium/devtools/DevToolsProvider.java Replace immediate DevTools connection creation with lazy-loaded implementation Add double-checked locking pattern for thread-safe initialization Move CDP connection logic inside `maybeGetDevTools()` method Defer version detection and connection creation until first access	+18/-6

qodo-merge-pro · 2025-09-20T16:23:55Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 9803 - Partially compliant Compliant requirements: Avoid premature failures when DevTools is not intended to be used. Ensure augmentation works so DevTools can be obtained when supported. Non-compliant requirements: Allow using DevTools with RemoteWebDriver in Grid without ClassCastException when casting to HasDevTools. Requires further human verification: Validate at runtime that casting to HasDevTools on an augmented RemoteWebDriver succeeds in a Selenium Grid and DevTools session can be created when CDP is reachable. Confirm no connection attempt happens during augmentation, only upon first use, by running in a Docker/grid with inaccessible CDP endpoint. 10132 - Partially compliant Compliant requirements: Do not require CDP connectivity at augmentation time. Non-compliant requirements: Fix ClassCastException when casting augmented RemoteWebDriver to HasDevTools with Augmenter. Ensure DevTools acquisition works with selenium/standalone-chrome. Requires further human verification: Run the provided snippet against selenium/standalone-chrome and verify casting and devTools.createSession() succeed when CDP is available, and that augmentation itself does not fail when CDP is not available. 2797 - Not compliant Non-compliant requirements: WebDriverEventListener afterScript should include the return value of executed scripts. 3107 - Not compliant Non-compliant requirements: Handle IE alert interrupting test execution and proceed without interruption. 5678 - Not compliant Non-compliant requirements: Resolve ChromeDriver connection failures on subsequent instantiations (ConnectFailure). 1234 - Not compliant Non-compliant requirements: Ensure Selenium 2.48 triggers javascript in link href on click() in Firefox.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Possible Issue The lambda captures capabilities and constructs a new WebSocket client each time HasBiDi#getBiDi() is called; verify this does not create multiple connections per driver instance or leak resources. Consider memoizing the created BiDi instance. public HasBiDi getImplementation(Capabilities caps, ExecuteMethod executeMethod) { return () -> { URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); return Optional.of(new BiDi(connection)); }; Behavior Change DevTools creation is deferred; ensure that exceptions for unsupported CDP are still clear and that repeated calls do not recreate connections. Consider caching the Optional to avoid redundant connection attempts. public HasDevTools getImplementation(Capabilities caps, ExecuteMethod executeMethod) { return () -> { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); Optional<DevTools> devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); return devTools; };

qodo-merge-pro · 2025-09-20T16:24:54Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Prevent resource leaks with caching~~ Suggestion Impact: The commit implemented caching of a single BiDi instance with double-checked locking and a separate lock object, avoiding repeated client/connection creation. It also split maybeGetBiDi and getBiDi methods and stored BiDi directly instead of Optional. code diff: @Override public HasBiDi getImplementation(Capabilities caps, ExecuteMethod executeMethod) { return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private volatile BiDi biDi; + private final Object lock = new Object(); @Override public Optional<BiDi> maybeGetBiDi() { + return Optional.ofNullable(biDi); + } + + @Override + public BiDi getBiDi() { if (biDi == null) { - synchronized (this) { + synchronized (lock) { if (biDi == null) { URI wsUri = - getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); - biDi = Optional.of(new BiDi(connection)); + biDi = new BiDi(connection); } } } Cache the `BiDi` instance to prevent creating a new HTTP client and connection on every invocation. This avoids potential resource leaks and performance issues. java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56] -return () -> { - URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); - - HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); - ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); - HttpClient wsClient = clientFactory.createClient(wsConfig); - Connection connection = new Connection(wsClient, wsUri.toString()); - - return Optional.of(new BiDi(connection)); +return new HasBiDi() { + private volatile Optional<BiDi> cachedBiDi; + + @Override + public Optional<BiDi> getBiDi() { + if (cachedBiDi == null) { + synchronized (this) { + if (cachedBiDi == null) { + URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); + ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); + HttpClient wsClient = clientFactory.createClient(wsConfig); + Connection connection = new Connection(wsClient, wsUri.toString()); + cachedBiDi = Optional.of(new BiDi(connection)); + } + } + } + return cachedBiDi; + } }; `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant issue where new HTTP clients and connections are created on each call, which can lead to resource exhaustion and performance degradation. The proposed caching solution is a robust fix for this problem.	Medium
Possible issue	✅ ~~Prevent connection recreation with caching~~ Suggestion Impact: The commit introduces caching of a single DevTools instance with double-checked locking and separates maybeGetDevTools and getDevTools, preventing repeated creation. This aligns with the suggestion’s intent to cache the DevTools connection. code diff: return new HasDevTools() { - private volatile Optional<DevTools> devTools; + private volatile DevTools devTools; + private final Object lock = new Object(); @Override public Optional<DevTools> maybeGetDevTools() { + return Optional.ofNullable(devTools); + } + + @Override + public DevTools getDevTools() { if (devTools == null) { - synchronized (this) { + synchronized (lock) { if (devTools == null) { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = - cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); + cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); this.devTools = - SeleniumCdpConnection.create(caps) - .map(conn -> new DevTools(info::getDomains, conn)); + SeleniumCdpConnection.create(caps) + .map(conn -> new DevTools(info::getDomains, conn)) + .orElseThrow(() -> new DevToolsException("Unable to create DevTools connection"));; } } } Cache the `DevTools` instance to avoid recreating connections and performing version matching on every call. This prevents potential resource leaks and unnecessary overhead. java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-53] -return () -> { - Object cdpVersion = caps.getCapability("se:cdpVersion"); - String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); - - CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); - Optional<DevTools> devTools = - SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); - return devTools; +return new HasDevTools() { + private volatile Optional<DevTools> cachedDevTools; + + @Override + public Optional<DevTools> getDevTools() { + if (cachedDevTools == null) { + synchronized (this) { + if (cachedDevTools == null) { + Object cdpVersion = caps.getCapability("se:cdpVersion"); + String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); + CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); + cachedDevTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); + } + } + } + return cachedDevTools; + } }; `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that repeatedly creating `DevTools` connections and performing version matching is inefficient and can lead to resource leaks. The proposed caching mechanism is an effective solution to ensure the connection is created only once.	Medium
Learned best practice	Ensure resources are properly closed Ensure created clients/connections are closed when no longer needed by using try-with-resources or by wiring them to a closeable BiDi that disposes underlying resources. java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56] return () -> { URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); - return Optional.of(new BiDi(connection)); + BiDi bidi = new BiDi(connection) { + @Override + public void close() { + super.close(); + try { + wsClient.close(); + } catch (Exception ignored) { + } + } + }; + return Optional.of(bidi); }; Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always close or dispose resources; ensure network clients and connections are shut down to prevent leaks.	Low
Learned best practice	Dispose DevTools connections properly Tie the lifecycle of the created DevTools to its underlying connection and ensure the connection is closed (e.g., implement close() or attach shutdown hook) to avoid leaks. java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-53] return () -> { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); - Optional<DevTools> devTools = - SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); - return devTools; + return SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn) { + @Override + public void close() { + super.close(); + try { + conn.close(); + } catch (Exception ignored) { + } + } + }); }; Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Always close or dispose resources; ensure connections/listeners are shut down to prevent leaks.	Low
Update

qodo-merge-pro · 2025-09-20T16:35:34Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 3107 - Partially compliant Compliant requirements: Allow test execution to continue when certain browser APIs/features are unavailable or unreachable. Non-compliant requirements: Avoid automation flow interruptions caused by unexpected alerts/popups in IE11 by ensuring tests can proceed without being blocked. Requires further human verification: Validate in real environments (IE11/legacy setups) that augmentation no longer fails when CDP/BiDi endpoints are unreachable and does not introduce new interruptions. 2797 - Not compliant Non-compliant requirements: AfterScript event listener should include the return value of executed scripts to improve logging. 10132 - PR Code Verified Compliant requirements: Using HasDevTools with RemoteWebDriver/Augmenter should not throw ClassCastException or fail when CDP is inaccessible. Allow DevTools session creation when supported, but do not break augmentation if DevTools/CDP is unreachable. Requires further human verification: Run against Selenium Grid / dockerized Chrome to ensure augmentation no longer attempts immediate CDP connection and casting works when not using DevTools. 9803 - PR Code Verified Compliant requirements: RemoteWebDriver should be usable with HasDevTools without ClassCastException; augmentation should support lazy DevTools connection. Requires further human verification: Verify in Grid setup that HasDevTools casting succeeds and DevTools connection is deferred until first use.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Lazy Init Behavior Ensure the returned lambda does not capture a failed Optional early and that exceptions from SeleniumCdpConnection.create are only raised when getDevTools() is invoked, not during augmentation. return () -> { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); Optional<DevTools> devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); return devTools; }; Error Messaging Confirm that when BiDi is unsupported or the websocket URL is missing/unreachable, the lazy supplier throws a clear BiDiException only upon invocation, and that augmenting other interfaces remains unaffected. return () -> { URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); return Optional.of(new BiDi(connection)); };

qodo-merge-pro · 2025-09-20T16:37:01Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Prevent multiple connection creation~~ Suggestion Impact: The commit implemented caching and lazy initialization of the BiDi instance with double-checked locking, preventing multiple connections. It changed from Optional to a single BiDi field, added maybeGetBiDi(), and synchronized on a lock object. code diff: return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private volatile BiDi biDi; + private final Object lock = new Object(); @Override public Optional<BiDi> maybeGetBiDi() { + return Optional.ofNullable(biDi); + } + + @Override + public BiDi getBiDi() { if (biDi == null) { - synchronized (this) { + synchronized (lock) { if (biDi == null) { URI wsUri = - getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); - biDi = Optional.of(new BiDi(connection)); + biDi = new BiDi(connection); } } } To prevent resource leaks from multiple connections, cache the `BiDi` instance so it is created only once. The current implementation creates a new connection every time `getBiDi()` is called. java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56] -return () -> { - URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); - - HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); - ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); - HttpClient wsClient = clientFactory.createClient(wsConfig); - Connection connection = new Connection(wsClient, wsUri.toString()); - - return Optional.of(new BiDi(connection)); +return new HasBiDi() { + private volatile Optional<BiDi> biDi; + + @Override + public Optional<BiDi> getBiDi() { + if (biDi == null) { + synchronized (this) { + if (biDi == null) { + URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); + ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); + HttpClient wsClient = clientFactory.createClient(wsConfig); + Connection connection = new Connection(wsClient, wsUri.toString()); + biDi = Optional.of(new BiDi(connection)); + } + } + } + return biDi; + } }; `[Suggestion processed]` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant issue where the PR's change to lazy-load the connection would create a new connection on every call, leading to resource leaks. The proposed fix using double-checked locking correctly implements a cached, lazy-initialized connection, which is a critical improvement for correctness and performance.	High
Learned best practice	Ensure connection is closed Ensure the created client/connection is properly closed when the BiDi instance is no longer needed by wiring a close hook or using try-with-resources where possible. java/src/org/openqa/selenium/bidi/BiDiProvider.java [50-55] HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); -return Optional.of(new BiDi(connection)); +BiDi bidi = new BiDi(connection); +bidi.onClose(() -> { + try { + connection.close(); + } finally { + wsClient.close(); + } +}); +return Optional.of(bidi); Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always close or dispose resources using try-with-resources or ensure explicit shutdown to prevent leaks.	Low
More

qodo-merge-pro · 2025-09-20T17:06:26Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 9803 - PR Code Verified Compliant requirements: RemoteWebDriver should be usable with HasDevTools without ClassCastException or premature connection attempts. Augmentation should not fail when CDP endpoint is inaccessible, unless DevTools is actually used. Support using RemoteWebDriver in Grid environments where CDP URL may be unreachable. Requires further human verification: Validate on real Grid/Selenoid/Moon setups that augmentation no longer fails and casting works. Confirm that calling getDevTools() still connects properly when CDP is reachable. 10132 - PR Code Verified Compliant requirements: Avoid ClassCastException when casting augmented RemoteWebDriver to HasDevTools. Ensure augmenter returns a proxy implementing HasDevTools even if CDP is not yet connected. Defer CDP connection until DevTools is first accessed. Requires further human verification: Run an end-to-end test using selenium/standalone-chrome with remote augmentation to confirm cast succeeds and session creation works when reachable. 2797 - Not compliant Non-compliant requirements: afterScript event should include the return value of executed script. 3107 - Not compliant Non-compliant requirements: Handle IE alert interrupting automation; allow test to proceed without interruption.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Thread Safety Double-checked locking uses a volatile Optional field but returns the same reference; ensure no visibility/race issues and that Optional is always non-null after initialization. return new HasDevTools() { private volatile Optional<DevTools> devTools; @Override public Optional<DevTools> maybeGetDevTools() { if (devTools == null) { synchronized (this) { if (devTools == null) { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); this.devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); } } } return devTools; } }; Lazy Initialization Behavior maybeGetBiDi throws BiDiException if no URL capability is present; confirm this matches prior behavior and that augmenting alone does not trigger the exception unless accessed. return new HasBiDi() { private volatile Optional<BiDi> biDi; @Override public Optional<BiDi> maybeGetBiDi() { if (biDi == null) { synchronized (this) { if (biDi == null) { URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); HttpClient wsClient = clientFactory.createClient(wsConfig); Connection connection = new Connection(wsClient, wsUri.toString()); biDi = Optional.of(new BiDi(connection)); } } } return biDi; } }; } Capability Handling Version detection now deferred; verify fallback to browserVersion is correct and works when se:cdpVersion is absent, and that NoOpCdpInfo is acceptable for older/unknown versions. Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); this.devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); } }

qodo-merge-pro · 2025-09-20T17:07:22Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	✅ ~~Simplify lazy-loading with a memoized supplier~~ Suggestion Impact: The commit replaced the manual double-checked locking with a dedicated lazy initialization utility (org.openqa.selenium.concurrent.Lazy), simplifying the code and achieving the same goal as using Suppliers.memoize. code diff: return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private final Lazy<BiDi> biDi = lazy(() -> establishBiDiConnection(caps)); @Override public Optional<BiDi> maybeGetBiDi() { - if (biDi == null) { - synchronized (this) { - if (biDi == null) { - URI wsUri = - getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + return biDi.getIfInitialized(); + } - HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); - ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); - HttpClient wsClient = clientFactory.createClient(wsConfig); - Connection connection = new Connection(wsClient, wsUri.toString()); - - biDi = Optional.of(new BiDi(connection)); - } - } - } - return biDi; + @Override + public BiDi getBiDi() { + return biDi.get(); } }; + } + + private BiDi establishBiDiConnection(Capabilities caps) { + URI wsUri = getBiDiUrl(caps) + .orElseThrow(() -> new BiDiException("BiDi not supported")); + + HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); + ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); + HttpClient wsClient = clientFactory.createClient(wsConfig); + Connection connection = new Connection(wsClient, wsUri.toString()); + + return new BiDi(connection); } Replace the manual double-checked locking implementation with Guava's `Suppliers.memoize` to simplify the code and improve robustness for lazy initialization. java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-68] return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private final Supplier<Optional<BiDi>> biDiSupplier = + Suppliers.memoize( + () -> { + URI wsUri = + getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + + HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); + ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); + HttpClient wsClient = clientFactory.createClient(wsConfig); + Connection connection = new Connection(wsClient, wsUri.toString()); + + return Optional.of(new BiDi(connection)); + }); @Override public Optional<BiDi> maybeGetBiDi() { - if (biDi == null) { - synchronized (this) { - if (biDi == null) { - URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); - - HttpClient.Factory clientFactory = HttpClient.Factory.createDefault(); - ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri); - HttpClient wsClient = clientFactory.createClient(wsConfig); - Connection connection = new Connection(wsClient, wsUri.toString()); - - biDi = Optional.of(new BiDi(connection)); - } - } - } - return biDi; + return biDiSupplier.get(); } }; `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the manual double-checked locking can be replaced by a simpler and more robust library utility (`Suppliers.memoize`), significantly improving code readability and maintainability.	Medium
Learned best practice	✅ ~~Use initialized Optional with DCL~~ Suggestion Impact: Instead of initializing an Optional and using DCL, the commit refactored to use Lazy initialization for DevTools, eliminating null checks and DCL entirely. This addresses the same concurrency and initialization concerns the suggestion targeted. code diff: + final Lazy<DevTools> devTools = lazy(() -> establishDevToolsConnection(caps)); + return new HasDevTools() { - private volatile Optional<DevTools> devTools; + @Override + public Optional<DevTools> maybeGetDevTools() { + return devTools.getIfInitialized(); + } @Override - public Optional<DevTools> maybeGetDevTools() { - if (devTools == null) { - synchronized (this) { - if (devTools == null) { - Object cdpVersion = caps.getCapability("se:cdpVersion"); - String version = - cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); - - CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); - this.devTools = - SeleniumCdpConnection.create(caps) - .map(conn -> new DevTools(info::getDomains, conn)); - } - } - } - return devTools; + public DevTools getDevTools() { + return devTools.get(); } }; + } + + private DevTools establishDevToolsConnection(Capabilities caps) { + Object cdpVersion = caps.getCapability("se:cdpVersion"); + String version = + cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); + + CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); + return SeleniumCdpConnection.create(caps) + .map(conn -> new DevTools(info::getDomains, conn)) + .orElseThrow(() -> new DevToolsException("Unable to create DevTools connection")); Initialize `devTools` to `Optional.empty()` and use a local variable in the double-checked locking to avoid null checks and publication races. java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-64] return new HasDevTools() { - private volatile Optional<DevTools> devTools; + private volatile Optional<DevTools> devTools = Optional.empty(); @Override public Optional<DevTools> maybeGetDevTools() { - if (devTools == null) { + Optional<DevTools> local = devTools; + if (local.isEmpty()) { synchronized (this) { - if (devTools == null) { + local = devTools; + if (local.isEmpty()) { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); - this.devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); + local = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); + devTools = local; } } } - return devTools; + return local; } }; `[Suggestion processed]` Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Initialize attributes to sane defaults to avoid null/partially-initialized states, especially when using double-checked locking.	Low
More

joerg1985 · 2025-09-21T17:15:22Z

I have some concerns about this PR:

At least for BDI the URI should only be inside the capabilities when requested by the user, so for BiDi this should not needed.
In case a feature the client requested is not working, the augmentation should fail in my mind.
I guess most people have a retry on startup error implemented, so the retry coukd have handled this in the past and now the exception happens later and might not handled by this any more.

asolntsev · 2025-09-21T20:23:13Z

At least for BDI the URI should only be inside the capabilities when requested by the user, so for BiDi this should not needed.

@joerg1985
Maybe for BiDi, yes. But for HasDevTools, it's automatically enabled for all Chromium browsers (at least). Users cannot avoid it (even if they don't need to use DevTools). That's the root problem.

In case a feature the client requested is not working, the augmentation should fail in my mind.

No, it should not. The goal of augmentation is NOT to check all the connectivity issues. The goal is to CAST RemoteWebDriver to the needed interface. Only to CAST, not to call its methods.

I guess most people have a retry on startup error implemented, so the retry coukd have handled this in the past and now the exception happens later and might not handled by this any more.

But the augmentation is NOT automatically called on startup.
The augmentation happens only user explicitly calls new Augmenter().augment(remoteWebDriver). So there is no automated retry out-of-the-box anyway.

AND if I really want to check BiDi connectivity, I would explicitly call some BiDi method on startup.

One argument for this PR:

All other AugmenterProviders ARE lazy. See AddHasPermissions, AddHasCdp, AddHasExtensions, AddHasLogEvents, AddHasContext, AddHasNetworkConditions - they all have getImplementation of form:

  public HasDebugger getImplementation(Capabilities capabilities, ExecuteMethod executeMethod) {
    return () -> { ... ALL THE WORK ONLY HERE ...}
  }

  // or

  public HasPermissions getImplementation(Capabilities capabilities, ExecuteMethod executeMethod) {
    return new HasPermissions() {
          ... ALL THE WORK ONLY HERE ...
    };
  }

Those people need to check connectivity on startup, they have an option: call any BiDi method.
But those people who don't need BIdi or DevTools (like in my case), just cannot avoid it. They have no way to avoid this connectivity issue.

joerg1985 · 2025-09-22T15:12:17Z

@asolntsev i only wanted to point to this change in behaviour, i think having the same behaviour for all augmentations is a good idea. So i only have one comment left, all other areas rely on external synchronizations in case multiple threads are involved.
Is there a specific need to sychronize here or could this be removed?

asolntsev · 2025-09-22T15:43:28Z

Is there a specific need to sychronize here or could this be removed?

If I understand your question correctly, then yes, the synchronization is needed in this case.
This is what I mean:

@Test void augmenterThreadSafety() {
  RemoteWebDriver driver = new RemoteWebDriver(gridUrl(), new ChromeOptions());
  WebDriver webDriver = new Augmenter().augment(driver);
  HasDevTools devTools = (HasDevTools) webDriver;

    for (int i = 0; i < 10; i++) {
      new Thread(() -> {
        devTools.getDevTools().getDomains(); // would create multiple instances of `DevTools` without synchronization
      }).start();
   }
}

joerg1985 · 2025-09-23T15:04:13Z

@asolntsev in my mind the client code has to take care about synchronization, in your example:

  RemoteWebDriver driver = new RemoteWebDriver(gridUrl(), new ChromeOptions());
  WebDriver webDriver = new Augmenter().augment(driver);
  HasDevTools devTools = (HasDevTools) webDriver;

    for (int i = 0; i < 10; i++) {
      new Thread(() -> {
	synchronized (devTools) {
        	devTools.getDevTools().getDomains(); 
	}
      }).start();
   }
}

Other areas of the selenium code do not expect the client code will use different threads.
So why expecting it inside the HasBiDi / HasDevTools implementation?

e.g. the code below will end in chaos without synchronization in the client code:

  RemoteWebDriver driver = new RemoteWebDriver(gridUrl(), new ChromeOptions());

    for (int i = 0; i < 10; i++) {
      new Thread(() -> {
	driver.findElement(By.id("x")).click();
      }).start();
   }
}

asolntsev · 2025-09-23T15:31:24Z

in my mind the client code has to take care about synchronization

You cannot expect from all Selenium users that they can do synchronization properly.
I sincerely don't understand why should we intentionally make Selenium less secure. This synchronization is very easy to do, it's cheap, it's safe. I see zero problems with it.

the code below will end in chaos without synchronization in the client code:

Yes, maybe this code wouldn't do anything reasonable, but it's safe. I mean, it doesn't cause connection leak etc.
While the code above will cause connection leak.

joerg1985 · 2025-09-23T18:32:44Z

@asolntsev lets see what others think about this

nvborisenko · 2025-09-23T18:36:03Z

Easy, should be thread-safe.

asolntsev · 2025-09-24T15:02:27Z

Easy, should be thread-safe.

@nvborisenko Great! Then can we already merge this PR?

java/src/org/openqa/selenium/concurrent/Lazy.java

titusfortner · 2025-10-12T15:14:13Z

I know it is just an abstraction, but the new code feels so much better to me. Thanks also for the tests.
I still think we should add logging that we're doing it lazy now instead of on startup (maybe in the getImplementation() methods?). We can also add a warning entry for URISyntaxException, which Chrome & Firefox implementations do.

To clarify for @diemol the use case that made me think we should do this now is that a user might need to run their code on a remote grid that doesn't support sockets, the grid config would need to change not just the client code. So the user can't just toggle this from their capabilities and ensure it works as I previously suggested.
Also, if this is the right approach for CDP, I think it should also be used for BiDi, because at some point we're going to be turning on BiDi by default, and this would make that transition easier (refer to previous conversations with Simon).

Now we have a convenient factory method `lazy` for declaring lazy-initialized values like BiDi or DevTools. This simplifies BiDiProvider and DevToolsProvider code.

asolntsev · 2025-10-12T15:25:01Z

I still think we should add logging that we're doing it lazy now instead of on startup

Sure, we can add the logging.
Just to clarify: it was never done on "startup", it was done when user called new Augmenter().augment(webdriver).

So you are suggesting that during new Augmenter().augment(webdriver) call, Selenium will write done two new logs:

INFO [BiDiProvider] Add BiDi implementation - but BiDi connection will be establish only on a first usage
INFO [DevToolsProvider] Add DevTools implementation - but DevTools connection will be establish only on a first usage

Right? Do the texts sound good? Should it be "INFO" or "WARNING" or "DEBUG"?

P.S. If you really assume some users want to check-up all connections during augmentation, we could add an optional parameter to augment method:

Augmenter augmenter = new Augmenter();
WebDriver wd1 = augmenter.augment(webdriver); // lazy-initialized
WebDriver wd1 = augmenter.augment(webdriver, LAZY); // lazy-initialized
WebDriver wd2 = augmenter.augment(webdriver, EAGER); // checks all connections right now

// where the second parameter is:
enum InitializationMode {LAZY, EAGER}

titusfortner · 2025-10-12T16:14:41Z

I was thinking of when you use webdriver builder, it automatically augments and that's on startup.

The get implementation method is only called during augmentation and we already know it matched on is applicable. Info is good for this now since this is a behavior change. I only do warnings if I think it's something the user needs to change.

I would say something like "driver augmented with bidi/devtools interface, but the connection is not verified until first use."

If someone really needs to check right away, they can cast and call the get bidi command, right? No need to make the implementation more complicated with an optional parameter.

…mediately anymore

asolntsev · 2025-10-12T20:42:50Z

@titusfortner Ok, added the log:

LOG.log(INFO, "WebDriver augmented with DevTools interface; connection will not be verified until first use.");
LOG.log(INFO, "WebDriver augmented with BiDi interface; connection will not be verified until first use.");

diemol

Please run ./scripts/format.sh so we can get this merged. Thanks for the effort to explain your approach.

asolntsev · 2025-10-13T12:15:05Z

Please run ./scripts/format.sh so we can get this merged

Strange... I am sure I already executed ./scripts/format.sh before...

UPD Now I see the problem: this script always fails on my machine:

INFO: Running command line: bazel-bin/buildifier.bash
./.bazelbsp/aspects/rules/android/android_info.bzl:3:30: unexpected input character '$'
./.bazelbsp/aspects/rules/android/android_info.bzl # reformat

titusfortner

Looks good, thank you for the updates and your patience 😄

asolntsev marked this pull request as draft September 20, 2025 16:23

selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Sep 20, 2025

qodo-merge-pro bot added the Review effort 2/5 label Sep 20, 2025

asolntsev force-pushed the fix/devtools-augmentation branch from 7bf31b3 to 6607c45 Compare September 20, 2025 16:34

asolntsev marked this pull request as ready for review September 20, 2025 16:34

asolntsev requested review from pujagani and shs96c September 20, 2025 16:35

asolntsev self-assigned this Sep 20, 2025

asolntsev marked this pull request as draft September 20, 2025 16:54

asolntsev force-pushed the fix/devtools-augmentation branch from 6607c45 to 9e23bc1 Compare September 20, 2025 17:00

asolntsev marked this pull request as ready for review September 20, 2025 17:05

asolntsev force-pushed the fix/devtools-augmentation branch from 9e23bc1 to 7e7bcf8 Compare September 20, 2025 17:18

asolntsev requested a review from titusfortner September 20, 2025 18:40

asolntsev requested a review from nvborisenko September 24, 2025 11:08

titusfortner reviewed Oct 12, 2025

View reviewed changes

java/src/org/openqa/selenium/concurrent/Lazy.java Outdated Show resolved Hide resolved

titusfortner reviewed Oct 12, 2025

View reviewed changes

java/src/org/openqa/selenium/concurrent/Lazy.java Show resolved Hide resolved

extract reusable code to Lazy.java

8eab877

Now we have a convenient factory method `lazy` for declaring lazy-initialized values like BiDi or DevTools. This simplifies BiDiProvider and DevToolsProvider code.

asolntsev force-pushed the fix/devtools-augmentation branch from d17ad21 to 8eab877 Compare October 12, 2025 15:14

add log informing user that CDP/BiDi connection is not established im…

976ffda

…mediately anymore

asolntsev force-pushed the fix/devtools-augmentation branch from 268225f to 976ffda Compare October 12, 2025 20:42

diemol approved these changes Oct 13, 2025

View reviewed changes

reformat the code

05e9f41

asolntsev requested a review from titusfortner October 13, 2025 12:15

Merge branch 'trunk' into fix/devtools-augmentation

3382527

titusfortner approved these changes Oct 14, 2025

View reviewed changes

Merge branch 'trunk' into fix/devtools-augmentation

f439400

diemol merged commit 8c97d29 into SeleniumHQ:trunk Oct 15, 2025
3 of 4 checks passed

asolntsev deleted the fix/devtools-augmentation branch October 15, 2025 14:02

Uh oh!

make augmentation of HasBiDi/HasDevTools lazy-loaded #16338

make augmentation of HasBiDi/HasDevTools lazy-loaded #16338

Uh oh!

Conversation

asolntsev commented Sep 20, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

Context:

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Sep 20, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

qodo-merge-pro bot commented Sep 20, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

qodo-merge-pro bot commented Sep 20, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

joerg1985 commented Sep 21, 2025

Uh oh!

asolntsev commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joerg1985 commented Sep 22, 2025

Uh oh!

asolntsev commented Sep 22, 2025

Uh oh!

joerg1985 commented Sep 23, 2025

Uh oh!

asolntsev commented Sep 23, 2025

Uh oh!

joerg1985 commented Sep 23, 2025

Uh oh!

nvborisenko commented Sep 23, 2025

Uh oh!

asolntsev commented Sep 24, 2025

Uh oh!

Uh oh!

Uh oh!

titusfortner commented Oct 12, 2025

Uh oh!

asolntsev commented Oct 12, 2025

Uh oh!

titusfortner commented Oct 12, 2025

Uh oh!

asolntsev commented Oct 12, 2025

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

asolntsev commented Oct 13, 2025

Uh oh!

titusfortner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

asolntsev commented Sep 20, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Sep 20, 2025 •

edited

Loading

qodo-merge-pro bot commented Sep 20, 2025 •

edited

Loading

qodo-merge-pro bot commented Sep 20, 2025 •

edited

Loading

asolntsev commented Sep 21, 2025 •

edited

Loading