npc_destroy: Defer NPC destruction via timer

055-1 _nodes.txt will call `destroy;` from within OnInit, that is during an
iteration of the global `ev_db`. Previously, concurrent modification
invalidated this iteration, resulting in a crash.

This still nullifes `oid`, dequeues all timers, and effectively calls
`builtin_end`.

`npc_data::deletion_pending` is extended to include a third state.
In addition to no deletion happening, and indicating when `npc_free` is
currently on the stack, it now also tracks whether the NPC is about to be
deleted by a timer.

This means that an NPC which is about to be deleted is still blocked from
triggering new events, much like an NPC actively being deleted would.

Starting or continuing an NPC dialog with an NPC that does not, or is about to
no longer exist, is now also blocked.

4 files changed, 36 insertions, 12 deletions

diff --git a/src/map/map.hpp b/src/map/map.hpp
index 7cf43d5..56d07a5 100644
--- a/src/map/map.hpp
+++ b/src/map/map.hpp
@@ -360,7 +360,11 @@ struct npc_data : block_list
     Opt0 option;
     short flag;
 
-    bool deletion_pending;
+    enum {
+        NOT_DELETING = 0,
+        DELETION_QUEUED = 1,
+        DELETION_ACTIVE = 2
+    } deletion_pending;
 
     Array<Timer, MAX_EVENTTIMER> eventtimer;
 
diff --git a/src/map/npc-parse.cpp b/src/map/npc-parse.cpp
index 47b851c..df1a09a 100644
--- a/src/map/npc-parse.cpp
+++ b/src/map/npc-parse.cpp
@@ -164,7 +164,7 @@ bool npc_load_warp(ast::npc::Warp& warp)
     nd->warp.xs = xs;
     nd->warp.ys = ys;
 
-    nd->deletion_pending = false;
+    nd->deletion_pending = npc_data::NOT_DELETING;
 
     npc_warp++;
     nd->bl_type = BL::NPC;
@@ -228,7 +228,7 @@ bool npc_load_shop(ast::npc::Shop& shop)
     nd->opt2 = Opt2::ZERO;
     nd->opt3 = Opt3::ZERO;
 
-    nd->deletion_pending = false;
+    nd->deletion_pending = npc_data::NOT_DELETING;
 
     npc_shop++;
     nd->bl_type = BL::NPC;
@@ -458,7 +458,7 @@ bool npc_load_script_none(ast::script::ScriptBody& body, ast::npc::ScriptNone& s
     nd->opt2 = Opt2::ZERO;
     nd->opt3 = Opt3::ZERO;
 
-    nd->deletion_pending = false;
+    nd->deletion_pending = npc_data::NOT_DELETING;
 
     npc_script++;
     nd->bl_type = BL::NPC;
@@ -568,7 +568,7 @@ bool npc_load_script_map(ast::script::ScriptBody& body, ast::npc::ScriptMap& scr
     nd->opt2 = Opt2::ZERO;
     nd->opt3 = Opt3::ZERO;
 
-    nd->deletion_pending = false;
+    nd->deletion_pending = npc_data::NOT_DELETING;
 
     npc_script++;
     nd->bl_type = BL::NPC;
diff --git a/src/map/npc.cpp b/src/map/npc.cpp
index e8d6d4b..8a6bead 100644
--- a/src/map/npc.cpp
+++ b/src/map/npc.cpp
@@ -357,7 +357,7 @@ void npc_eventtimer(TimerData *, tick_t, BlockId, NpcEvent data)
                     data);
         return;
     });
-    if ((nd = ev->nd) == nullptr || nd->deletion_pending == true)
+    if ((nd = ev->nd) == nullptr || nd->deletion_pending != npc_data::NOT_DELETING)
     {
         if (battle_config.error_log)
             PRINTF("npc_event: event not found [%s]\n"_fmt,
@@ -591,7 +591,7 @@ int npc_event(dumb_ptr<map_session_data> sd, NpcEvent eventname,
         ev.pos = ev2->pos;
     }
 
-    if ((nd = ev.nd) == nullptr || nd->deletion_pending == true)
+    if ((nd = ev.nd) == nullptr || nd->deletion_pending != npc_data::NOT_DELETING)
     {
         if (!mob_kill && battle_config.error_log)
             PRINTF("npc_event: event not found [%s]\n"_fmt,
@@ -774,6 +774,14 @@ int npc_click(dumb_ptr<map_session_data> sd, BlockId id)
 
     nd = map_id_is_npc(id);
 
+    // If someone clicked on an NPC that is about to no longer exist, then
+    // release them
+    if (nd->deletion_pending != npc_data::NOT_DELETING)
+    {
+        clif_scriptclose(sd, id);
+        return 1;
+    }
+
     if (nd->flag & 1)           // 無効化されている
         return 1;
 
@@ -818,7 +826,8 @@ int npc_scriptcont(dumb_ptr<map_session_data> sd, BlockId id)
 
     nd = map_id_is_npc(id);
 
-    if (!nd /* NPC was disposed? */)
+    // If the NPC is about to be deleted, release the PC
+    if (nd->deletion_pending != npc_data::NOT_DELETING)
     {
         clif_scriptclose(sd, id);
         npc_event_dequeue(sd);
@@ -1091,10 +1100,10 @@ void npc_propagate_update(dumb_ptr<npc_data> nd)
 
 void npc_free(dumb_ptr<npc_data> nd)
 {
-    if (nd == nullptr || nd->deletion_pending == true)
+    if (nd == nullptr || nd->deletion_pending == npc_data::DELETION_ACTIVE)
         return;
 
-    nd->deletion_pending = true;
+    nd->deletion_pending = npc_data::DELETION_ACTIVE;
     nd->flag |= 1;
     clif_clearchar(nd, BeingRemoveWhy::GONE);
     npc_propagate_update(nd);
diff --git a/src/map/script-fun.cpp b/src/map/script-fun.cpp
index 0014805..13b4bc8 100644
--- a/src/map/script-fun.cpp
+++ b/src/map/script-fun.cpp
@@ -1283,8 +1283,19 @@ void builtin_destroy(ScriptState *st)
         npc_event_dequeue(sd);
     }
 
+    // Cancel all existing timers on the NPC.
+    // They "would" never fire, and we don't want race conditions here.
+    for (int i = 0; i < MAX_EVENTTIMER; i++)
+    {
+        nd->eventtimer[i].cancel();
+    }
+    // Schedule the NPC to be freed on the next available tick.
+    // Scripts can be invoked under iteration of the ev_db global event
+    // database, and we don't want to invalidate active iterators.
+    nd->deletion_pending = npc_data::DELETION_QUEUED;
+    nd->eventtimer[0] = Timer(gettick(), std::bind(npc_free, nd));
+
     nd = nd->is_script();
-    npc_free(nd);
     st->oid = BlockId();
 
     if (!HARG(0))
@@ -1350,7 +1361,7 @@ void builtin_puppet(ScriptState *st)
     nd->npc_subtype = NpcSubtype::SCRIPT;
     npc_script++;
 
-    nd->deletion_pending = false;
+    nd->deletion_pending = npc_data::NOT_DELETING;
 
     nd->n = map_addnpc(nd->bl_m, nd);