Diagnostics for the edge-of-loaded-area crashes, plus spawn hardening

Code/client/Games/Skyrim/Actor.cpp

@@ -234,7 +234,16 @@ GamePtr<Actor> Actor::Create(TESNPC* apBaseForm) noexcept
     if (pCell && !(pCell->cellFlags & 1))
         pCell = nullptr;
 
-    ModManager::Get()->Spawn(position, rotation, pCell, pWorldSpace, pActor);
+    // If the game doesn't place the reference, the actor has no form ID or handle and the returned GamePtr
+    // holds its only reference, so callers that keep a raw Actor* use it after it is freed. Moving it queues
+    // engine work that can then run on the freed actor (crash at SkyrimSE 13641 under MoveRefToNewSpace,
+    // with a form ID 0 actor). Don't hand out an actor that was never placed.
+    const uint32_t cRefHandle = ModManager::Get()->Spawn(position, rotation, pCell, pWorldSpace, pActor);
+    if (cRefHandle == 0 || pActor->formID == 0)
+    {
+        spdlog::error("Actor::Create: failed to place actor with base form {:X}", apBaseForm ? apBaseForm->formID : 0);
+        return {};
+    }
 
     pActor->ForcePosition(position);
 
@@ -439,12 +448,51 @@ bool TP_MAKE_THISCALL(HookRemoveSpell, Actor, MagicItem* apSpell)
     return result;
 }
 
+
+namespace
+{
+// Diagnostic for SkyrimSE+1AF607: Characters that end up with form ID 0 later crash on a stale parent cell.
+// Logs the caller chain as module+offset so the code that creates or loads them can be found.
+void LogFormIdZeroActor(const char* acpWhere, const Actor* apActor) noexcept
+{
+    void* frames[24]{};
+    const USHORT cCount = RtlCaptureStackBackTrace(1, static_cast<DWORD>(std::size(frames)), frames, nullptr);
+
+    std::string stack;
+    for (USHORT i = 0; i < cCount; ++i)
+    {
+        HMODULE hModule = nullptr;
+        char moduleName[MAX_PATH] = "?";
+        if (GetModuleHandleExA(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, static_cast<LPCSTR>(frames[i]), &hModule) && hModule)
+        {
+            char path[MAX_PATH]{};
+            if (GetModuleFileNameA(hModule, path, MAX_PATH))
+            {
+                const char* cpName = std::strrchr(path, '\\');
+                strncpy_s(moduleName, cpName ? cpName + 1 : path, _TRUNCATE);
+            }
+        }
+
+        const auto cOffset = reinterpret_cast<uintptr_t>(frames[i]) - reinterpret_cast<uintptr_t>(hModule);
+        stack += fmt::format(" {}+{:X}", moduleName, cOffset);
+    }
+
+    const auto* pNpc = Cast<TESNPC>(apActor->baseForm);
+    spdlog::warn("[FormIdZeroActor] {}: actor {}, base {:X} ({}), parent cell {}, flags {:X}, thread {}, stack:{}", acpWhere, fmt::ptr(apActor),
+                 apActor->baseForm ? apActor->baseForm->formID : 0, pNpc && pNpc->fullName.value.AsAscii() ? pNpc->fullName.value.AsAscii() : "", fmt::ptr(apActor->parentCell),
+                 apActor->flags, GetCurrentThreadId(), stack);
+}
+} // namespace
+
 Actor* TP_MAKE_THISCALL(HookCharacterConstructor, Actor)
 {
     TP_EMPTY_HOOK_PLACEHOLDER;
 
     TiltedPhoques::ThisCall(RealCharacterConstructor, apThis);
 
+    if (apThis->formID == 0)
+        LogFormIdZeroActor("Character constructor", apThis);
+
     return apThis;
 }
 
@@ -454,6 +502,9 @@ Actor* TP_MAKE_THISCALL(HookCharacterConstructor2, Actor, uint8_t aUnk)
 
     TiltedPhoques::ThisCall(RealCharacterConstructor2, apThis, aUnk);
 
+    if (apThis->formID == 0)
+        LogFormIdZeroActor("Character constructor 2", apThis);
+
     return apThis;
 }
 
@@ -954,6 +1005,9 @@ bool TP_MAKE_THISCALL(HookSpawnActorInWorld, Actor)
         spdlog::info("Spawn Actor: {:X}, and NPC {}", apThis->formID, pNpc->fullName.value);
     }
 
+    if (apThis->formID == 0)
+        LogFormIdZeroActor("UpdateReference3D", apThis);
+
     return TiltedPhoques::ThisCall(RealSpawnActorInWorld, apThis);
 }
 

Code/client/Services/Debug/DebugService.cpp

@@ -87,6 +87,8 @@ void __declspec(noinline) DebugService::PlaceActorInWorld() noexcept
     const auto pPlayerBaseForm = static_cast<TESNPC*>(PlayerCharacter::Get()->baseForm);
 
     auto pActor = Actor::Create(pPlayerBaseForm);
+    if (!pActor)
+        return;
 
     const Inventory inventory = PlayerCharacter::Get()->GetActorInventory();
     pActor->SetActorInventory(inventory);

Code/client/Services/Generic/CharacterService.cpp

@@ -227,6 +227,26 @@ void CharacterService::DeleteTempActor(const uint32_t aFormId) noexcept
     }
 }
 
+namespace
+{
+// Diagnostic for actors that loop between spawn and removal (e.g. a follower both saves share): where the actor
+// is, and where the player is, at each step.
+void LogActorPlacement(const char* acpWhat, uint32_t aFormId) noexcept
+{
+    auto* pActor = Cast<Actor>(TESForm::GetById(aFormId));
+    const auto* pPlayer = PlayerCharacter::Get();
+    if (!pActor || !pPlayer)
+        return;
+
+    const auto* pCell = pActor->GetParentCellEx();
+    const auto* pPlayerCell = pPlayer->GetParentCellEx();
+    spdlog::info("[ActorPlacement] {} {:X}: cell {:X}, worldspace {:X}, position ({:.0f}, {:.0f}, {:.0f}), remote {}; player cell {:X}, worldspace {:X}, position ({:.0f}, {:.0f}, {:.0f})",
+                 acpWhat, aFormId, pCell ? pCell->formID : 0, pActor->GetWorldSpace() ? pActor->GetWorldSpace()->formID : 0, pActor->position.x, pActor->position.y,
+                 pActor->position.z, pActor->GetExtension()->IsRemote(), pPlayerCell ? pPlayerCell->formID : 0, pPlayer->GetWorldSpace() ? pPlayer->GetWorldSpace()->formID : 0,
+                 pPlayer->position.x, pPlayer->position.y, pPlayer->position.z);
+}
+} // namespace
+
 void CharacterService::OnActorAdded(const ActorAddedEvent& acEvent) noexcept
 {
     Actor* pActor = Cast<Actor>(TESForm::GetById(acEvent.FormId));
@@ -260,11 +280,15 @@ void CharacterService::OnActorAdded(const ActorAddedEvent& acEvent) noexcept
     m_world.emplace_or_replace<FormIdComponent>(entity, acEvent.FormId);
     m_world.emplace_or_replace<EarlyAnimationBufferComponent>(entity);
 
+    LogActorPlacement("Added", acEvent.FormId);
+
     ProcessNewEntity(entity);
 }
 
 void CharacterService::OnActorRemoved(const ActorRemovedEvent& acEvent) noexcept
 {
+    LogActorPlacement("Removing", acEvent.FormId);
+
     if (auto* pActor = Cast<Actor>(TESForm::GetById(acEvent.FormId)))
         pActor->GetExtension()->Reconciliation = ActorExtension::ReconciliationStage::None;
 
@@ -485,8 +509,15 @@ void CharacterService::OnAssignCharacter(const AssignCharacterResponse& acMessag
 
         ReconcileActorData(cEntity, pActor, acMessage.OwnershipEpoch, actorData, true, false);
 
+        spdlog::info("[ActorPlacement] Moving remote {:X} to owner's worldspace {:X}:{:X}, cell {:X}:{:X}, position ({:.0f}, {:.0f}, {:.0f})", pActor->formID,
+                     acMessage.WorldSpaceId.ModId, acMessage.WorldSpaceId.BaseId, acMessage.CellId.ModId, acMessage.CellId.BaseId, acMessage.Position.x, acMessage.Position.y,
+                     acMessage.Position.z);
+        LogActorPlacement("Before move", pActor->formID);
+
         MoveActor(pActor, acMessage.WorldSpaceId, acMessage.CellId, acMessage.Position);
 
+        LogActorPlacement("After move", pActor->formID);
+
         // The owner's leveled pick rides the assignment response for actors we discovered ourselves
         ApplyLeveledNpcPick(pActor, acMessage.LeveledNpcPickId);
     }
@@ -588,6 +619,11 @@ void CharacterService::OnCharacterSpawn(const CharacterSpawnRequest& acMessage)
     if (!pActor)
     {
         spdlog::error("Actor object {:X} could not be created.", acMessage.ServerId);
+
+        // The custom form branch created this entity for the new actor
+        if (acMessage.FormId == GameId{} && entity)
+            m_world.destroy(*entity);
+
         return;
     }