Fix crash on redraw in BufCloseFifo hook

FifoWatcher::read_fifo() deletes the fifo watcher in

	m_buffer.values().erase(fifo_watcher_id); // will delete this

which calls:           HashMap::unordered_remove()
                           constexpr_swap(m_items[index], m_items.back());
destructor called here --> m_items.pop_back();
                           m_index.remove(hash, index);

So hash map invariants (of buffer.values()) are broken, when calling
~FifoWatcher which fires BufCloseFifo hooks. Things blow up if those
hooks access buffer.values() such as by accessing cached highlighters
to redraw the buffer.  A shell call with a long sleep in the client
context seems to trigger this.

Fix this by destroying removed map items only at the end of
HashMap::remove(), when invariants are restored.  Alternatively, we
could introduce a fifo_trash container; I haven't explored that.

5 files changed, 35 insertions, 5 deletions

diff --git a/src/hash_map.hh b/src/hash_map.hh
index eef082dd..dc6263e5 100644
--- a/src/hash_map.hh
+++ b/src/hash_map.hh
@@ -3,6 +3,7 @@
 
 #include "hash.hh"
 #include "memory.hh"
+#include "optional.hh"
 #include "vector.hh"
 
 namespace Kakoune
@@ -268,47 +269,56 @@ struct HashMap
     }
 
     template<typename KeyType> requires IsHashCompatible<Key, KeyType>
-    constexpr void remove(const KeyType& key)
+    constexpr Optional<Item> remove(const KeyType& key)
     {
+        Optional<Item> removed;
         const auto hash = hash_value(key);
         int index = find_index(key, hash);
         if (index >= 0)
         {
+            removed.emplace(std::move(m_items[index]));
             m_items.erase(m_items.begin() + index);
             m_index.remove(hash, index);
             m_index.ordered_fix_entries(index);
         }
+        return removed;
     }
 
     template<typename KeyType> requires IsHashCompatible<Key, KeyType>
-    constexpr void unordered_remove(const KeyType& key)
+    constexpr Optional<Item> unordered_remove(const KeyType& key)
     {
+        Optional<Item> removed;
         const auto hash = hash_value(key);
         int index = find_index(key, hash);
         if (index >= 0)
         {
-            constexpr_swap(m_items[index], m_items.back());
+            removed.emplace(std::move(m_items[index]));
+            m_items[index] = std::move(m_items.back());
             m_items.pop_back();
             m_index.remove(hash, index);
             if (index != m_items.size())
                 m_index.unordered_fix_entries(hash_value(item_key(m_items[index])), m_items.size(), index);
         }
+        return removed;
     }
 
     template<typename KeyType> requires IsHashCompatible<Key, KeyType>
-    constexpr void erase(const KeyType& key) { unordered_remove(key); }
+    constexpr Optional<Item> erase(const KeyType& key) { return unordered_remove(key); }
 
     template<typename KeyType> requires IsHashCompatible<Key, KeyType>
-    constexpr void remove_all(const KeyType& key)
+    constexpr Vector<Item> remove_all(const KeyType& key)
     {
+        Vector<Item> removed;
         const auto hash = hash_value(key);
         for (int index = find_index(key, hash); index >= 0;
              index = find_index(key, hash))
         {
+            removed.push_back(std::move(m_items[index]));
             m_items.erase(m_items.begin() + index);
             m_index.remove(hash, index);
             m_index.ordered_fix_entries(index);
         }
+        return removed;
     }
 
     using iterator = typename ContainerType::iterator;
diff --git a/test/regression/0-slow-BufCloseFifo/cmd b/test/regression/0-slow-BufCloseFifo/cmd
new file mode 100644
index 00000000..2eeea72e
--- /dev/null
+++ b/test/regression/0-slow-BufCloseFifo/cmd
@@ -0,0 +1 @@
+:run<ret>
diff --git a/test/regression/0-slow-BufCloseFifo/in b/test/regression/0-slow-BufCloseFifo/in
new file mode 100644
index 00000000..8b137891
--- /dev/null
+++ b/test/regression/0-slow-BufCloseFifo/in
@@ -0,0 +1 @@
+
diff --git a/test/regression/0-slow-BufCloseFifo/rc b/test/regression/0-slow-BufCloseFifo/rc
new file mode 100644
index 00000000..8a953e18
--- /dev/null
+++ b/test/regression/0-slow-BufCloseFifo/rc
@@ -0,0 +1,16 @@
+define-command run %{
+    evaluate-commands %sh{
+        mkfifo fifo1 fifo2 2>/dev/null
+        ( : >fifo1 & ) > /dev/null 2>&1 </dev/null
+    }
+    edit! -fifo fifo1 *fifo*
+    add-highlighter global/myhl regex foo 0:green
+    hook -once global BufCloseFifo .* %{
+        evaluate-commands -client client0 %{
+            nop %sh{sleep 2}
+        }
+        hook -once buffer NormalIdle .* %{
+            echo -to-file fifo2 still alive
+        }
+    }
+}
diff --git a/test/regression/0-slow-BufCloseFifo/script b/test/regression/0-slow-BufCloseFifo/script
new file mode 100644
index 00000000..2147a8e8
--- /dev/null
+++ b/test/regression/0-slow-BufCloseFifo/script
@@ -0,0 +1,2 @@
+mkfifo fifo2 2>/dev/null
+assert_eq "$(cat fifo2)" "still alive"