From bf0bc62654803565a6f39ade63d9172cc48c085a Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 25 Oct 2024 10:28:29 -0400 Subject: [PATCH] WebContent+WebDriver: Asynchronously wait for navigations to complete Similar to commit c2cf65adac78912883996153fb608dafe389b6e0, we should avoid spinning the event loop from the WebContent-side of the WebDriver connection. This can result in deadlocks if another component in LibWeb also spins the event loop. The AO to await navigations has two event loop spinners - waiting for the navigation to complete and for the document to reach the target readiness state. We now use NavigationObserver and DocumentObserver to be notified when these conditions are met. And we use the same async IPC mechanism as script execution to notify the WebDriver process when all conditions are met (or timed out). --- .../Libraries/LibWeb/WebDriver/HeapTimer.cpp | 9 + .../Libraries/LibWeb/WebDriver/HeapTimer.h | 1 + Userland/Services/WebContent/PageClient.cpp | 9 + .../WebContent/WebDriverConnection.cpp | 162 ++++++++++++------ .../Services/WebContent/WebDriverConnection.h | 9 +- .../Services/WebContent/WebDriverServer.ipc | 1 + Userland/Services/WebDriver/Client.cpp | 2 +- Userland/Services/WebDriver/Session.cpp | 7 + Userland/Services/WebDriver/Session.h | 2 + .../WebDriver/WebContentConnection.cpp | 6 + .../Services/WebDriver/WebContentConnection.h | 2 + 11 files changed, 157 insertions(+), 53 deletions(-) diff --git a/Userland/Libraries/LibWeb/WebDriver/HeapTimer.cpp b/Userland/Libraries/LibWeb/WebDriver/HeapTimer.cpp index 954736728f3..25ba7b85155 100644 --- a/Userland/Libraries/LibWeb/WebDriver/HeapTimer.cpp +++ b/Userland/Libraries/LibWeb/WebDriver/HeapTimer.cpp @@ -42,6 +42,15 @@ void HeapTimer::start(u64 timeout_ms, JS::NonnullGCPtr> m_timer->start(); } +void HeapTimer::stop_and_fire_timeout_handler() +{ + auto on_timeout = m_on_timeout; + stop(); + + if (on_timeout) + on_timeout->function()(); +} + void HeapTimer::stop() { m_on_timeout = nullptr; diff --git a/Userland/Libraries/LibWeb/WebDriver/HeapTimer.h b/Userland/Libraries/LibWeb/WebDriver/HeapTimer.h index 69749c6966a..3a5800cf20c 100644 --- a/Userland/Libraries/LibWeb/WebDriver/HeapTimer.h +++ b/Userland/Libraries/LibWeb/WebDriver/HeapTimer.h @@ -21,6 +21,7 @@ public: virtual ~HeapTimer() override; void start(u64 timeout_ms, JS::NonnullGCPtr> on_timeout); + void stop_and_fire_timeout_handler(); void stop(); bool is_timed_out() const { return m_timed_out; } diff --git a/Userland/Services/WebContent/PageClient.cpp b/Userland/Services/WebContent/PageClient.cpp index 5e3da2cae17..b3af662a03a 100644 --- a/Userland/Services/WebContent/PageClient.cpp +++ b/Userland/Services/WebContent/PageClient.cpp @@ -389,6 +389,9 @@ void PageClient::page_did_request_media_context_menu(Web::CSSPixelPoint content_ void PageClient::page_did_request_alert(String const& message) { client().async_did_request_alert(m_id, message); + + if (m_webdriver) + m_webdriver->page_did_open_dialog({}); } void PageClient::alert_closed() @@ -399,6 +402,9 @@ void PageClient::alert_closed() void PageClient::page_did_request_confirm(String const& message) { client().async_did_request_confirm(m_id, message); + + if (m_webdriver) + m_webdriver->page_did_open_dialog({}); } void PageClient::confirm_closed(bool accepted) @@ -409,6 +415,9 @@ void PageClient::confirm_closed(bool accepted) void PageClient::page_did_request_prompt(String const& message, String const& default_) { client().async_did_request_prompt(m_id, message, default_); + + if (m_webdriver) + m_webdriver->page_did_open_dialog({}); } void PageClient::page_did_request_set_prompt_text(String const& text) diff --git a/Userland/Services/WebContent/WebDriverConnection.cpp b/Userland/Services/WebContent/WebDriverConnection.cpp index 14e0dd83fcd..05958661af3 100644 --- a/Userland/Services/WebContent/WebDriverConnection.cpp +++ b/Userland/Services/WebContent/WebDriverConnection.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -40,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -52,6 +54,7 @@ #include #include #include +#include #include #include #include @@ -201,6 +204,9 @@ void WebDriverConnection::visit_edges(JS::Cell::Visitor& visitor) visitor.visit(m_current_parent_browsing_context); visitor.visit(m_current_top_level_browsing_context); visitor.visit(m_action_executor); + visitor.visit(m_document_observer); + visitor.visit(m_navigation_observer); + visitor.visit(m_navigation_timer); } // https://w3c.github.io/webdriver/#dfn-close-the-session @@ -288,22 +294,28 @@ Messages::WebDriverClient::NavigateToResponse WebDriverConnection::navigate_to(J // 7. Navigate the current top-level browsing context to url. current_top_level_browsing_context()->page().load(url); + auto navigation_complete = JS::create_heap_function(current_top_level_browsing_context()->heap(), [this](Web::WebDriver::Response result) { + // 9. Set the current browsing context with the current top-level browsing context. + set_current_browsing_context(*current_top_level_browsing_context()); + + // FIXME: 10. If the current top-level browsing context contains a refresh state pragma directive of time 1 second or less, wait until the refresh timeout has elapsed, a new navigate has begun, and return to the first step of this algorithm. + + async_navigation_complete(move(result)); + }); + // 8. If url is special except for file and current URL and URL do not have the same absolute URL: // AD-HOC: We wait for the navigation to complete regardless of whether the current URL differs from the provided // URL. Even if they're the same, the navigation queues a tasks that we must await, otherwise subsequent // endpoint invocations will attempt to operate on the wrong page. if (url.is_special() && url.scheme() != "file"sv) { // a. Try to wait for navigation to complete. - TRY(wait_for_navigation_to_complete()); + wait_for_navigation_to_complete(navigation_complete); // FIXME: b. Try to run the post-navigation checks. + } else { + navigation_complete->function()(JsonValue {}); } - // 9. Set the current browsing context with the current top-level browsing context. - set_current_browsing_context(*current_top_level_browsing_context()); - - // FIXME: 10. If the current top-level browsing context contains a refresh state pragma directive of time 1 second or less, wait until the refresh timeout has elapsed, a new navigate has begun, and return to the first step of this algorithm. - // 11. Return success with data null. return JsonValue {}; } @@ -1365,14 +1377,14 @@ Messages::WebDriverClient::ElementClickResponse WebDriverConnection::element_cli // FIXME: 10. Perform implementation-defined steps to allow any navigations triggered by the click to start. // 11. Try to wait for navigation to complete. - if (auto navigation_result = wait_for_navigation_to_complete(); navigation_result.is_error()) { - async_actions_performed(navigation_result.release_error()); - return; - } + wait_for_navigation_to_complete(JS::create_heap_function(current_browsing_context().heap(), [this, result = move(result)](Web::WebDriver::Response navigation_result) mutable { + // FIXME: 12. Try to run the post-navigation checks. - // FIXME: 12. Try to run the post-navigation checks. - - async_actions_performed(move(result)); + if (navigation_result.is_error()) + async_actions_performed(move(navigation_result)); + else + async_actions_performed(move(result)); + })); }); // 8. Matching on element: @@ -2394,57 +2406,105 @@ ErrorOr WebDriverConnection::handle_any_user_prompt return {}; } -// https://w3c.github.io/webdriver/#dfn-waiting-for-the-navigation-to-complete +// https://w3c.github.io/webdriver/#dfn-wait-for-navigation-to-complete // FIXME: Update this AO to the latest spec steps. -ErrorOr WebDriverConnection::wait_for_navigation_to_complete() +void WebDriverConnection::wait_for_navigation_to_complete(OnNavigationComplete on_complete) { // 1. If the current session has a page loading strategy of none, return success with data null. - if (m_page_load_strategy == Web::WebDriver::PageLoadStrategy::None) - return {}; + if (m_page_load_strategy == Web::WebDriver::PageLoadStrategy::None) { + on_complete->function()(JsonValue {}); + return; + } // 2. If the current browsing context is no longer open, return success with data null. - if (ensure_browsing_context_is_open(current_browsing_context()).is_error()) - return {}; + if (ensure_browsing_context_is_open(current_browsing_context()).is_error()) { + on_complete->function()(JsonValue {}); + return; + } + auto& realm = current_browsing_context().active_document()->realm(); auto navigable = current_browsing_context().active_document()->navigable(); - if (!navigable || navigable->ongoing_navigation().has()) - return {}; - // 3. Start a timer. If this algorithm has not completed before timer reaches the session’s session page load timeout in milliseconds, return an error with error code timeout. - auto page_load_timeout_fired = false; - auto timer = Core::Timer::create_single_shot(m_timeouts_configuration.page_load_timeout.value_or(300'000), [&] { - page_load_timeout_fired = true; - }); - timer->start(); - // 4. If there is an ongoing attempt to navigate the current browsing context that has not yet matured, wait for navigation to mature. - Web::Platform::EventLoopPlugin::the().spin_until([&] { - return page_load_timeout_fired || navigable->ongoing_navigation().has(); + if (!navigable || navigable->ongoing_navigation().has()) { + on_complete->function()(JsonValue {}); + return; + } + + auto reset_observers = [](auto& self) { + if (self.m_navigation_observer) { + self.m_navigation_observer->set_navigation_complete({}); + self.m_navigation_observer = nullptr; + } + if (self.m_document_observer) { + self.m_document_observer->set_document_readiness_observer({}); + self.m_document_observer = nullptr; + } + }; + + // 3. Start a timer. If this algorithm has not completed before timer reaches the session’s session page load timeout + // in milliseconds, return an error with error code timeout. + m_navigation_timer = realm.heap().allocate(realm); + + // 4. If there is an ongoing attempt to navigate the current browsing context that has not yet matured, wait for + // navigation to mature. + m_navigation_observer = realm.heap().allocate(realm, realm, *navigable); + + m_navigation_observer->set_navigation_complete([this, &realm, reset_observers]() { + reset_observers(*this); + + // 5. Let readiness target be the document readiness state associated with the current session’s page loading + // strategy, which can be found in the table of page load strategies. + auto readiness_target = [this]() { + switch (m_page_load_strategy) { + case Web::WebDriver::PageLoadStrategy::Normal: + return Web::HTML::DocumentReadyState::Complete; + case Web::WebDriver::PageLoadStrategy::Eager: + return Web::HTML::DocumentReadyState::Interactive; + default: + VERIFY_NOT_REACHED(); + }; + }(); + + // 6. Wait for the current browsing context’s document readiness state to reach readiness target, + // or for the session page load timeout to pass, whichever occurs sooner. + if (auto* document = current_browsing_context().active_document(); document->readiness() != readiness_target) { + m_document_observer = realm.heap().allocate(realm, realm, *document); + + m_document_observer->set_document_readiness_observer([this, readiness_target](Web::HTML::DocumentReadyState readiness) { + if (readiness == readiness_target) + m_navigation_timer->stop_and_fire_timeout_handler(); + }); + } else { + m_navigation_timer->stop_and_fire_timeout_handler(); + } }); - // 5. Let readiness target be the document readiness state associated with the current session’s page loading strategy, which can be found in the table of page load strategies. - auto readiness_target = [this]() { - switch (m_page_load_strategy) { - case Web::WebDriver::PageLoadStrategy::Normal: - return Web::HTML::DocumentReadyState::Complete; - case Web::WebDriver::PageLoadStrategy::Eager: - return Web::HTML::DocumentReadyState::Interactive; - default: - VERIFY_NOT_REACHED(); - }; - }(); + m_navigation_timer->start(m_timeouts_configuration.page_load_timeout.value_or(300'000), JS::create_heap_function(realm.heap(), [this, on_complete, reset_observers]() { + reset_observers(*this); - // 6. Wait for the current browsing context’s document readiness state to reach readiness target, - // or for the session page load timeout to pass, whichever occurs sooner. - Web::Platform::EventLoopPlugin::the().spin_until([&]() { - return page_load_timeout_fired || current_browsing_context().active_document()->readiness() == readiness_target; - }); + auto did_time_out = m_navigation_timer->is_timed_out(); + m_navigation_timer = nullptr; - // 7. If the previous step completed by the session page load timeout being reached and the browser does not have an active user prompt, return error with error code timeout. - if (page_load_timeout_fired && !current_browsing_context().page().has_pending_dialog()) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::Timeout, "Navigation timed out"sv); + // 7. If the previous step completed by the session page load timeout being reached and the browser does + // not have an active user prompt, return error with error code timeout. + if (did_time_out && !current_browsing_context().active_document()->page().has_pending_dialog()) { + on_complete->function()(Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::Timeout, "Navigation timed out"sv)); + return; + } - // 8. Return success with data null. - return {}; + // 8. Return success with data null. + on_complete->function()(JsonValue {}); + })); +} + +void WebDriverConnection::page_did_open_dialog(Badge) +{ + // OPTMIZATION: If a dialog is opened while we are awaiting a specific document readiness state, that state will + // never be reached, as the dialog will block the HTML event loop from any further processing. Instead + // of waiting for the session's page load timeout to expire, unblock the waiter immediately. This also + // seems to match how other browsers behave. + if (m_navigation_timer) + m_navigation_timer->stop_and_fire_timeout_handler(); } // https://w3c.github.io/webdriver/#dfn-restore-the-window diff --git a/Userland/Services/WebContent/WebDriverConnection.h b/Userland/Services/WebContent/WebDriverConnection.h index beef698ff97..20777b00d58 100644 --- a/Userland/Services/WebContent/WebDriverConnection.h +++ b/Userland/Services/WebContent/WebDriverConnection.h @@ -37,6 +37,8 @@ public: void visit_edges(JS::Cell::Visitor&); + void page_did_open_dialog(Badge); + private: WebDriverConnection(IPC::Transport transport, Web::PageClient& page_client); @@ -122,7 +124,8 @@ private: Gfx::IntRect maximize_the_window(); Gfx::IntRect iconify_the_window(); - ErrorOr wait_for_navigation_to_complete(); + using OnNavigationComplete = JS::NonnullGCPtr>; + void wait_for_navigation_to_complete(OnNavigationComplete); Gfx::IntPoint calculate_absolute_position_of_element(JS::NonnullGCPtr rect); Gfx::IntRect calculate_absolute_rect_of_element(Web::DOM::Element const& element); @@ -159,6 +162,10 @@ private: JS::GCPtr m_current_top_level_browsing_context; JS::GCPtr m_action_executor; + + JS::GCPtr m_document_observer; + JS::GCPtr m_navigation_observer; + JS::GCPtr m_navigation_timer; }; } diff --git a/Userland/Services/WebContent/WebDriverServer.ipc b/Userland/Services/WebContent/WebDriverServer.ipc index 753461970b4..0d51131222d 100644 --- a/Userland/Services/WebContent/WebDriverServer.ipc +++ b/Userland/Services/WebContent/WebDriverServer.ipc @@ -1,6 +1,7 @@ #include endpoint WebDriverServer { + navigation_complete(Web::WebDriver::Response response) =| script_executed(Web::WebDriver::Response response) =| actions_performed(Web::WebDriver::Response response) =| } diff --git a/Userland/Services/WebDriver/Client.cpp b/Userland/Services/WebDriver/Client.cpp index 1fd49758ca6..665fe41306b 100644 --- a/Userland/Services/WebDriver/Client.cpp +++ b/Userland/Services/WebDriver/Client.cpp @@ -241,7 +241,7 @@ Web::WebDriver::Response Client::navigate_to(Web::WebDriver::Parameters paramete { dbgln_if(WEBDRIVER_DEBUG, "Handling POST /session//url"); auto session = TRY(find_session_with_id(parameters[0])); - return session->web_content_connection().navigate_to(payload); + return session->navigate_to(payload); } // 10.2 Get Current URL, https://w3c.github.io/webdriver/#dfn-get-current-url diff --git a/Userland/Services/WebDriver/Session.cpp b/Userland/Services/WebDriver/Session.cpp index 0a4086dda07..578fdb85f8e 100644 --- a/Userland/Services/WebDriver/Session.cpp +++ b/Userland/Services/WebDriver/Session.cpp @@ -202,6 +202,13 @@ static Web::WebDriver::Response perform_async_action(Handler& handler, Action&& return response.release_value(); } +Web::WebDriver::Response Session::navigate_to(JsonValue payload) const +{ + return perform_async_action(web_content_connection().on_navigation_complete, [&]() { + return web_content_connection().navigate_to(move(payload)); + }); +} + Web::WebDriver::Response Session::execute_script(JsonValue payload, ScriptMode mode) const { return perform_async_action(web_content_connection().on_script_executed, [&]() { diff --git a/Userland/Services/WebDriver/Session.h b/Userland/Services/WebDriver/Session.h index 035117668af..4d271d8f053 100644 --- a/Userland/Services/WebDriver/Session.h +++ b/Userland/Services/WebDriver/Session.h @@ -57,6 +57,8 @@ public: Web::WebDriver::Response get_window_handles() const; ErrorOr ensure_current_window_handle_is_valid() const; + Web::WebDriver::Response navigate_to(JsonValue) const; + enum class ScriptMode { Sync, Async, diff --git a/Userland/Services/WebDriver/WebContentConnection.cpp b/Userland/Services/WebDriver/WebContentConnection.cpp index 823252ab714..39fcae896d7 100644 --- a/Userland/Services/WebDriver/WebContentConnection.cpp +++ b/Userland/Services/WebDriver/WebContentConnection.cpp @@ -20,6 +20,12 @@ void WebContentConnection::die() on_close(); } +void WebContentConnection::navigation_complete(Web::WebDriver::Response const& response) +{ + if (on_navigation_complete) + on_navigation_complete(response); +} + void WebContentConnection::script_executed(Web::WebDriver::Response const& response) { if (on_script_executed) diff --git a/Userland/Services/WebDriver/WebContentConnection.h b/Userland/Services/WebDriver/WebContentConnection.h index cac9ce3fd1f..3017dcf82f6 100644 --- a/Userland/Services/WebDriver/WebContentConnection.h +++ b/Userland/Services/WebDriver/WebContentConnection.h @@ -22,12 +22,14 @@ public: explicit WebContentConnection(IPC::Transport transport); Function on_close; + Function on_navigation_complete; Function on_script_executed; Function on_actions_performed; private: virtual void die() override; + virtual void navigation_complete(Web::WebDriver::Response const&) override; virtual void script_executed(Web::WebDriver::Response const&) override; virtual void actions_performed(Web::WebDriver::Response const&) override; };