[js] Ensure BiDi is not exposed on Driver

javascript/selenium-webdriver/BUILD.bazel

@@ -200,6 +200,7 @@ SMALL_TESTS = [
     "test/bidi/index_test.js",
     "test/io/io_test.js",
     "test/io/zip_test.js",
+    "test/lib/bidi_connection_test.js",
     "test/lib/by_test.js",
     "test/lib/credentials_test.js",
     "test/lib/error_test.js",

javascript/selenium-webdriver/lib/bidi_connection.js

@@ -0,0 +1,84 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+const BiDi = require('../bidi')
+
+/**
+ * One BiDi connection per driver, kept outside the WebDriver class itself so
+ * it isn't a discoverable method on it — see
+ * docs/decisions/17670-bidi-implementation-boundaries.md. Not re-exported from
+ * the package's public entry point.
+ *
+ * Stores the in-flight promise, not the resolved connection, so a second
+ * concurrent call for the same driver awaits the same creation instead of
+ * racing its own and leaking whichever connection loses.
+ * @type {WeakMap<object, Promise<BiDi>>}
+ */
+const connections = new WeakMap()
+
+/**
+ * Returns the BiDi connection for `driver`, creating it on first access.
+ * @param {object} driver The WebDriver instance to obtain a BiDi connection for.
+ * @returns {Promise<BiDi>} A promise resolving to `driver`'s BiDi connection, shared
+ *     with any other in-flight or already-resolved call for the same driver.
+ */
+function getBidiConnection(driver) {
+  if (!connections.has(driver)) {
+    connections.set(driver, createConnection(driver))
+  }
+  return connections.get(driver)
+}
+
+/**
+ * @param {object} driver
+ * @returns {Promise<BiDi>}
+ */
+async function createConnection(driver) {
+  const caps = await driver.getCapabilities()
+  const webSocketUrl = caps['map_'].get('webSocketUrl')
+  if (!webSocketUrl) {
+    throw new Error('WebDriver instance must support BiDi protocol')
+  }
+  return new BiDi(webSocketUrl.replace('localhost', '127.0.0.1'))
+}
+
+/**
+ * Closes `driver`'s BiDi connection, if one was ever opened. A no-op
+ * otherwise — must not lazily create a connection just to close it.
+ *
+ * Callers (e.g. quit()) invoke this fire-and-forget, so it must never reject:
+ * if the original connection attempt itself had failed, `pending` is already
+ * rejected and there is nothing live to close.
+ * @param {object} driver The WebDriver instance whose BiDi connection should be closed.
+ * @returns {Promise<void>} A promise that always resolves, once any open connection
+ *     has been closed (or immediately, if none was ever opened).
+ */
+async function closeBidiConnection(driver) {
+  const pending = connections.get(driver)
+  if (pending === undefined) {
+    return
+  }
+  connections.delete(driver)
+  try {
+    const connection = await pending
+    await connection.close()
+  } catch {
+    // Nothing to close — the original connection attempt failed.
+  }
+}
+
+module.exports = { getBidiConnection, closeBidiConnection }

javascript/selenium-webdriver/lib/logging.js

@@ -271,6 +271,9 @@ class Logger {
 
     /** @private {Set<function(!Entry)>} */
     this.handlers_ = null
+
+    /** @private {Set<string>} ids already reported via {@link #deprecate}. */
+    this.deprecated_ = new Set()
   }
 
   /** @return {string} the name of this logger. */
@@ -392,6 +395,38 @@ class Logger {
     this.log(Level.INFO, loggable)
   }
 
+  /**
+   * Logs a deprecation notice at the {@link Level.WARNING} log level, once per
+   * `id` for this logger's lifetime — a repeat call with the same `id` is a
+   * no-op, matching the once-only behavior `util.deprecate` gives by call-site
+   * identity, but keyed on a stable id instead so it survives being wrapped,
+   * rebound, or called through multiple paths.
+   *
+   * `id` is only claimed once the notice is actually loggable at this
+   * logger's effective level — under the default `Level.OFF` root level, a
+   * call here logs nothing and leaves `id` unclaimed, so a later call (once
+   * logging is enabled) still gets to report it instead of finding it already
+   * silently used up.
+   * @param {string} id a stable, non-empty identifier for this deprecation
+   *     (e.g. `'webdriver-getBidi'`), distinct from the message text so
+   *     tooling can key off it even if the wording changes later.
+   * @param {string} message the deprecation notice to log.
+   * @throws {TypeError} if `id` is empty.
+   */
+  deprecate(id, message) {
+    if (!id) {
+      throw new TypeError('Logger#deprecate() requires a non-empty id')
+    }
+    if (this.deprecated_.has(id)) {
+      return
+    }
+    if (!this.isLoggable(Level.WARNING)) {
+      return
+    }
+    this.deprecated_.add(id)
+    this.warning(`[${id}] ${message}`)
+  }
+
   /**
    * Logs a message at the {@link Level.DEBUG} log level.
    * @param {(string|function(): string)} loggable the message to log, or a

javascript/selenium-webdriver/lib/webdriver.js

@@ -40,7 +40,7 @@ const cdpTargets = ['page', 'browser']
 const { Credential } = require('./virtual_authenticator')
 const webElement = require('./webelement')
 const { isObject } = require('./util')
-const BIDI = require('../bidi')
+const { getBidiConnection, closeBidiConnection } = require('./bidi_connection')
 const { PinnedScript } = require('./pinnedScript')
 const JSZip = require('jszip')
 const Script = require('./script')
@@ -691,6 +691,16 @@ class WebDriver {
     this.pinnedScripts_ = {}
   }
 
+  /**
+   * The shared logger deprecation notices (e.g. {@link WebDriver#getBidi})
+   * are emitted through — a class-level accessor since a deprecation can be
+   * reported from a static/prototype context with no driver instance at hand.
+   * @return {!./logging.Logger} the shared `selenium.webdriver.webdriver` logger.
+   */
+  static get logger() {
+    return logging.getLogger('selenium.webdriver.webdriver')
+  }
+
   /**
    * Creates a new WebDriver session.
    *
@@ -794,10 +804,9 @@ class WebDriver {
         this._cdpWsConnection.close()
       }
 
-      // Close the BiDi websocket connection
-      if (this._bidiConnection !== undefined) {
-        this._bidiConnection.close()
-      }
+      // Not awaited: the session is already torn down by this point, so
+      // closing our end of the socket doesn't need to gate quit() completing.
+      closeBidiConnection(this)
     })
   }
 
@@ -1312,19 +1321,6 @@ class WebDriver {
     this._cdpConnection.execute('Target.getTargets')
   }
 
-  /**
-   * Initiates bidi connection using 'webSocketUrl'
-   * @returns {BIDI}
-   */
-  async getBidi() {
-    if (this._bidiConnection === undefined) {
-      const caps = await this.getCapabilities()
-      let WebSocketUrl = caps['map_'].get('webSocketUrl')
-      this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1'))
-    }
-    return this._bidiConnection
-  }
-
   /**
    * Retrieves 'webSocketDebuggerUrl' by sending a http request using debugger address
    * @param {string} debuggerAddress
@@ -1789,6 +1785,36 @@ class WebDriver {
   }
 }
 
+/**
+ * Returns the WebDriver BiDi connection for this session.
+ *
+ * @deprecated BiDi is an internal implementation detail — this accessor hands
+ * back the raw transport directly, which is no longer supported public API.
+ * Use a composed BiDi module instead, e.g. `Network.create(driver)` or
+ * `require('selenium-webdriver/bidi/network')`.
+ * @function
+ * @name WebDriver#getBidi
+ * @returns {Promise<import('../bidi')>} A promise resolving to this session's raw
+ *     BiDi connection, opened on first access and reused afterward.
+ */
+// Object.defineProperty, not `WebDriver.prototype.getBidi = function () {...}`:
+// a plain assignment creates an enumerable property, but a method declared in
+// the class body (like every other method here) is non-enumerable — so BiDi
+// would become more discoverable off the driver than it was before.
+Object.defineProperty(WebDriver.prototype, 'getBidi', {
+  value: function () {
+    WebDriver.logger.deprecate(
+      'webdriver-getBidi',
+      'WebDriver#getBidi() is deprecated. Use a composed BiDi module instead, e.g. Network.create(driver) or ' +
+        "require('selenium-webdriver/bidi/network').",
+    )
+    return getBidiConnection(this)
+  },
+  writable: true,
+  enumerable: false,
+  configurable: true,
+})
+
 /**
  * Interface for navigating back and forth in the browser history.
  *

javascript/selenium-webdriver/test/lib/bidi_connection_test.js

@@ -0,0 +1,126 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+'use strict'
+
+const assert = require('node:assert')
+const { WebSocketServer } = require('ws')
+const { getBidiConnection, closeBidiConnection } = require('selenium-webdriver/lib/bidi_connection')
+const { WebDriver } = require('selenium-webdriver/lib/webdriver')
+const { Session } = require('selenium-webdriver/lib/session')
+
+function startEchoServer() {
+  return new Promise((resolve) => {
+    const server = new WebSocketServer({ port: 0 }, () => {
+      const { port } = server.address()
+      resolve({ server, url: `ws://127.0.0.1:${port}` })
+    })
+    server.on('connection', (ws) => {
+      ws.on('message', (data) => {
+        const { id } = JSON.parse(data.toString())
+        ws.send(JSON.stringify({ id, result: {} }))
+      })
+    })
+  })
+}
+
+describe('bidi_connection', function () {
+  let server
+
+  afterEach(async function () {
+    if (server !== undefined) {
+      await new Promise((resolve) => server.close(resolve))
+      server = undefined
+    }
+  })
+
+  it('creates exactly one BiDi connection under concurrent first access', async function () {
+    const started = await startEchoServer()
+    server = started.server
+
+    let capabilitiesCalls = 0
+    const driver = {
+      async getCapabilities() {
+        capabilitiesCalls++
+        // Yield the event loop before resolving, so concurrent callers race.
+        await new Promise((resolve) => setTimeout(resolve, 20))
+        return { map_: new Map([['webSocketUrl', started.url]]) }
+      },
+    }
+
+    const [a, b, c] = await Promise.all([
+      getBidiConnection(driver),
+      getBidiConnection(driver),
+      getBidiConnection(driver),
+    ])
+
+    assert.strictEqual(capabilitiesCalls, 1, 'getCapabilities() should only be called once')
+    assert.strictEqual(a, b)
+    assert.strictEqual(b, c)
+
+    await closeBidiConnection(driver)
+  })
+
+  it('closeBidiConnection() is a no-op when no connection was ever opened', async function () {
+    const driver = {
+      async getCapabilities() {
+        throw new Error('should not be called')
+      },
+    }
+    await assert.doesNotReject(closeBidiConnection(driver))
+  })
+
+  it('closeBidiConnection() does not reject when the original connection attempt failed', async function () {
+    const driver = {
+      async getCapabilities() {
+        throw new Error('driver does not support BiDi')
+      },
+    }
+    await assert.rejects(getBidiConnection(driver), /does not support BiDi/)
+    // quit() calls this fire-and-forget (no await/catch at the call site), so
+    // it must swallow the earlier failure rather than re-throwing it.
+    await assert.doesNotReject(closeBidiConnection(driver))
+  })
+
+  it('quit() closes the BiDi connection stored outside the driver instance', async function () {
+    const started = await startEchoServer()
+    server = started.server
+
+    // Resolves only once the server observes the client actually closing the
+    // socket — if quit() stopped delegating to closeBidiConnection(), this
+    // would never resolve and the test would time out rather than pass.
+    const serverSawClose = new Promise((resolve) => {
+      server.on('connection', (ws) => ws.on('close', () => resolve(true)))
+    })
+
+    const session = new Session('test-session-id', { webSocketUrl: started.url })
+    const driver = new WebDriver(session, { execute: async () => null })
+
+    // Open the connection the same way a composed BiDi module would (via the
+    // driver, not by calling getBidiConnection/closeBidiConnection directly),
+    // so there is a real connection for quit() to close. getBidiConnection()
+    // resolves as soon as the BiDi instance is constructed, not once its
+    // websocket handshake actually completes — wait for that too, or closing
+    // immediately can race the handshake and the server never sees 'connection'.
+    const connection = await getBidiConnection(driver)
+    await connection.waitForConnection()
+
+    await driver.quit()
+
+    assert.strictEqual(await serverSawClose, true, "quit() should close the driver's BiDi connection")
+  })
+})