Browser+WebDriver: Remove the connection between Browser and WebDriver

WebDriver now only has an IPC connection to WebContent. WebDriver still
launches the browser, but now when the session ends, we simply send a
SIGTERM signal to the browser.

Userland/Applications/Browser/Browser.h

@@ -8,7 +8,6 @@
 
 #include <AK/String.h>
 #include <Applications/Browser/IconBag.h>
-#include <Applications/Browser/WebDriverConnection.h>
 
 namespace Browser {
 
@@ -20,7 +19,6 @@ extern Vector<String> g_proxies;
 extern HashMap<String, size_t> g_proxy_mappings;
 extern bool g_content_filters_enabled;
 extern IconBag g_icon_bag;
-extern RefPtr<Browser::WebDriverConnection> g_web_driver_connection;
 extern String g_webdriver_content_ipc_path;
 
 }

Userland/Applications/Browser/CMakeLists.txt

@@ -5,9 +5,6 @@ serenity_component(
     DEPENDS BrowserSettings ImageDecoder RequestServer WebContent WebSocket
 )
 
-compile_ipc(WebDriverSessionServer.ipc WebDriverSessionServerEndpoint.h)
-compile_ipc(WebDriverSessionClient.ipc WebDriverSessionClientEndpoint.h)
-
 compile_gml(BrowserWindow.gml BrowserWindowGML.h browser_window_gml)
 compile_gml(EditBookmark.gml EditBookmarkGML.h edit_bookmark_gml)
 compile_gml(StorageWidget.gml StorageWidgetGML.h storage_widget_gml)
@@ -27,7 +24,6 @@ set(SOURCES
     StorageModel.cpp
     StorageWidget.cpp
     Tab.cpp
-    WebDriverConnection.cpp
     WindowActions.cpp
     main.cpp
 )
@@ -37,8 +33,6 @@ set(GENERATED_SOURCES
     EditBookmarkGML.h
     StorageWidgetGML.h
     TabGML.h
-    WebDriverSessionClientEndpoint.h
-    WebDriverSessionServerEndpoint.h
 )
 
 serenity_app(Browser ICON app-browser)

Userland/Applications/Browser/WebDriverConnection.cpp

@@ -1,30 +0,0 @@
-/*
- * Copyright (c) 2022, Florent Castelli <florent.castelli@gmail.com>
- * Copyright (c) 2022, Sam Atkins <atkinssj@serenityos.org>
- * Copyright (c) 2022, Tobias Christiansen <tobyase@serenityos.org>
- * Copyright (c) 2022, Tim Flynn <trflynn89@serenityos.org>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#include "WebDriverConnection.h"
-#include "BrowserWindow.h"
-#include <AK/Vector.h>
-#include <LibWebView/WebContentClient.h>
-
-namespace Browser {
-
-WebDriverConnection::WebDriverConnection(NonnullOwnPtr<Core::Stream::LocalSocket> socket, NonnullRefPtr<BrowserWindow> browser_window)
-    : IPC::ConnectionToServer<WebDriverSessionClientEndpoint, WebDriverSessionServerEndpoint>(*this, move(socket))
-    , m_browser_window(move(browser_window))
-{
-}
-
-void WebDriverConnection::quit()
-{
-    dbgln_if(WEBDRIVER_DEBUG, "WebDriverConnection: quit");
-    if (auto browser_window = m_browser_window.strong_ref())
-        browser_window->close();
-}
-
-}

Userland/Applications/Browser/WebDriverConnection.h

@@ -1,48 +0,0 @@
-/*
- * Copyright (c) 2022, Florent Castelli <florent.castelli@gmail.com>
- * Copyright (c) 2022, Sam Atkins <atkinssj@serenityos.org>
- * Copyright (c) 2022, Tobias Christiansen <tobyase@serenityos.org>
- * Copyright (c) 2022, Tim Flynn <trflynn89@serenityos.org>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#pragma once
-
-#include "BrowserWindow.h"
-#include <AK/Error.h>
-#include <AK/String.h>
-#include <Applications/Browser/WebDriverSessionClientEndpoint.h>
-#include <Applications/Browser/WebDriverSessionServerEndpoint.h>
-#include <LibCore/LocalServer.h>
-#include <LibGUI/Application.h>
-#include <LibIPC/ConnectionToServer.h>
-#include <unistd.h>
-
-namespace Browser {
-
-class WebDriverConnection final
-    : public IPC::ConnectionToServer<WebDriverSessionClientEndpoint, WebDriverSessionServerEndpoint> {
-    C_OBJECT_ABSTRACT(WebDriverConnection)
-public:
-    static ErrorOr<NonnullRefPtr<WebDriverConnection>> connect_to_webdriver(NonnullRefPtr<BrowserWindow> browser_window, String path)
-    {
-        dbgln_if(WEBDRIVER_DEBUG, "Trying to connect to {}", path);
-        auto result = TRY(Core::Stream::LocalSocket::connect(path));
-        dbgln_if(WEBDRIVER_DEBUG, "Connected to WebDriver");
-        return TRY(adopt_nonnull_ref_or_enomem(new (nothrow) WebDriverConnection(move(result), browser_window)));
-    }
-
-    virtual ~WebDriverConnection() = default;
-
-    virtual void die() override { }
-
-    virtual void quit() override;
-
-private:
-    WebDriverConnection(NonnullOwnPtr<Core::Stream::LocalSocket> socket, NonnullRefPtr<BrowserWindow> browser_window);
-
-    WeakPtr<BrowserWindow> m_browser_window;
-};
-
-}

Userland/Applications/Browser/WebDriverSessionClient.ipc

@@ -1,16 +0,0 @@
-#include <AK/URL.h>
-#include <AK/Vector.h>
-#include <LibGfx/Point.h>
-#include <LibGfx/Rect.h>
-#include <LibGfx/ShareableBitmap.h>
-#include <LibGfx/Size.h>
-#include <LibWeb/Cookie/Cookie.h>
-#include <LibWeb/Cookie/ParsedCookie.h>
-#include <LibWeb/WebDriver/ExecuteScript.h>
-
-// FIXME: This isn't used here, but the generated IPC fails to compile without this include.
-#include <LibWeb/WebDriver/Response.h>
-
-endpoint WebDriverSessionClient {
-    quit() =|
-}

Userland/Applications/Browser/main.cpp

@@ -11,7 +11,6 @@
 #include <Applications/Browser/BrowserWindow.h>
 #include <Applications/Browser/CookieJar.h>
 #include <Applications/Browser/Tab.h>
-#include <Applications/Browser/WebDriverConnection.h>
 #include <Applications/Browser/WindowActions.h>
 #include <LibConfig/Client.h>
 #include <LibCore/ArgsParser.h>
@@ -40,7 +39,6 @@ bool g_content_filters_enabled { true };
 Vector<String> g_proxies;
 HashMap<String, size_t> g_proxy_mappings;
 IconBag g_icon_bag;
-RefPtr<Browser::WebDriverConnection> g_web_driver_connection;
 String g_webdriver_content_ipc_path;
 
 }
@@ -69,11 +67,9 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
     TRY(Core::System::pledge("stdio recvfd sendfd unix fattr cpath rpath wpath proc exec"));
 
     Vector<String> specified_urls;
-    String webdriver_browser_ipc_path;
 
     Core::ArgsParser args_parser;
     args_parser.add_positional_argument(specified_urls, "URLs to open", "url", Core::ArgsParser::Required::No);
-    args_parser.add_option(webdriver_browser_ipc_path, "Path to WebDriver IPC file for Browser", "webdriver-browser-path", 0, "path");
     args_parser.add_option(Browser::g_webdriver_content_ipc_path, "Path to WebDriver IPC for WebContent", "webdriver-content-path", 0, "path");
 
     args_parser.parse(arguments);
@@ -89,10 +85,8 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
     TRY(Desktop::Launcher::add_allowed_url(URL::create_with_file_scheme(Core::StandardPaths::downloads_directory())));
     TRY(Desktop::Launcher::seal_allowlist());
 
-    if (!webdriver_browser_ipc_path.is_empty()) {
+    if (!Browser::g_webdriver_content_ipc_path.is_empty())
         specified_urls.empend("about:blank");
-        TRY(Core::System::unveil(webdriver_browser_ipc_path, "rw"sv));
-    }
 
     TRY(Core::System::unveil("/sys/kernel/processes", "r"));
     TRY(Core::System::unveil("/tmp/session/%sid/portal/filesystemaccess", "rw"));
@@ -149,9 +143,6 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
     Browser::CookieJar cookie_jar;
     auto window = Browser::BrowserWindow::construct(cookie_jar, first_url);
 
-    if (!webdriver_browser_ipc_path.is_empty())
-        Browser::g_web_driver_connection = TRY(Browser::WebDriverConnection::connect_to_webdriver(window, webdriver_browser_ipc_path));
-
     auto content_filters_watcher = TRY(Core::FileWatcher::create());
     content_filters_watcher->on_change = [&](Core::FileWatcherEvent const&) {
         dbgln("Reloading content filters because config file changed");

Userland/Services/WebDriver/BrowserConnection.cpp

@@ -1,26 +0,0 @@
-/*
- * Copyright (c) 2022, Florent Castelli <florent.castelli@gmail.com>
- * Copyright (c) 2022, Sam Atkins <atkinssj@serenityos.org>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#include "BrowserConnection.h"
-#include "Client.h"
-
-namespace WebDriver {
-
-BrowserConnection::BrowserConnection(NonnullOwnPtr<Core::Stream::LocalSocket> socket, NonnullRefPtr<Client> client, unsigned session_id)
-    : IPC::ConnectionFromClient<WebDriverSessionClientEndpoint, WebDriverSessionServerEndpoint>(*this, move(socket), 1)
-    , m_client(move(client))
-    , m_session_id(session_id)
-{
-}
-
-void BrowserConnection::die()
-{
-    dbgln_if(WEBDRIVER_DEBUG, "Session {} was closed remotely. Shutting down...", m_session_id);
-    m_client->close_session(m_session_id);
-}
-
-}

Userland/Services/WebDriver/BrowserConnection.h

@@ -1,34 +0,0 @@
-/*
- * Copyright (c) 2022, Florent Castelli <florent.castelli@gmail.com>
- * Copyright (c) 2022, Sam Atkins <atkinssj@serenityos.org>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#pragma once
-
-#include <AK/Format.h>
-#include <Applications/Browser/WebDriverSessionClientEndpoint.h>
-#include <Applications/Browser/WebDriverSessionServerEndpoint.h>
-#include <LibGUI/Application.h>
-#include <LibIPC/ConnectionFromClient.h>
-#include <LibIPC/Encoder.h>
-
-namespace WebDriver {
-
-class Client;
-
-class BrowserConnection
-    : public IPC::ConnectionFromClient<WebDriverSessionClientEndpoint, WebDriverSessionServerEndpoint> {
-    C_OBJECT_ABSTRACT(BrowserConnection)
-public:
-    BrowserConnection(NonnullOwnPtr<Core::Stream::LocalSocket> socket, NonnullRefPtr<Client> client, unsigned session_id);
-
-    virtual void die() override;
-
-private:
-    NonnullRefPtr<Client> m_client;
-    unsigned m_session_id { 0 };
-};
-
-}

Userland/Services/WebDriver/CMakeLists.txt

@@ -5,7 +5,6 @@ serenity_component(
 )
 
 set(SOURCES
-    BrowserConnection.cpp
     Client.cpp
     Session.cpp
     TimeoutsConfiguration.cpp
@@ -14,8 +13,6 @@ set(SOURCES
 )
 
 set(GENERATED_SOURCES
-    ../../Applications/Browser/WebDriverSessionClientEndpoint.h
-    ../../Applications/Browser/WebDriverSessionServerEndpoint.h
     ../../Services/WebContent/WebDriverClientEndpoint.h
     ../../Services/WebContent/WebDriverServerEndpoint.h
 )

Userland/Services/WebDriver/Session.cpp

@@ -9,7 +9,6 @@
  */
 
 #include "Session.h"
-#include "BrowserConnection.h"
 #include "Client.h"
 #include <AK/JsonObject.h>
 #include <AK/JsonParser.h>
@@ -55,42 +54,24 @@ ErrorOr<void, Web::WebDriver::Error> Session::check_for_open_top_level_browsing_
     return {};
 }
 
-ErrorOr<NonnullRefPtr<Core::LocalServer>> Session::create_server(String const& socket_path, ServerType type, NonnullRefPtr<ServerPromise> promise)
+ErrorOr<NonnullRefPtr<Core::LocalServer>> Session::create_server(String const& socket_path, NonnullRefPtr<ServerPromise> promise)
 {
     dbgln("Listening for WebDriver connection on {}", socket_path);
 
     auto server = TRY(Core::LocalServer::try_create());
     server->listen(socket_path);
 
-    server->on_accept = [this, type, promise](auto client_socket) mutable {
-        switch (type) {
-        case ServerType::Browser: {
-            auto maybe_connection = adopt_nonnull_ref_or_enomem(new (nothrow) BrowserConnection(move(client_socket), m_client, session_id()));
-            if (maybe_connection.is_error()) {
-                promise->resolve(maybe_connection.release_error());
-                return;
-            }
-
-            dbgln("WebDriver is connected to Browser socket");
-            m_browser_connection = maybe_connection.release_value();
-            break;
+    server->on_accept = [this, promise](auto client_socket) mutable {
+        auto maybe_connection = adopt_nonnull_ref_or_enomem(new (nothrow) WebContentConnection(move(client_socket), m_client, session_id()));
+        if (maybe_connection.is_error()) {
+            promise->resolve(maybe_connection.release_error());
+            return;
         }
 
-        case ServerType::WebContent: {
-            auto maybe_connection = adopt_nonnull_ref_or_enomem(new (nothrow) WebContentConnection(move(client_socket), m_client, session_id()));
-            if (maybe_connection.is_error()) {
-                promise->resolve(maybe_connection.release_error());
-                return;
-            }
+        dbgln("WebDriver is connected to WebContent socket");
+        m_web_content_connection = maybe_connection.release_value();
 
-            dbgln("WebDriver is connected to WebContent socket");
-            m_web_content_connection = maybe_connection.release_value();
-            break;
-        }
-        }
-
-        if (m_browser_connection && m_web_content_connection)
-            promise->resolve({});
+        promise->resolve({});
     };
 
     server->on_accept_error = [promise](auto error) mutable {
@@ -104,22 +85,17 @@ ErrorOr<void> Session::start()
 {
     auto promise = TRY(ServerPromise::try_create());
 
-    auto browser_socket_path = String::formatted("/tmp/webdriver/browser_{}_{}", getpid(), m_id);
-    auto browser_server = TRY(create_server(browser_socket_path, ServerType::Browser, promise));
-
-    auto web_content_socket_path = String::formatted("/tmp/webdriver/content_{}_{}", getpid(), m_id);
-    auto web_content_server = TRY(create_server(web_content_socket_path, ServerType::WebContent, promise));
+    auto web_content_socket_path = String::formatted("/tmp/webdriver/session_{}_{}", getpid(), m_id);
+    auto web_content_server = TRY(create_server(web_content_socket_path, promise));
 
     char const* argv[] = {
         "/bin/Browser",
-        "--webdriver-browser-path",
-        browser_socket_path.characters(),
         "--webdriver-content-path",
         web_content_socket_path.characters(),
         nullptr,
     };
 
-    TRY(Core::System::posix_spawn("/bin/Browser"sv, nullptr, nullptr, const_cast<char**>(argv), environ));
+    m_browser_pid = TRY(Core::System::posix_spawn("/bin/Browser"sv, nullptr, nullptr, const_cast<char**>(argv), environ));
 
     // FIXME: Allow this to be more asynchronous. For now, this at least allows us to propagate
     //        errors received while accepting the Browser and WebContent sockets.
@@ -144,7 +120,11 @@ Web::WebDriver::Response Session::stop()
     // NOTE: Handled by WebDriver::Client.
 
     // 3. Perform any implementation-specific cleanup steps.
-    m_browser_connection->async_quit();
+    if (m_browser_pid.has_value()) {
+        MUST(Core::System::kill(*m_browser_pid, SIGTERM));
+        m_browser_pid = {};
+    }
+
     m_started = false;
 
     // 4. If an error has occurred in any of the steps above, return the error, otherwise return success with data null.

Userland/Services/WebDriver/Session.h

@@ -14,7 +14,6 @@
 #include <LibCore/Promise.h>
 #include <LibWeb/WebDriver/Error.h>
 #include <LibWeb/WebDriver/Response.h>
-#include <WebDriver/BrowserConnection.h>
 #include <WebDriver/TimeoutsConfiguration.h>
 #include <WebDriver/WebContentConnection.h>
 #include <unistd.h>
@@ -57,20 +56,16 @@ public:
     Web::WebDriver::Response take_element_screenshot(StringView element_id);
 
 private:
-    enum class ServerType {
-        Browser,
-        WebContent,
-    };
     using ServerPromise = Core::Promise<ErrorOr<void>>;
-    ErrorOr<NonnullRefPtr<Core::LocalServer>> create_server(String const& socket_path, ServerType type, NonnullRefPtr<ServerPromise> promise);
+    ErrorOr<NonnullRefPtr<Core::LocalServer>> create_server(String const& socket_path, NonnullRefPtr<ServerPromise> promise);
 
     NonnullRefPtr<Client> m_client;
     bool m_started { false };
     unsigned m_id { 0 };
     HashMap<String, NonnullOwnPtr<Window>> m_windows;
     String m_current_window_handle;
-    RefPtr<BrowserConnection> m_browser_connection;
     RefPtr<WebContentConnection> m_web_content_connection;
+    Optional<pid_t> m_browser_pid;
 
     // https://w3c.github.io/webdriver/#dfn-session-script-timeout
     TimeoutsConfiguration m_timeouts_configuration;