disown crashes the shell

[xaverdh]

Entering the following sequence into fish reliably causes the shell to crash on my machine:

sleep 1 | begin ; sleep 1&; disown $last_pid; end

The fish version is 3.0.1 on NixOS (linux kernel 4.20.13, x86_64).
I tested with alacritty and xfce4-terminal, the result is independent of terminal used.

sh -c 'env HOME=$(mktemp -d) fish' does not change anything.

last_pid was not present in fish-v2, so I think the bug was not present there yet.

[faho]

I can reproduce in git, with 3.0.2-566-g08fd8b647. It trips over

src/parser.cpp:588: void parser_t::job_promote(job_t*): Zusicherung »loc != my_job_list.end()«

(where "Zusicherung" means "assertion". Since when are these messages translated?)

That assertion seems fairly critical - it's basically that the job_t* is still valid, which it seems to not be anymore.

I think it's that the begin; sleep 1 & disown $last_pid; end block is one job that consists of two subjobs - sleep 1 & and disown $last_pid. The disown removes the sleep job, so it's no longer in the list before the parent job is completed.

[mqudsi]

Well, the job_t is still valid because it's coming out of a shared_ptr, but that job no longer exists in the jobs list as disown directly removes it from there (bad idea!).

I fixed this in 394623b by preventing disown from directly manipulating the jobs list (@ridiculousfish should be happy with one less outside manipulation!) and leaving that up to the parser.

There is a bug abberation now (where previously there was a crash) that jobs called within a subjob prints a disowned job, that surprisingly has nothing to do with the fact that disown no longer actually removes the job. The job that is disowned is the subjob but the parent job still exists (though it is functionally empty).. or something:

mqudsi@ZBOOK ~/r/fish-shell> sleep 1 | begin ; jobs; sleep 1&; disown $last_pid; jobs; end
jobs: There are no jobs
Job     Group   CPU     State   Command
3       13656   0%      running sleep 1&

I say "or something" because this doesn't take care of it (not that it would be correct to do so even if it did):

diff --git a/src/builtin_disown.cpp b/src/builtin_disown.cpp
index 25654d138..22b9ddd02 100644
--- a/src/builtin_disown.cpp
+++ b/src/builtin_disown.cpp
@@ -35,6 +35,9 @@ static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &stream
     // within the context of a subjob which will cause the parent job to crash in exec_job().
     // Instead, we set a flag and the parser removes the job from the jobs list later.
     j->set_flag(job_flag_t::PENDING_REMOVAL, true);
+    for (auto k = j; k->parent_job != nullptr; k = k->parent_job.get()) {
+        k->parent_job->set_flag(job_flag_t::PENDING_REMOVAL, true);
+    }
     add_disowned_pgid(j->pgid);
 
     return STATUS_CMD_OK;
diff --git a/src/proc.h b/src/proc.h
index 3b6186799..b311508c5 100644
--- a/src/proc.h
+++ b/src/proc.h
@@ -303,6 +303,7 @@ class job_t {
 
     // The parent job. If we were created as a nested job due to execution of a block or function in
     // a pipeline, then this refers to the job corresponding to that pipeline. Otherwise it is null.
+public:
     const std::shared_ptr<job_t> parent_job;
 
     // No copying.