WebDriver: Keep the window alive when a process swap loses its signal

Services/WebDriver/Client.cpp

@@ -342,7 +342,8 @@ Web::WebDriver::Response Client::traverse_history_from_ui(Web::WebDriver::Parame
     RefPtr previous_connection { &session->web_content_connection() };
     auto response = TRY(session->perform_async_action([&](auto& connection) {
         return connection.traverse_history_from_ui(move(payload));
-    }));
+    },
+        Session::WebContentReplacement::Allow));
     if (response.is_object() && response.as_object().get_bool("willReplaceWebContentProcess"sv).value_or(false))
         session->mark_current_window_as_awaiting_replacement(*previous_connection);
 

Services/WebDriver/Session.cpp

@@ -288,7 +288,7 @@ ErrorOr<void> Session::accept_web_content_transport(NonnullOwnPtr<IPC::Transport
 
         if (auto window = m_windows.find(window_handle); window != m_windows.end()) {
             window->value.web_content_connection = move(pending_connection);
-            window->value.is_awaiting_replacement = false;
+            window->value.awaiting_replacement = Window::AwaitingReplacement::No;
         } else {
             m_windows.set(window_handle, Session::Window { window_handle, move(pending_connection) });
         }
@@ -308,7 +308,13 @@ void Session::web_content_connection_closed(WebContentConnection const& connecti
         if (window.value.web_content_connection.ptr() != &connection)
             continue;
 
-        if (window.value.is_awaiting_replacement) {
+        if (window.value.is_awaiting_replacement()) {
+            window.value.web_content_connection = nullptr;
+            return;
+        }
+
+        if (&connection == m_connection_awaiting_possible_replacement) {
+            window.value.awaiting_replacement = Window::AwaitingReplacement::InferredFromClosedConnection;
             window.value.web_content_connection = nullptr;
             return;
         }
@@ -340,11 +346,11 @@ void Session::did_update_window_handle(String window_handle, WebContentConnectio
 
     auto window = maybe_window.release_value();
     window.handle = window_handle;
-    window.is_awaiting_replacement = false;
+    window.awaiting_replacement = Window::AwaitingReplacement::No;
 
     if (auto existing_window = m_windows.find(window_handle); existing_window != m_windows.end()) {
         existing_window->value.web_content_connection = move(window.web_content_connection);
-        existing_window->value.is_awaiting_replacement = false;
+        existing_window->value.awaiting_replacement = Window::AwaitingReplacement::No;
     } else {
         m_windows.set(window_handle, move(window));
     }
@@ -359,7 +365,7 @@ void Session::did_start_window_replacement(String const& window_handle, WebConte
     if (window == m_windows.end() || window->value.web_content_connection.ptr() != &connection)
         return;
 
-    window->value.is_awaiting_replacement = true;
+    window->value.awaiting_replacement = Window::AwaitingReplacement::Announced;
     window->value.web_content_connection = nullptr;
 }
 
@@ -369,7 +375,7 @@ void Session::mark_current_window_as_awaiting_replacement(WebContentConnection c
     if (window == m_windows.end() || window->value.web_content_connection.ptr() != &connection)
         return;
 
-    window->value.is_awaiting_replacement = true;
+    window->value.awaiting_replacement = Window::AwaitingReplacement::Announced;
     window->value.web_content_connection = nullptr;
 }
 
@@ -537,18 +543,6 @@ Web::WebDriver::Response Session::get_window_handles() const
     return JsonValue { move(handles) };
 }
 
-ErrorOr<void, Web::WebDriver::Error> Session::ensure_current_window_handle_is_valid() const
-{
-    auto current_window = m_windows.get(m_current_window_handle);
-    if (!current_window.has_value())
-        return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchWindow, "Window not found"sv);
-
-    if (!current_window->web_content_connection)
-        return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::UnknownError, "Window is waiting for a replacement WebContent process"sv);
-
-    return {};
-}
-
 ErrorOr<bool, Web::WebDriver::Error> Session::wait_for_current_window_to_have_web_content_connection()
 {
     m_event_loop.pump(Core::EventLoop::WaitMode::PollForEvents);
@@ -560,6 +554,9 @@ ErrorOr<bool, Web::WebDriver::Error> Session::wait_for_current_window_to_have_we
     if (current_window->web_content_connection)
         return false;
 
+    static constexpr u64 INFERRED_REPLACEMENT_TIMEOUT_MS = 5000;
+    auto replacement_was_inferred = current_window->awaiting_replacement == Window::AwaitingReplacement::InferredFromClosedConnection;
+
     Optional<u64> page_load_timeout = Web::WebDriver::TimeoutsConfiguration {}.page_load_timeout;
     if (m_timeouts_configuration.has_value() && m_timeouts_configuration->is_object()) {
         if (auto value = m_timeouts_configuration->as_object().get("pageLoad"sv); value.has_value()) {
@@ -569,6 +566,8 @@ ErrorOr<bool, Web::WebDriver::Error> Session::wait_for_current_window_to_have_we
                 page_load_timeout = value->get_integer<u64>().value_or(*page_load_timeout);
         }
     }
+    if (replacement_was_inferred)
+        page_load_timeout = min(page_load_timeout.value_or(INFERRED_REPLACEMENT_TIMEOUT_MS), INFERRED_REPLACEMENT_TIMEOUT_MS);
 
     bool timed_out = false;
     RefPtr<Core::Timer> timer;
@@ -588,11 +587,30 @@ ErrorOr<bool, Web::WebDriver::Error> Session::wait_for_current_window_to_have_we
     if (timer)
         timer->stop();
 
-    if (timed_out)
-        return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::Timeout, "Timed out waiting for replacement WebContent process"sv);
+    // Refetch the window — rather than trusting timed_out: If the replacement registered in the same event-loop batch
+    // that fired the timer, its arrival wins over the timeout.
+    current_window = m_windows.get(m_current_window_handle);
+    if (!current_window.has_value())
+        return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchWindow, "Window not found"sv);
+    if (current_window->web_content_connection)
+        return true;
+    VERIFY(timed_out);
+
+    // The replacement this window was waiting for never arrived — and nothing else will ever connect a WebContent
+    // process to this window. So, without a transition here, every later command would reach this same wait, and repeat
+    // this same timeout — for the life of the session. This timeout is the sole owner of that failure transition:
+    // Remove the window — converging on the end state an unannounced connection close has. Later commands then observe
+    // an absent window — for which the WebDriver spec prescribes the error in every command's step 1; e.g., from
+    // https://w3c.github.io/webdriver/#get-current-url:
+    // 1. If the current top-level browsing context is no longer open, return error with error code no such window.
+    auto window_handle = m_current_window_handle;
+    remove_window(window_handle);
 
-    TRY(ensure_current_window_handle_is_valid());
-    return true;
+    if (replacement_was_inferred)
+        return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchWindow,
+            MUST(String::formatted("The window's WebContent process disconnected and was not replaced within {} ms", *page_load_timeout)));
+    return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchWindow,
+        MUST(String::formatted("The window's replacement WebContent process did not connect within {} ms", *page_load_timeout)));
 }
 
 }

Services/WebDriver/Session.h

@@ -46,9 +46,17 @@ class Session : public RefCounted<Session> {
     static void close_all();
 
     struct Window {
+        enum class AwaitingReplacement {
+            No,
+            Announced,
+            InferredFromClosedConnection,
+        };
+
         String handle;
         RefPtr<WebContentConnection> web_content_connection;
-        bool is_awaiting_replacement { false };
+        AwaitingReplacement awaiting_replacement { AwaitingReplacement::No };
+
+        bool is_awaiting_replacement() const { return awaiting_replacement != AwaitingReplacement::No; }
     };
 
     WebContentConnection& web_content_connection() const
@@ -73,7 +81,6 @@ class Session : public RefCounted<Session> {
     Web::WebDriver::Response close_window();
     Web::WebDriver::Response switch_to_window(StringView);
     Web::WebDriver::Response get_window_handles() const;
-    ErrorOr<void, Web::WebDriver::Error> ensure_current_window_handle_is_valid() const;
     ErrorOr<bool, Web::WebDriver::Error> wait_for_current_window_to_have_web_content_connection();
     void mark_current_window_as_awaiting_replacement(WebContentConnection const&);
 
@@ -88,7 +95,14 @@ class Session : public RefCounted<Session> {
         Optional<Web::WebDriver::Response> response;
         RefPtr connection { &web_content_connection() };
 
-        ScopeGuard guard { [&]() { connection->on_driver_execution_complete = nullptr; } };
+        auto previous_connection_awaiting_replacement = m_connection_awaiting_possible_replacement;
+        if (web_content_replacement == WebContentReplacement::Allow)
+            m_connection_awaiting_possible_replacement = connection.ptr();
+
+        ScopeGuard guard { [&]() {
+            connection->on_driver_execution_complete = nullptr;
+            m_connection_awaiting_possible_replacement = previous_connection_awaiting_replacement;
+        } };
         connection->on_driver_execution_complete = [&](auto result) { response = move(result); };
 
         TRY(action(*connection));
@@ -101,7 +115,17 @@ class Session : public RefCounted<Session> {
                 return false;
 
             auto current_window = m_windows.get(m_current_window_handle);
-            return !current_window.has_value() || (current_window->is_awaiting_replacement && !current_window->web_content_connection);
+            if (!current_window.has_value())
+                return true;
+
+            // A replacement WebContent process can register itself with this session before the event loop dispatches
+            // the closing of the connection this action was sent to. That close matches no window – so, the awaiting-
+            // replacement state this wait watches for is never entered. The current window being connected to another
+            // connection is equally proof that no response will ever arrive from the connection this was sent to.
+            if (current_window->web_content_connection)
+                return current_window->web_content_connection.ptr() != connection.ptr();
+
+            return current_window->is_awaiting_replacement();
         });
 
         if (response.has_value())
@@ -137,6 +161,8 @@ class Session : public RefCounted<Session> {
     HashMap<String, Window> m_windows;
     String m_current_window_handle;
 
+    WebContentConnection const* m_connection_awaiting_possible_replacement { nullptr };
+
     HashMap<u64, NonnullRefPtr<WebContentConnection>> m_pending_connections;
     u64 m_next_pending_connection_id { 0 };
 

Tests/LibWebView/test-webdriver-session-history.py

@@ -9,6 +9,7 @@
 import http.server
 import json
 import os
+import signal
 import socket
 import subprocess
 import sys
@@ -2674,6 +2675,128 @@ def run_provisional_navigation_browser_ui_back_tests(
     )
 
 
+def find_browser_pid_for_session(session_id):
+    for _ in range(50):
+        pgrep = subprocess.run(["pgrep", "-f", session_id], capture_output=True, text=True)
+        pids = [int(pid) for pid in pgrep.stdout.split()]
+        if len(pids) > 1:
+            raise AssertionError(f"Expected one browser process for session {session_id}, found pids: {pids}")
+        if len(pids) == 1:
+            return pids[0]
+        time.sleep(0.1)
+    raise AssertionError(f"Did not find a browser process for session {session_id}")
+
+
+def find_web_content_pid(browser_pid):
+    for _ in range(50):
+        pgrep = subprocess.run(["pgrep", "-P", str(browser_pid), "-f", "WebContent"], capture_output=True, text=True)
+        pids = [int(pid) for pid in pgrep.stdout.split()]
+        if len(pids) > 1:
+            raise AssertionError(f"Expected one WebContent child of browser {browser_pid}, found pids: {pids}")
+        if len(pids) == 1:
+            return pids[0]
+        time.sleep(0.1)
+    raise AssertionError(f"Did not find a WebContent child of browser process {browser_pid}")
+
+
+def long_pause_actions_payload():
+    return {
+        "actions": [
+            {
+                "type": "none",
+                "id": "pause-input",
+                "actions": [{"type": "pause", "duration": 20000}],
+            }
+        ]
+    }
+
+
+def post_actions_in_thread(webdriver_port, session_id):
+    result = {}
+
+    def post_actions():
+        try:
+            result["response"] = request_raw(
+                webdriver_port, "POST", f"/session/{session_id}/actions", long_pause_actions_payload()
+            )
+        except Exception as error:
+            result["error"] = error
+
+    actions_thread = threading.Thread(target=post_actions, daemon=True)
+    actions_thread.start()
+    return actions_thread, result
+
+
+def run_second_ui_forward_during_pending_forward_test(webdriver_port, page_server, url_a, url_forward_blocked, url_c):
+    session_id = create_session(webdriver_port)
+    expect_second_ui_forward_during_pending_forward_does_not_hang(
+        webdriver_port,
+        session_id,
+        page_server,
+        url_a,
+        url_forward_blocked,
+        url_c,
+    )
+    request(webdriver_port, "DELETE", f"/session/{session_id}")
+
+
+def run_unannounced_web_content_death_tests(webdriver_port, url_a):
+    run_unannounced_web_content_death_recovery_test(webdriver_port, url_a)
+    run_unannounced_web_content_death_without_replacement_test(webdriver_port)
+
+
+def run_unannounced_web_content_death_recovery_test(webdriver_port, url_a):
+    session_id = create_session(webdriver_port)
+    log = [f"unannounced WebContent death recovery initial: {current_url(webdriver_port, session_id)}"]
+    request(webdriver_port, "POST", f"/session/{session_id}/url", {"url": url_a})
+    browser_pid = find_browser_pid_for_session(session_id)
+    web_content_pid = find_web_content_pid(browser_pid)
+
+    actions_thread, result = post_actions_in_thread(webdriver_port, session_id)
+    time.sleep(2)
+    os.kill(web_content_pid, signal.SIGKILL)
+    actions_thread.join(timeout=45)
+    if actions_thread.is_alive():
+        raise AssertionError("Perform Actions never returned after its WebContent process was killed")
+    if "error" in result:
+        raise AssertionError(f"Perform Actions failed after its WebContent process was killed: {result['error']}")
+    status, _, response_body = result["response"]
+    if status != 200:
+        raise AssertionError(f"Perform Actions after WebContent death returned HTTP {status}: {response_body}")
+
+    wait_for_url(webdriver_port, session_id, "after unannounced WebContent death recovery", url_a, log)
+    request(webdriver_port, "DELETE", f"/session/{session_id}")
+
+
+def run_unannounced_web_content_death_without_replacement_test(webdriver_port):
+    session_id = create_session(webdriver_port)
+    browser_pid = find_browser_pid_for_session(session_id)
+
+    actions_thread, result = post_actions_in_thread(webdriver_port, session_id)
+    time.sleep(2)
+    os.kill(browser_pid, signal.SIGKILL)
+    actions_thread.join(timeout=45)
+    if actions_thread.is_alive():
+        raise AssertionError("Perform Actions never returned after the browser was killed")
+    if "error" in result:
+        raise AssertionError(f"Perform Actions failed after the browser was killed: {result['error']}")
+    status, payload, response_body = result["response"]
+    value = payload.get("value")
+    error = value.get("error") if isinstance(value, dict) else None
+    if error != "no such window":
+        raise AssertionError(
+            f"Expected 'no such window' after the browser was killed mid-command, got HTTP {status}: {response_body}"
+        )
+
+    status, payload, response_body = request_raw(webdriver_port, "GET", f"/session/{session_id}/url")
+    value = payload.get("value")
+    error = value.get("error") if isinstance(value, dict) else None
+    if error != "invalid session id":
+        raise AssertionError(
+            f"Expected 'invalid session id' after the session's browser died, got HTTP {status}: {response_body}"
+        )
+
+
 def run_test(webdriver_binary):
     page_server = TestPageServer(("0.0.0.0", 0), TestPageHandler)
     page_server_thread = threading.Thread(target=page_server.serve_forever, daemon=True)
@@ -2757,17 +2880,11 @@ def run_test(webdriver_binary):
             url_cross_site_navigation_blocked,
         )
 
-        session_id = create_session(webdriver_port)
-        expect_second_ui_forward_during_pending_forward_does_not_hang(
-            webdriver_port,
-            session_id,
-            page_server,
-            url_a,
-            url_forward_blocked,
-            url_c,
+        run_second_ui_forward_during_pending_forward_test(
+            webdriver_port, page_server, url_a, url_forward_blocked, url_c
         )
-        request(webdriver_port, "DELETE", f"/session/{session_id}")
-        session_id = None
+
+        run_unannounced_web_content_death_tests(webdriver_port, url_a)
 
         session_id = create_session(webdriver_port)
         log = [f"first-entry replace initial: {current_url(webdriver_port, session_id)}"]