WebDriver: Keep the window alive when a process swap loses its signal - #10730
Open
sideshowbarker wants to merge 2 commits into
Open
WebDriver: Keep the window alive when a process swap loses its signal#10730sideshowbarker wants to merge 2 commits into
sideshowbarker wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughWebDriver now tracks announced and inferred WebContent replacements during asynchronous actions. It bounds replacement waits, removes windows with failed replacements, returns distinct errors, and tests recovery after WebContent or browser termination. ChangesWebDriver window replacement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebDriverClient
participant Session
participant WebContentConnection
participant Window
WebDriverClient->>Session: Start async action with replacement allowed
Session->>WebContentConnection: Track possible replacement connection
WebContentConnection->>Session: Close during action
Session->>Window: Mark replacement as inferred
Session->>Session: Wait and refetch window
Session-->>WebDriverClient: Return recovery or NoSuchWindow result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sideshowbarker
force-pushed
the
webdriver-keep-window-across-swap
branch
from
July 17, 2026 08:01
1eb5b3b to
1f3d50f
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
sideshowbarker
force-pushed
the
webdriver-keep-window-across-swap
branch
from
July 17, 2026 12:21
1f3d50f to
ad216ba
Compare
Collaborator
|
If an actual crash happens during traversal, aren't we now timing out instead of catching that immediately? Can we reduce the time it takes before we see that crash? |
Member
Author
hmm yeah, pretty much — it’s essentially a regression 😅
Yeah, I’ll go back and work on (re)implementing a proper fix for the original problem. |
sideshowbarker
force-pushed
the
webdriver-keep-window-across-swap
branch
from
July 27, 2026 12:10
ad216ba to
ad3ee1d
Compare
Member
Author
|
@gmta OK, pushed an update to get things working right. |
Problem: TestWebDriverSessionHistory intermittently hangs during a UI-initiated history traversal that swaps the WebContent process. The traverse-history-from-ui request never returns — so the test’s HTTP client times out, and the harness kills WebDriver. Cause: traverse_history_from_ui waits in perform_async_action for a driver-execution-complete signal from the current renderer. When the traversal replaces the process, that — and did_start_window_replacement preceding it — are fired by the old renderer from a callback that only runs after an async round-trip to the UI process. If the swap tears the old renderer down before that callback, both signals are lost: the Allow wait in perform_async_action can’t observe the loss, spins forever, and web_content_connection_closed sees no pending replacement. So it removes the window, clears the current window handle, and closes the session. Fix: While perform_async_action awaits a replacement-capable action, remember the connection it’s waiting on. If that closes first, treat the close as the start of the swap: web_content_connection_closed keeps the window awaiting its replacement instead of removing it — letting the incoming renderer reclaim the window, and letting the wait observe the replacement and return. Mark the traverse_history_from_ui handler as replacement-capable — so it takes this path; and navigate_to and perform_actions, already replacement-capable, get the same protection. Fixes LadybirdBrowser#10711
sideshowbarker
force-pushed
the
webdriver-keep-window-across-swap
branch
from
August 5, 2026 08:18
ad3ee1d to
61e5862
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Services/WebDriver/Session.cpp`:
- Around line 316-319: Update the inferred replacement timeout handling in the
window-replacement flow around the connection-close branch and its
five-second/page-load timeout wait so that an expired replacement transitions to
a terminal closed/error state or removes the affected window before returning
ErrorCode::Timeout. Ensure subsequent find_session() calls cannot rediscover the
window as awaiting replacement and repeat the timeout indefinitely.
In `@Services/WebDriver/Session.h`:
- Line 119: Update the replacement predicate in Session’s perform_async_action
flow to compare the current window’s WebContentConnection identity with the
captured connection, treating any non-null different connection as replacement;
preserve the existing window-removal and awaiting-replacement checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7628f609-7e26-48f0-beed-5f9c6ee907b8
📒 Files selected for processing (3)
Services/WebDriver/Client.cppServices/WebDriver/Session.cppServices/WebDriver/Session.h
Problem: A WebContent process swap can strand WebDriver in two ways. A command awaiting a replacement-capable action can spin forever after its renderer dies. And if a renderer dies with no replacement ever arriving, the command fails with “timeout” — and every later command stalls five seconds to fail the same way, for the life of the session. Cause: Window::AwaitingReplacement has no terminal transitions. If the replacement registers itself before the event loop dispatches the old connection’s close, that close matches no window — so the awaiting- replacement state watched for by the wait in perform_async_action is never entered, no response can arrive from the dead renderer, and the wait has no other exit. And the wait returns “timeout” but leaves the window awaiting its replacement with no connection when it’s in wait_for_current_window_to_have_web_content_connection and times out — So every later command re-enters the same wait, repeating the timeout. Fix: Give each wait a terminal outcome, with a single owner for each transition. The wait in perform_async_action now also exits when the current window is connected to a different connection than the one its action was sent to — and the command then completes the way an announced swap does. The replacement-wait timeout, as sole owner of the failure transition, removes the window — converging on the end state of an un- announced connection close: The waiting command fails with “no such window”, the spec’s error for a torn-down browsing context, and truthful for a crash that “timeout” misreported; the session closes when that window was its last — and later commands fail immediately, instead of repeating the wait. A replacement that registers in the same event-loop batch firing the timer still wins: The wait rechecks the window before finalizing. The now-unused ensure_current_window_handle_is_valid: gone.
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
TestWebDriverSessionHistoryintermittently hangs during a UI-initiated history traversal that swaps the WebContent process. Thetraverse-history-from-uirequest never returns — so the test’s HTTP client times out, and the harness kills WebDriver.Cause:
traverse_history_from_uiwaits inperform_async_actionfor adriver-execution-completesignal from the current renderer. When the traversal replaces the process, that — anddid_start_window_replacementpreceding it — are fired by the old renderer from a callback that only runs after an async round-trip to the UI process. If the swap tears the old renderer down before that callback, both signals are lost: theAllowwait inperform_async_actioncan’t observe the loss, spins forever, andweb_content_connection_closedsees no pending replacement. So it removes the window, clears the current window handle, and closes the session.Fix: While
perform_async_actionawaits a replacement-capable action, remember the connection it’s waiting on. If that closes first, treat the close as the start of the swap:web_content_connection_closedkeeps the window awaiting its replacement instead of removing it — letting the incoming renderer reclaim the window, and letting the wait observe the replacement and return. Mark thetraverse_history_from_uihandler as replacement-capable — so it takes this path; andnavigate_toandperform_actions, already replacement-capable, get the same protection. Fixes #10711.