Prevent disown from directly removing jobs

This prevents the `disown` builtin from directly removing jobs out of
the jobs list to prevent sanity issues, as `disown` may be called within
the context of a subjob (e.g. in a function or block) in which case the
parent job might not yet be done with the reference to the child job.

Instead, a flag is set and the parser removes the job from the list only
after the entire execution chain has completed.

Closes #5720.

src/builtin_disown.cpp

@@ -17,7 +17,7 @@
 
 /// Helper for builtin_disown.
 static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &streams, job_t *j) {
-    if (j == 0) {
+    if (j == nullptr) {
         streams.err.append_format(_(L"%ls: Unknown job '%ls'\n"), L"bg");
         builtin_print_error_trailer(parser, streams.err, cmd);
         return STATUS_INVALID_ARGS;
@@ -31,17 +31,13 @@ static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &stream
         streams.err.append_format(fmt, cmd, j->job_id, j->command_wcstr());
     }
 
-    for (auto itr = jobs().begin(); itr != jobs().end(); ++itr) {
+    // We cannot directly remove the job from the jobs() list as `disown` might be called
-        auto job = itr->get();
+    // within the context of a subjob which will cause the parent job to crash in exec_job().
-        if (job == j) {
+    // Instead, we set a flag and the parser removes the job from the jobs list later.
-            pid_t pgid = j->pgid;
+    j->set_flag(job_flag_t::PENDING_REMOVAL, true);
-            add_disowned_pgid(pgid);
+    add_disowned_pgid(j->pgid);
-            jobs().erase(itr);
-            return STATUS_CMD_OK;
-        }
-    }
 
-    return STATUS_CMD_ERROR;
+    return STATUS_CMD_OK;
 }
 
 /// Builtin for removing jobs from the job list.

src/builtin_jobs.cpp

@@ -175,7 +175,7 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     if (print_last) {
         // Ignore unconstructed jobs, i.e. ourself.
         for (const auto &j : jobs()) {
-            if (j->is_constructed() && !j->is_completed()) {
+            if (j->is_visible()) {
                 builtin_jobs_print(j.get(), mode, !streams.out_is_redirected, streams);
                 return STATUS_CMD_ERROR;
             }
@@ -217,7 +217,7 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         } else {
             for (const auto &j : jobs()) {
                 // Ignore unconstructed jobs, i.e. ourself.
-                if (j->is_constructed() && !j->is_completed()) {
+                if (j->is_visible()) {
                     builtin_jobs_print(j.get(), mode, !found && !streams.out_is_redirected, streams);
                     found = true;
                 }

src/parse_execution.cpp

@@ -1269,6 +1269,11 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
             remove_job(job.get());
         }
 
+        // This job was disowned during its own execution or the execution of its subjobs
+        if (job->get_flag(job_flag_t::PENDING_REMOVAL)) {
+            remove_job(job.get());
+        }
+
         // Only external commands require a new fishd barrier.
         if (job_contained_external_command) {
             set_proc_had_barrier(false);

src/proc.h

@@ -257,6 +257,10 @@ enum class job_flag_t {
     JOB_CONTROL,
     /// Whether the job wants to own the terminal when in the foreground.
     TERMINAL,
+    /// This job needs to be removed from the list of jobs for one reason or another (killed,
+    /// completed, disowned, etc). This flag is set rather than directly manipulating the jobs
+    /// list.
+    PENDING_REMOVAL,
 
     JOB_FLAG_COUNT
 };
@@ -396,6 +400,8 @@ class job_t {
     bool is_completed() const;
     /// The job is in a stopped state
     bool is_stopped() const;
+    /// The job is OK to be externally visible, e.g. to the user via `jobs`
+    bool is_visible() const { return !is_completed() && is_constructed() && !get_flag(job_flag_t::PENDING_REMOVAL); };
 
     /// \return the parent job, or nullptr.
     const std::shared_ptr<job_t> get_parent() const { return parent_job; }