LibCore+Userland+Ladybird: Explicitly manage Core::Process's end-state

This removes the implicit `disown`ing of the Process in the destructor
in favor of up front requests in the constructor. We also force users
to exclusively and explicitly disown, wait or take the pid of the child.
I hope that it will a good way to prevent API misuses, like calling C
functions with the pid or `wait_for_termination` on a disowned child.

It unfortunately prevents the usage of the "old" API with
KeepAsChild::Yes, this is why there are some refactorings that come
with the patch.

This also should make it possible to implement KeepAsChild::No on other
platforms.

Author: LucasChollet

Ladybird/HelperProcess.cpp

@@ -33,7 +33,7 @@ static ErrorOr<NonnullRefPtr<ClientType>> launch_server_process_impl(
     }
 
     for (auto [i, path] : enumerate(candidate_server_paths)) {
-        Core::ProcessSpawnOptions options { .name = server_name, .arguments = arguments };
+        Core::ProcessSpawnOptions options { .name = server_name, .arguments = arguments, .keep_as_child = Core::KeepAsChild::Yes };
 
         if (enable_callgrind_profiling == Ladybird::EnableCallgrindProfiling::Yes) {
             options.executable = "valgrind"sv;
@@ -47,12 +47,13 @@ static ErrorOr<NonnullRefPtr<ClientType>> launch_server_process_impl(
 
         if (!result.is_error()) {
             auto process = result.release_value();
+            auto pid = process.process.take_pid();
 
             if constexpr (requires { process.client->set_pid(pid_t {}); })
-                process.client->set_pid(process.process.pid());
+                process.client->set_pid(pid);
 
             if (register_with_process_manager == RegisterWithProcessManager::Yes)
-                WebView::ProcessManager::the().add_process(WebView::process_type_from_name(server_name), process.process.pid());
+                WebView::ProcessManager::the().add_process(WebView::process_type_from_name(server_name), pid);
 
             if (enable_callgrind_profiling == Ladybird::EnableCallgrindProfiling::Yes) {
                 dbgln();

Ladybird/WebDriver/main.cpp

@@ -18,23 +18,22 @@
 
 static Vector<ByteString> certificates;
 
-static ErrorOr<pid_t> launch_process(StringView application, ReadonlySpan<char const*> arguments)
+static ErrorOr<pid_t> launch_process(StringView application, Vector<ByteString> arguments)
 {
     auto paths = TRY(get_paths_for_helper_process(application));
 
-    ErrorOr<pid_t> result = -1;
     for (auto const& path : paths) {
         auto path_view = path.view();
-        result = Core::Process::spawn(path_view, arguments, {}, Core::Process::KeepAsChild::Yes);
-        if (!result.is_error())
-            break;
+        auto maybe_process = Core::Process::spawn(Core::ProcessSpawnOptions { .executable = path_view, .arguments = arguments, .keep_as_child = Core::KeepAsChild::Yes });
+        if (!maybe_process.is_error())
+            return maybe_process.value().take_pid();
     }
-    return result;
+    return -1;
 }
 
 static ErrorOr<pid_t> launch_browser(ByteString const& socket_path)
 {
-    auto arguments = Vector {
+    auto arguments = Vector<ByteString> {
         "--webdriver-content-path",
         socket_path.characters(),
     };
@@ -50,14 +49,14 @@ static ErrorOr<pid_t> launch_browser(ByteString const& socket_path)
 
     arguments.append("about:blank");
 
-    return launch_process("Ladybird"sv, arguments.span());
+    return launch_process("Ladybird"sv, arguments);
 }
 
 static ErrorOr<pid_t> launch_headless_browser(ByteString const& socket_path)
 {
     auto resources = ByteString::formatted("{}/res", s_serenity_resource_root);
     return launch_process("headless-browser"sv,
-        Array {
+        Vector<ByteString> {
             "--resources",
             resources.characters(),
             "--webdriver-ipc-path",

Tests/LibCore/TestLibCoreProcess.cpp

@@ -0,0 +1,74 @@
+/*
+ * Copyright (c) 2025, Lucas Chollet <[email protected]>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include <LibCore/Process.h>
+#include <LibTest/TestCase.h>
+
+TEST_CASE(crash_on_api_misuse)
+{
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::No }));
+
+        EXPECT_CRASH("calling wait_for_termination() on disowned child", [&] {
+            EXPECT(!process.wait_for_termination().is_error());
+            return Test::Crash::Failure::DidNotCrash;
+        });
+
+        EXPECT_CRASH("calling take_pid() on disowned child", [&] {
+            process.take_pid();
+            return Test::Crash::Failure::DidNotCrash;
+        });
+    }
+
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::Yes }));
+
+        EXPECT_CRASH("calling take_pid() after wait_for_termination()", [&] {
+            EXPECT(!process.wait_for_termination().is_error());
+            process.take_pid();
+            return Test::Crash::Failure::DidNotCrash;
+        });
+
+        EXPECT_CRASH("calling wait_for_termination() after take_pid()", [&] {
+            EXPECT(!process.wait_for_termination().is_error());
+            process.take_pid();
+            return Test::Crash::Failure::DidNotCrash;
+        });
+        // This creates a zombie process.
+        process.take_pid();
+    }
+
+    EXPECT_CRASH("Require explicit call to wait_for_termination() of take_pid()", [&] {
+        {
+            auto maybe_process = Core::Process::spawn(
+                { .executable = "/bin/true",
+                    .keep_as_child = Core::KeepAsChild::Yes });
+            EXPECT(!maybe_process.is_error());
+        }
+        return Test::Crash::Failure::DidNotCrash;
+    });
+}
+
+TEST_CASE(no_crash)
+{
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::No }));
+    }
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::Yes }));
+        TRY_OR_FAIL(process.wait_for_termination());
+    }
+    {
+        auto process = TRY_OR_FAIL(Core::Process::spawn({ .executable = "/bin/true",
+            .keep_as_child = Core::KeepAsChild::Yes }));
+        // This creates a zombie process.
+        process.take_pid();
+    }
+}

Tests/LibELF/TestOrder.cpp

@@ -26,6 +26,7 @@ static ByteBuffer run(ByteString executable)
                 .fd = 1,
             },
         },
+        .keep_as_child = Core::KeepAsChild::Yes,
     }));
     MUST(process.wait_for_termination());
     auto output = MUST(Core::File::open(path_to_captured_output.string(), Core::File::OpenMode::Read));

Userland/Applications/Run/RunWindow.cpp

@@ -108,11 +108,15 @@ void RunWindow::do_run()
 bool RunWindow::run_as_command(ByteString const& run_input)
 {
     // TODO: Query and use the user's preferred shell.
-    auto maybe_child_pid = Core::Process::spawn("/bin/Shell"sv, Array { "-c", run_input.characters() }, {}, Core::Process::KeepAsChild::Yes);
-    if (maybe_child_pid.is_error())
+    auto maybe_child_process = Core::Process::spawn(Core::ProcessSpawnOptions {
+        .executable = "/bin/Shell"sv,
+        .arguments = { "-c", run_input.characters() },
+        .keep_as_child = Core::KeepAsChild::Yes });
+
+    if (maybe_child_process.is_error())
         return false;
 
-    pid_t child_pid = maybe_child_pid.release_value();
+    pid_t child_pid = maybe_child_process.value().take_pid();
 
     // The child shell was able to start. Let's save it to the history immediately so users can see it as the first entry the next time they run this program.
     prepend_history(run_input);

Userland/Applications/Terminal/main.cpp

@@ -103,7 +103,7 @@ class TerminalChangeListener : public Config::Listener {
 static ErrorOr<void> utmp_update(StringView tty, pid_t pid, bool create)
 {
     auto pid_string = String::number(pid);
-    Array utmp_update_command {
+    Vector<ByteString> utmp_update_command {
         "-f"sv,
         "Terminal"sv,
         "-p"sv,
@@ -112,7 +112,11 @@ static ErrorOr<void> utmp_update(StringView tty, pid_t pid, bool create)
         tty,
     };
 
-    auto utmpupdate_pid = TRY(Core::Process::spawn("/bin/utmpupdate"sv, utmp_update_command, {}, Core::Process::KeepAsChild::Yes));
+    auto utmpupdate_pid = TRY(Core::Process::spawn(Core::ProcessSpawnOptions {
+                                  .executable = "/bin/utmpupdate"sv,
+                                  .arguments = utmp_update_command,
+                                  .keep_as_child = Core::KeepAsChild::Yes }))
+                              .take_pid();
 
     Core::System::WaitPidResult status;
     auto wait_successful = false;

Userland/Libraries/LibCore/Process.cpp

@@ -114,7 +114,12 @@ ErrorOr<Process> Process::spawn(ProcessSpawnOptions const& options)
     } else {
         pid = TRY(System::posix_spawn(options.executable.view(), &spawn_actions, nullptr, const_cast<char**>(argv_list.get().data()), Core::Environment::raw_environ()));
     }
-    return Process { pid };
+
+    auto process = Process { pid };
+    if (options.keep_as_child == KeepAsChild::No)
+        TRY(process.disown());
+
+    return process;
 }
 
 ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<ByteString> arguments, ByteString working_directory, KeepAsChild keep_as_child)
@@ -123,15 +128,10 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<ByteString> argument
         .executable = path,
         .arguments = Vector<ByteString> { arguments },
         .working_directory = working_directory.is_empty() ? Optional<ByteString> {} : Optional<ByteString> { working_directory },
+        .keep_as_child = keep_as_child,
     }));
 
-    if (keep_as_child == KeepAsChild::No)
-        TRY(process.disown());
-    else {
-        // FIXME: This won't be needed if return value is changed to Process.
-        process.m_should_disown = false;
-    }
-    return process.pid();
+    return process.m_pid;
 }
 
 ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<StringView> arguments, ByteString working_directory, KeepAsChild keep_as_child)
@@ -145,13 +145,10 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<StringView> argument
         .executable = path,
         .arguments = backing_strings,
         .working_directory = working_directory.is_empty() ? Optional<ByteString> {} : Optional<ByteString> { working_directory },
+        .keep_as_child = keep_as_child,
     }));
 
-    if (keep_as_child == KeepAsChild::No)
-        TRY(process.disown());
-    else
-        process.m_should_disown = false;
-    return process.pid();
+    return process.m_pid;
 }
 
 ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<char const*> arguments, ByteString working_directory, KeepAsChild keep_as_child)
@@ -165,13 +162,10 @@ ErrorOr<pid_t> Process::spawn(StringView path, ReadonlySpan<char const*> argumen
         .executable = path,
         .arguments = backing_strings,
         .working_directory = working_directory.is_empty() ? Optional<ByteString> {} : Optional<ByteString> { working_directory },
+        .keep_as_child = keep_as_child,
     }));
 
-    if (keep_as_child == KeepAsChild::No)
-        TRY(process.disown());
-    else
-        process.m_should_disown = false;
-    return process.pid();
+    return process.m_pid;
 }
 
 ErrorOr<String> Process::get_name()
@@ -320,22 +314,20 @@ void Process::wait_for_debugger_and_break()
 
 ErrorOr<void> Process::disown()
 {
-    if (m_pid != 0 && m_should_disown) {
 #ifdef AK_OS_SERENITY
-        TRY(System::disown(m_pid));
+    TRY(System::disown(m_pid));
 #else
-        // FIXME: Support disown outside Serenity.
+    // FIXME: Support disown outside Serenity.
 #endif
-        m_should_disown = false;
-        return {};
-    } else {
-        return Error::from_errno(EINVAL);
-    }
+    m_was_managed = true;
+    return {};
 }
 
 ErrorOr<bool> Process::wait_for_termination()
 {
     VERIFY(m_pid > 0);
+    VERIFY(!m_was_managed);
+    m_was_managed = true;
 
     bool exited_with_code_0 = true;
     int status;
@@ -353,7 +345,6 @@ ErrorOr<bool> Process::wait_for_termination()
         VERIFY_NOT_REACHED();
     }
 
-    m_should_disown = false;
     return exited_with_code_0;
 }
 
@@ -457,7 +448,7 @@ ErrorOr<IPCProcess::ProcessPaths> IPCProcess::paths_for_process(StringView proce
 ErrorOr<IPCProcess::ProcessAndIPCSocket> IPCProcess::spawn_singleton_and_connect_to_process(ProcessSpawnOptions const& options)
 {
     auto [socket_path, pid_path] = TRY(paths_for_process(options.name));
-    Process process { -1 };
+    Optional<Process> process;
 
     if (auto existing_pid = TRY(get_process_pid(options.name, pid_path)); existing_pid.has_value()) {
         process = Process { *existing_pid };
@@ -485,7 +476,7 @@ ErrorOr<IPCProcess::ProcessAndIPCSocket> IPCProcess::spawn_singleton_and_connect
             auto process = TRY(Process::spawn(options));
             {
                 auto pid_file = TRY(File::open(pid_path, File::OpenMode::Write));
-                TRY(pid_file->write_until_depleted(ByteString::number(process.pid())));
+                TRY(pid_file->write_until_depleted(ByteString::number(process.m_pid)));
             }
 
             TRY(System::kill(getpid(), SIGTERM));
@@ -504,7 +495,7 @@ ErrorOr<IPCProcess::ProcessAndIPCSocket> IPCProcess::spawn_singleton_and_connect
     auto ipc_socket = TRY(LocalSocket::connect(socket_path));
     TRY(ipc_socket->set_blocking(true));
 
-    return ProcessAndIPCSocket { move(process), move(ipc_socket) };
+    return ProcessAndIPCSocket { process.release_value(), move(ipc_socket) };
 }
 
 }