From 0722a3b1c091adbafc056686ebedbf5c2db84207 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 25 Oct 2024 12:31:07 -0400 Subject: [PATCH] LibWeb+WebContent+WebDriver: Asynchronously wait for dialog dismissal There was a timing issue here where WebDriver would dismiss a dialog, and then invoke another endpoint before the dialog was actually closed. This is because the dismissal first has to hop over to the UI process to close the graphical dialog, which then asynchronously informs WebContent of the result. It's not until WebContent receives that result that the dialog is considered closed, thus those subsequent endpoints would abort due a dialog being "open". We now wait for dialogs to be fully closed before returning from the dismissal endpoints. --- Userland/Libraries/LibWeb/Page/Page.cpp | 29 ++++++++++++++----- Userland/Libraries/LibWeb/Page/Page.h | 7 +++-- .../WebContent/WebDriverConnection.cpp | 8 +++-- .../Services/WebContent/WebDriverServer.ipc | 1 + Userland/Services/WebDriver/Client.cpp | 4 +-- Userland/Services/WebDriver/Session.cpp | 14 +++++++++ Userland/Services/WebDriver/Session.h | 3 ++ .../WebDriver/WebContentConnection.cpp | 6 ++++ .../Services/WebDriver/WebContentConnection.h | 2 ++ 9 files changed, 60 insertions(+), 14 deletions(-) diff --git a/Userland/Libraries/LibWeb/Page/Page.cpp b/Userland/Libraries/LibWeb/Page/Page.cpp index 134d94837d6..439619d0641 100644 --- a/Userland/Libraries/LibWeb/Page/Page.cpp +++ b/Userland/Libraries/LibWeb/Page/Page.cpp @@ -48,6 +48,7 @@ void Page::visit_edges(JS::Cell::Visitor& visitor) Base::visit_edges(visitor); visitor.visit(m_top_level_traversable); visitor.visit(m_client); + visitor.visit(m_on_pending_dialog_closed); } HTML::Navigable& Page::focused_navigable() @@ -287,9 +288,8 @@ void Page::did_request_alert(String const& message) void Page::alert_closed() { if (m_pending_dialog == PendingDialog::Alert) { - m_pending_dialog = PendingDialog::None; m_pending_alert_response = Empty {}; - m_pending_dialog_text.clear(); + on_pending_dialog_closed(); } } @@ -307,9 +307,8 @@ bool Page::did_request_confirm(String const& message) void Page::confirm_closed(bool accepted) { if (m_pending_dialog == PendingDialog::Confirm) { - m_pending_dialog = PendingDialog::None; m_pending_confirm_response = accepted; - m_pending_dialog_text.clear(); + on_pending_dialog_closed(); } } @@ -327,14 +326,15 @@ Optional Page::did_request_prompt(String const& message, String const& d void Page::prompt_closed(Optional response) { if (m_pending_dialog == PendingDialog::Prompt) { - m_pending_dialog = PendingDialog::None; m_pending_prompt_response = move(response); - m_pending_dialog_text.clear(); + on_pending_dialog_closed(); } } -void Page::dismiss_dialog() +void Page::dismiss_dialog(JS::GCPtr> on_dialog_closed) { + m_on_pending_dialog_closed = on_dialog_closed; + switch (m_pending_dialog) { case PendingDialog::None: break; @@ -348,8 +348,10 @@ void Page::dismiss_dialog() } } -void Page::accept_dialog() +void Page::accept_dialog(JS::GCPtr> on_dialog_closed) { + m_on_pending_dialog_closed = on_dialog_closed; + switch (m_pending_dialog) { case PendingDialog::None: break; @@ -361,6 +363,17 @@ void Page::accept_dialog() } } +void Page::on_pending_dialog_closed() +{ + m_pending_dialog = PendingDialog::None; + m_pending_dialog_text.clear(); + + if (m_on_pending_dialog_closed) { + m_on_pending_dialog_closed->function()(); + m_on_pending_dialog_closed = nullptr; + } +} + void Page::did_request_color_picker(WeakPtr target, Color current_color) { if (m_pending_non_blocking_dialog == PendingNonBlockingDialog::None) { diff --git a/Userland/Libraries/LibWeb/Page/Page.h b/Userland/Libraries/LibWeb/Page/Page.h index a656b36988a..028bfee9d2d 100644 --- a/Userland/Libraries/LibWeb/Page/Page.h +++ b/Userland/Libraries/LibWeb/Page/Page.h @@ -145,8 +145,8 @@ public: bool has_pending_dialog() const { return m_pending_dialog != PendingDialog::None; } PendingDialog pending_dialog() const { return m_pending_dialog; } Optional const& pending_dialog_text() const { return m_pending_dialog_text; } - void dismiss_dialog(); - void accept_dialog(); + void dismiss_dialog(JS::GCPtr> on_dialog_closed = nullptr); + void accept_dialog(JS::GCPtr> on_dialog_closed = nullptr); void did_request_color_picker(WeakPtr target, Color current_color); void color_picker_update(Optional picked_color, HTML::ColorPickerUpdateState state); @@ -224,6 +224,8 @@ private: FindInPageResult perform_find_in_page_query(FindInPageQuery const&, Optional = {}); void update_find_in_page_selection(Vector> matches); + void on_pending_dialog_closed(); + JS::NonnullGCPtr m_client; WeakPtr m_focused_navigable; @@ -249,6 +251,7 @@ private: Optional m_pending_alert_response; Optional m_pending_confirm_response; Optional> m_pending_prompt_response; + JS::GCPtr> m_on_pending_dialog_closed; PendingNonBlockingDialog m_pending_non_blocking_dialog { PendingNonBlockingDialog::None }; WeakPtr m_pending_non_blocking_dialog_target; diff --git a/Userland/Services/WebContent/WebDriverConnection.cpp b/Userland/Services/WebContent/WebDriverConnection.cpp index 05958661af3..bbd7b34edbc 100644 --- a/Userland/Services/WebContent/WebDriverConnection.cpp +++ b/Userland/Services/WebContent/WebDriverConnection.cpp @@ -2162,7 +2162,9 @@ Messages::WebDriverClient::DismissAlertResponse WebDriverConnection::dismiss_ale return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchAlert, "No user dialog is currently open"sv); // 3. Dismiss the current user prompt. - current_browsing_context().page().dismiss_dialog(); + current_browsing_context().page().dismiss_dialog(JS::create_heap_function(current_browsing_context().heap(), [this]() { + async_dialog_closed(JsonValue {}); + })); // 4. Return success with data null. return JsonValue {}; @@ -2179,7 +2181,9 @@ Messages::WebDriverClient::AcceptAlertResponse WebDriverConnection::accept_alert return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchAlert, "No user dialog is currently open"sv); // 3. Accept the current user prompt. - current_browsing_context().page().accept_dialog(); + current_browsing_context().page().accept_dialog(JS::create_heap_function(current_browsing_context().heap(), [this]() { + async_dialog_closed(JsonValue {}); + })); // 4. Return success with data null. return JsonValue {}; diff --git a/Userland/Services/WebContent/WebDriverServer.ipc b/Userland/Services/WebContent/WebDriverServer.ipc index 0d51131222d..d058d7c4d75 100644 --- a/Userland/Services/WebContent/WebDriverServer.ipc +++ b/Userland/Services/WebContent/WebDriverServer.ipc @@ -4,4 +4,5 @@ endpoint WebDriverServer { navigation_complete(Web::WebDriver::Response response) =| script_executed(Web::WebDriver::Response response) =| actions_performed(Web::WebDriver::Response response) =| + dialog_closed(Web::WebDriver::Response response) =| } diff --git a/Userland/Services/WebDriver/Client.cpp b/Userland/Services/WebDriver/Client.cpp index 665fe41306b..8dca2449863 100644 --- a/Userland/Services/WebDriver/Client.cpp +++ b/Userland/Services/WebDriver/Client.cpp @@ -721,7 +721,7 @@ Web::WebDriver::Response Client::dismiss_alert(Web::WebDriver::Parameters parame { dbgln_if(WEBDRIVER_DEBUG, "Handling POST /session//alert/dismiss"); auto session = TRY(find_session_with_id(parameters[0])); - return session->web_content_connection().dismiss_alert(); + return session->dismiss_alert(); } // 16.2 Accept Alert, https://w3c.github.io/webdriver/#accept-alert @@ -730,7 +730,7 @@ Web::WebDriver::Response Client::accept_alert(Web::WebDriver::Parameters paramet { dbgln_if(WEBDRIVER_DEBUG, "Handling POST /session//alert/accept"); auto session = TRY(find_session_with_id(parameters[0])); - return session->web_content_connection().accept_alert(); + return session->accept_alert(); } // 16.3 Get Alert Text, https://w3c.github.io/webdriver/#get-alert-text diff --git a/Userland/Services/WebDriver/Session.cpp b/Userland/Services/WebDriver/Session.cpp index 578fdb85f8e..a3a2cdb90ec 100644 --- a/Userland/Services/WebDriver/Session.cpp +++ b/Userland/Services/WebDriver/Session.cpp @@ -243,4 +243,18 @@ Web::WebDriver::Response Session::perform_actions(JsonValue payload) const }); } +Web::WebDriver::Response Session::dismiss_alert() const +{ + return perform_async_action(web_content_connection().on_dialog_closed, [&]() { + return web_content_connection().dismiss_alert(); + }); +} + +Web::WebDriver::Response Session::accept_alert() const +{ + return perform_async_action(web_content_connection().on_dialog_closed, [&]() { + return web_content_connection().accept_alert(); + }); +} + } diff --git a/Userland/Services/WebDriver/Session.h b/Userland/Services/WebDriver/Session.h index 4d271d8f053..88c504aaf12 100644 --- a/Userland/Services/WebDriver/Session.h +++ b/Userland/Services/WebDriver/Session.h @@ -69,6 +69,9 @@ public: Web::WebDriver::Response element_send_keys(String, JsonValue) const; Web::WebDriver::Response perform_actions(JsonValue) const; + Web::WebDriver::Response dismiss_alert() const; + Web::WebDriver::Response accept_alert() const; + private: using ServerPromise = Core::Promise>; ErrorOr> create_server(NonnullRefPtr promise); diff --git a/Userland/Services/WebDriver/WebContentConnection.cpp b/Userland/Services/WebDriver/WebContentConnection.cpp index 39fcae896d7..71f0824e4b1 100644 --- a/Userland/Services/WebDriver/WebContentConnection.cpp +++ b/Userland/Services/WebDriver/WebContentConnection.cpp @@ -38,4 +38,10 @@ void WebContentConnection::actions_performed(Web::WebDriver::Response const& res on_actions_performed(response); } +void WebContentConnection::dialog_closed(Web::WebDriver::Response const& response) +{ + if (on_dialog_closed) + on_dialog_closed(response); +} + } diff --git a/Userland/Services/WebDriver/WebContentConnection.h b/Userland/Services/WebDriver/WebContentConnection.h index 3017dcf82f6..04464d1a8d4 100644 --- a/Userland/Services/WebDriver/WebContentConnection.h +++ b/Userland/Services/WebDriver/WebContentConnection.h @@ -25,6 +25,7 @@ public: Function on_navigation_complete; Function on_script_executed; Function on_actions_performed; + Function on_dialog_closed; private: virtual void die() override; @@ -32,6 +33,7 @@ private: 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; + virtual void dialog_closed(Web::WebDriver::Response const&) override; }; }