From 63c16ff41ae53be5e101a757d3b2805edd1c8660 Mon Sep 17 00:00:00 2001
From: Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
Date: Sun, 19 Mar 2023 13:36:32 +0300
Subject: [PATCH] WebDriver: Inline `Sesssion::stop()` in session destructor

Previously it was possible to have following sequence of calls
while destroying a session:
1. `WebContentConnection::die()` calls `Client::close_session()`
2. `Client::close_session()` removes a session from active sessions
map which causes session destructor call.
3. Session destructor calls `Client::close_session()` to remove a
session from active sessions.

With `stop()` method inlined into destructor `close_session()` need
to be called just once while destroying a session.
---
 Userland/Services/WebDriver/Client.cpp  |  2 +-
 Userland/Services/WebDriver/Session.cpp | 54 ++++++++++---------------
 Userland/Services/WebDriver/Session.h   |  1 -
 3 files changed, 23 insertions(+), 34 deletions(-)

diff --git a/Userland/Services/WebDriver/Client.cpp b/Userland/Services/WebDriver/Client.cpp
index b97b732f67..28076d4d58 100644
--- a/Userland/Services/WebDriver/Client.cpp
+++ b/Userland/Services/WebDriver/Client.cpp
@@ -185,7 +185,7 @@ Web::WebDriver::Response Client::delete_session(Web::WebDriver::Parameters param
 
     // 1. If the current session is an active session, try to close the session.
     if (auto session = find_session_with_id(parameters[0]); !session.is_error())
-        TRY(session.value()->stop());
+        close_session(session.value()->session_id());
 
     // 2. Return success with data null.
     return JsonValue {};
diff --git a/Userland/Services/WebDriver/Session.cpp b/Userland/Services/WebDriver/Session.cpp
index 674ef6f8e3..9a04bf9b4f 100644
--- a/Userland/Services/WebDriver/Session.cpp
+++ b/Userland/Services/WebDriver/Session.cpp
@@ -26,10 +26,29 @@ Session::Session(unsigned session_id, NonnullRefPtr<Client> client, Web::WebDriv
 {
 }
 
+// https://w3c.github.io/webdriver/#dfn-close-the-session
 Session::~Session()
 {
-    if (auto error = stop(); error.is_error())
-        warnln("Failed to stop session {}: {}", m_id, error.error());
+    if (!m_started)
+        return;
+
+    // 1. Perform the following substeps based on the remote end’s type:
+    // NOTE: We perform the "Remote end is an endpoint node" steps in the WebContent process.
+    web_content_connection().close_session();
+
+    // 2. Remove the current session from active sessions.
+    // NOTE: We are in a session destruction which means it is already removed
+    // from active sessions
+
+    // 3. Perform any implementation-specific cleanup steps.
+    if (m_browser_pid.has_value()) {
+        MUST(Core::System::kill(*m_browser_pid, SIGTERM));
+        m_browser_pid = {};
+    }
+    if (m_web_content_socket_path.has_value()) {
+        MUST(Core::System::unlink(*m_web_content_socket_path));
+        m_web_content_socket_path = {};
+    }
 }
 
 ErrorOr<NonnullRefPtr<Core::LocalServer>> Session::create_server(NonnullRefPtr<ServerPromise> promise)
@@ -86,35 +105,6 @@ ErrorOr<void> Session::start(LaunchBrowserCallbacks const& callbacks)
     return {};
 }
 
-// https://w3c.github.io/webdriver/#dfn-close-the-session
-Web::WebDriver::Response Session::stop()
-{
-    if (!m_started)
-        return JsonValue {};
-
-    // 1. Perform the following substeps based on the remote end’s type:
-    // NOTE: We perform the "Remote end is an endpoint node" steps in the WebContent process.
-    web_content_connection().close_session();
-
-    // 2. Remove the current session from active sessions.
-    m_client->close_session(session_id());
-
-    // 3. Perform any implementation-specific cleanup steps.
-    if (m_browser_pid.has_value()) {
-        MUST(Core::System::kill(*m_browser_pid, SIGTERM));
-        m_browser_pid = {};
-    }
-    if (m_web_content_socket_path.has_value()) {
-        MUST(Core::System::unlink(*m_web_content_socket_path));
-        m_web_content_socket_path = {};
-    }
-
-    m_started = false;
-
-    // 4. If an error has occurred in any of the steps above, return the error, otherwise return success with data null.
-    return JsonValue {};
-}
-
 // 11.2 Close Window, https://w3c.github.io/webdriver/#dfn-close-window
 Web::WebDriver::Response Session::close_window()
 {
@@ -127,7 +117,7 @@ Web::WebDriver::Response Session::close_window()
 
         // 4. If there are no more open top-level browsing contexts, then close the session.
         if (m_windows.size() == 1)
-            TRY(stop());
+            m_client->close_session(session_id());
     }
 
     // 5. Return the result of running the remote end steps for the Get Window Handles command.
diff --git a/Userland/Services/WebDriver/Session.h b/Userland/Services/WebDriver/Session.h
index cea6609217..9b60177ba7 100644
--- a/Userland/Services/WebDriver/Session.h
+++ b/Userland/Services/WebDriver/Session.h
@@ -49,7 +49,6 @@ public:
     }
 
     ErrorOr<void> start(LaunchBrowserCallbacks const&);
-    Web::WebDriver::Response stop();
     Web::WebDriver::Response close_window();
     Web::WebDriver::Response switch_to_window(StringView);
     Web::WebDriver::Response get_window_handles() const;