LibCore/Process: Make all spawn overloads return ErrorOr<Process>

Author: https://github.com/stasoid
Commit: 4a731b3858
Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/2377
Reviewed-by: https://github.com/ADKaster ✅

Libraries/LibCore/Process.cpp

@@ -121,7 +121,7 @@ ErrorOr<Process> Process::spawn(ProcessSpawnOptions const& options)
     return Process { pid };
 }
 
-ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<ByteString> arguments, ByteString working_directory, KeepAsChild keep_as_child)
+ErrorOr<Process> Process::spawn(StringView path, ReadonlySpan<ByteString> arguments, ByteString working_directory, KeepAsChild keep_as_child)
 {
     auto process = TRY(spawn({
         .executable = path,
@@ -131,14 +131,11 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<ByteString> argument
 
     if (keep_as_child == KeepAsChild::No)
         TRY(process.disown());
-    else {
+
-        // FIXME: This won't be needed if return value is changed to Process.
+    return process;
-        process.m_should_disown = false;
-    }
-    return process.pid();
 }
 
-ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<StringView> arguments, ByteString working_directory, KeepAsChild keep_as_child)
+ErrorOr<Process> Process::spawn(StringView path, ReadonlySpan<StringView> arguments, ByteString working_directory, KeepAsChild keep_as_child)
 {
     Vector<ByteString> backing_strings;
     backing_strings.ensure_capacity(arguments.size());
@@ -153,9 +150,8 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<StringView> argument
 
     if (keep_as_child == KeepAsChild::No)
         TRY(process.disown());
-    else
+
-        process.m_should_disown = false;
+    return process;
-    return process.pid();
 }
 
 ErrorOr<String> Process::get_name()

Libraries/LibCore/Process.h

@@ -74,9 +74,8 @@ public:
     static ErrorOr<Process> spawn(ProcessSpawnOptions const& options);
     static Process current();
 
-    // FIXME: Make the following 2 functions return Process instance or delete them.
+    static ErrorOr<Process> spawn(StringView path, ReadonlySpan<ByteString> arguments, ByteString working_directory = {}, KeepAsChild keep_as_child = KeepAsChild::No);
-    static ErrorOr<pid_t> spawn(StringView path, ReadonlySpan<ByteString> arguments, ByteString working_directory = {}, KeepAsChild keep_as_child = KeepAsChild::No);
+    static ErrorOr<Process> spawn(StringView path, ReadonlySpan<StringView> arguments, ByteString working_directory = {}, KeepAsChild keep_as_child = KeepAsChild::No);
-    static ErrorOr<pid_t> spawn(StringView path, ReadonlySpan<StringView> arguments, ByteString working_directory = {}, KeepAsChild keep_as_child = KeepAsChild::No);
 
     static ErrorOr<String> get_name();
     enum class SetThreadName {
@@ -96,7 +95,7 @@ public:
     ErrorOr<bool> wait_for_termination();
 
 private:
-    Process(pid_t pid)
+    Process(pid_t pid = -1)
         : m_pid(pid)
         , m_should_disown(true)
     {

Services/WebDriver/Client.h

@@ -19,8 +19,8 @@
 namespace WebDriver {
 
 struct LaunchBrowserCallbacks {
-    Function<ErrorOr<pid_t>(ByteString const&)> launch_browser;
+    Function<ErrorOr<Core::Process>(ByteString const&)> launch_browser;
-    Function<ErrorOr<pid_t>(ByteString const&)> launch_headless_browser;
+    Function<ErrorOr<Core::Process>(ByteString const&)> launch_headless_browser;
 };
 
 class Client final : public Web::WebDriver::Client {

Services/WebDriver/Session.cpp

@@ -43,10 +43,9 @@ Session::~Session()
     // from active sessions
 
     // 3. Perform any implementation-specific cleanup steps.
-    if (m_browser_pid.has_value()) {
+    if (m_browser_process.has_value())
-        MUST(Core::System::kill(*m_browser_pid, SIGTERM));
+        MUST(Core::System::kill(m_browser_process->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 = {};
@@ -172,9 +171,9 @@ ErrorOr<void> Session::start(LaunchBrowserCallbacks const& callbacks)
     m_web_content_server = TRY(create_server(promise));
 
     if (m_options.headless)
-        m_browser_pid = TRY(callbacks.launch_headless_browser(*m_web_content_socket_path));
+        m_browser_process = TRY(callbacks.launch_headless_browser(*m_web_content_socket_path));
     else
-        m_browser_pid = TRY(callbacks.launch_browser(*m_web_content_socket_path));
+        m_browser_process = TRY(callbacks.launch_browser(*m_web_content_socket_path));
 
     // 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.

Services/WebDriver/Session.h

@@ -15,6 +15,7 @@
 #include <AK/ScopeGuard.h>
 #include <AK/String.h>
 #include <LibCore/EventLoop.h>
+#include <LibCore/Process.h>
 #include <LibCore/Promise.h>
 #include <LibWeb/WebDriver/Capabilities.h>
 #include <LibWeb/WebDriver/Error.h>
@@ -95,7 +96,7 @@ private:
     String m_current_window_handle;
 
     Optional<ByteString> m_web_content_socket_path;
-    Optional<pid_t> m_browser_pid;
+    Optional<Core::Process> m_browser_process;
 
     RefPtr<Core::LocalServer> m_web_content_server;
 

Services/WebDriver/main.cpp

@@ -19,11 +19,11 @@
 
 static Vector<ByteString> certificates;
 
-static ErrorOr<pid_t> launch_process(StringView application, ReadonlySpan<ByteString> arguments)
+static ErrorOr<Core::Process> launch_process(StringView application, ReadonlySpan<ByteString> arguments)
 {
     auto paths = TRY(WebView::get_paths_for_helper_process(application));
 
-    ErrorOr<pid_t> result = -1;
+    ErrorOr<Core::Process> result = Error::from_string_literal("All paths failed to launch");
     for (auto const& path : paths) {
         auto path_view = path.view();
         result = Core::Process::spawn(path_view, arguments, {}, Core::Process::KeepAsChild::Yes);
@@ -56,13 +56,13 @@ static Vector<ByteString> create_arguments(ByteString const& socket_path, bool f
     return arguments;
 }
 
-static ErrorOr<pid_t> launch_browser(ByteString const& socket_path, bool force_cpu_painting)
+static ErrorOr<Core::Process> launch_browser(ByteString const& socket_path, bool force_cpu_painting)
 {
     auto arguments = create_arguments(socket_path, force_cpu_painting);
     return launch_process("Ladybird"sv, arguments.span());
 }
 
-static ErrorOr<pid_t> launch_headless_browser(ByteString const& socket_path, bool force_cpu_painting)
+static ErrorOr<Core::Process> launch_headless_browser(ByteString const& socket_path, bool force_cpu_painting)
 {
     auto arguments = create_arguments(socket_path, force_cpu_painting);
     return launch_process("headless-browser"sv, arguments.span());