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

@@ -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<String> Page::did_request_prompt(String const& message, String const& d
 void Page::prompt_closed(Optional<String> 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<JS::HeapFunction<void()>> 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<JS::HeapFunction<void()>> 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<HTML::HTMLInputElement> target, Color current_color)
 {
     if (m_pending_non_blocking_dialog == PendingNonBlockingDialog::None) {

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<String> const& pending_dialog_text() const { return m_pending_dialog_text; }
-    void dismiss_dialog();
-    void accept_dialog();
+    void dismiss_dialog(JS::GCPtr<JS::HeapFunction<void()>> on_dialog_closed = nullptr);
+    void accept_dialog(JS::GCPtr<JS::HeapFunction<void()>> on_dialog_closed = nullptr);
 
     void did_request_color_picker(WeakPtr<HTML::HTMLInputElement> target, Color current_color);
     void color_picker_update(Optional<Color> picked_color, HTML::ColorPickerUpdateState state);
@@ -224,6 +224,8 @@ private:
     FindInPageResult perform_find_in_page_query(FindInPageQuery const&, Optional<SearchDirection> = {});
     void update_find_in_page_selection(Vector<JS::Handle<DOM::Range>> matches);
 
+    void on_pending_dialog_closed();
+
     JS::NonnullGCPtr<PageClient> m_client;
 
     WeakPtr<HTML::Navigable> m_focused_navigable;
@@ -249,6 +251,7 @@ private:
     Optional<Empty> m_pending_alert_response;
     Optional<bool> m_pending_confirm_response;
     Optional<Optional<String>> m_pending_prompt_response;
+    JS::GCPtr<JS::HeapFunction<void()>> m_on_pending_dialog_closed;
 
     PendingNonBlockingDialog m_pending_non_blocking_dialog { PendingNonBlockingDialog::None };
     WeakPtr<HTML::HTMLElement> m_pending_non_blocking_dialog_target;

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 {};

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) =|
 }

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/<session_id>/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/<session_id>/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

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();
+    });
+}
+
 }

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<void>>;
     ErrorOr<NonnullRefPtr<Core::LocalServer>> create_server(NonnullRefPtr<ServerPromise> promise);

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);
+}
+
 }

Userland/Services/WebDriver/WebContentConnection.h

@@ -25,6 +25,7 @@ public:
     Function<void(Web::WebDriver::Response)> on_navigation_complete;
     Function<void(Web::WebDriver::Response)> on_script_executed;
     Function<void(Web::WebDriver::Response)> on_actions_performed;
+    Function<void(Web::WebDriver::Response)> 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;
 };
 
 }