[java][bidi] Handle user prompts during BiDi navigation #17244
[java][bidi] Handle user prompts during BiDi navigation #17244krishnamohan-kothapalli wants to merge 13 commits into
Conversation
Implements get(), back(), forward(), and refresh() via BrowsingContext when webSocketUrl capability is present, falling back to Classic otherwise. Maps pageLoadStrategy to ReadinessState: normal->COMPLETE, eager->INTERACTIVE, none->NONE. Adds integration tests covering all navigation commands with BiDi enabled. Fixes SeleniumHQ#13995
- Guard BiDi path with instanceof HasBiDi check to prevent IllegalArgumentException on plain RemoteWebDriver instances - Check webSocketUrl instanceof String (not just != null) to avoid false positive on Boolean.TRUE during session setup - Handle PageLoadStrategy as both enum and String when mapping to ReadinessState to fix eager/none being silently ignored Addresses review feedback on SeleniumHQ#13995
Review Summary by QodoHandle user prompts during BiDi navigation with automatic dismissal
WalkthroughsDescription• Handle user prompts during BiDi navigation via listener • Route navigation commands (get, back, forward, refresh) through BiDi • Map pageLoadStrategy to ReadinessState for BiDi compatibility • Automatically dismiss or accept alerts per unhandledPromptBehavior capability File Changes1. java/src/org/openqa/selenium/remote/RemoteWebDriver.java
|
Code Review by Qodo
1.
|
…t interference Replace the permanent session-scoped browsingContext.userPromptOpened listener (biDiPromptListenerInstalled + biDiNavigatingContextId gate) with a per-navigation register/deregister pattern. The listener is now installed via BiDi.addListener() scoped to the navigating context immediately before navigation runs, then removed via BiDi.removeListener(id) in a finally block. This ensures the listener cannot fire for user-triggered alerts that appear after page load, which was causing AlertsTest, FrameSwitchingTest, HistoryNavigationTest, and PageLoadingTest failures. Also fix import ordering: move json imports after io (alphabetical), remove the now-unused AtomicBoolean import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit cfed07f |
…licies navigateViaBiDi previously collapsed ACCEPT_AND_NOTIFY and DISMISS_AND_NOTIFY into the same silent behaviour as ACCEPT/DISMISS. Since DISMISS_AND_NOTIFY is the W3C spec default, callers were never notified when a prompt was auto-handled. Fix: compute a `notify` flag alongside `accept`. When notify is true, capture the first handled UserPromptOpened in an AtomicReference. After navigation succeeds (listener already removed), throw UnhandledAlertException with the alert text so callers observe the same behaviour as classic WebDriver. Navigation exceptions take precedence — the notify throw only happens on a clean navigation run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit 0f63055 |
Two related issues: 1. The volatile field declaration was lost during the trunk merge rebase; re-add it. volatile is required so that the outer unsynchronized read in the double-checked locking pattern in getPageLoadDuration() always sees the latest value. 2. pageLoadTimeout() was writing biDiPageLoadTimeout outside any lock, creating a race: if getPageLoadDuration()'s lazy-init synchronized block ran concurrently and completed after the user's write, it would silently overwrite the user-set value with the GET_TIMEOUTS result. Fix by synchronizing the write on RemoteWebDriver.this, the same monitor used by getPageLoadDuration()'s inner block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit 30d2ffb |
…on null safety Two compile/nullability issues: 1. Json and JsonInput are used by USER_PROMPT_OPENED_EVENT but their imports were absent from the committed HEAD (lost in a prior rebase). Re-add them in alphabetical order (io → json → logging). 2. getPageLoadDuration() returned biDiPageLoadTimeout (@nullable Duration) directly as the non-nullable Duration return type, violating the package-level @NullMarked contract. Switch to the canonical local-variable DCL pattern: read the volatile field once into 'cached', initialize inside the synchronized block and assign back to both 'cached' and the field, then return 'cached'. This keeps the return type non-nullable and avoids the extra volatile read on the hot path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit c1aff62 |
…Di navigation BiDi.addListener always sends session.subscribe, while removeListener only removes the local callback without session.unsubscribe. The per-navigation listener approach therefore accumulated remote subscriptions on every call to get/back/forward/refresh with no corresponding unsubscribe. Fix by switching to a single session-scoped listener installed lazily on the first non-IGNORE navigation (ensureBiDiPromptListener). The listener is gated on biDiNavigatingContextId (non-null only while a navigation is in progress) so it has no effect on user-triggered alerts outside of navigation — preserving the behaviour that fixed the AlertsTest/FrameSwitchingTest regressions. Thread-safety: biDiAcceptPrompt, biDiNotifyOnPrompt, and biDiHandledPrompt are written before the volatile write of biDiNavigatingContextId. The listener's volatile read of biDiNavigatingContextId establishes happens-before with those prior writes, so no additional synchronisation is needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit b42bd10 |
… retry ensureBiDiPromptListener() set biDiPromptListenerInstalled before calling addListener/session.subscribe. If the subscribe threw (transient network error, browser rejection), the flag stayed true and the listener could never be installed or retried, permanently disabling prompt handling for the session. Fix: wrap addListener in try/catch and reset the flag on any RuntimeException so the next navigation attempt retries the subscription. The exception still propagates — the navigation fails cleanly rather than silently proceeding without a handler. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit aa400a8 |
🔗 Related Issues
💥 What does this PR do?
When navigating via BiDi (
get(),back(),forward(),refresh()), anonloadalert blocks the page from reaching its readiness state, causingbrowsingContext.navigate/traverseHistory/reloadto hang indefinitely.This fix registers a
browsingContext.userPromptOpenedlistener before eachBiDi navigation that automatically dismisses or accepts prompts according to
the
unhandledPromptBehaviorcapability (defaulting todismiss and notify).The listener is removed in a
finallyblock after navigation completes.🔧 Implementation Notes
Classic WebDriver delegated prompt handling during navigation to the browser
via the
unhandledPromptBehaviorsession capability. BiDi navigation commandsdon't have that mechanism, so we replicate it by subscribing to
browsingContext.userPromptOpenedfor the duration of each navigation call.The listener runs on the BiDi WebSocket receiver thread while
sendAndWaitblocks the caller thread — the
Connectionimplementation already handlesthis pattern safely (see the
tryLockcomment inhandleEventResponse).IGNOREbehaviour is preserved: navigation runs without a listener,leaving the alert open for the caller to handle.
💡 Additional Considerations
to replicate this behaviour for their BiDi navigation paths.
🔄 Types of changes