fix(NetworkClient): Defer ApplySpawnPayload until OnObjectSpawnFinished

#3097 (#3961)

* fix(NetworkClient): Defer ApplySpawnPayload until OnObjectSpawnFinished
Virtually eliminates null refs with GO/NI/NB SyncVars by getting all objects into spawned dictionary before applying payloads and invoking hooks.
- No change to normal spawning, only initial spawn
- Uses and clears a static dictionary, so no garbage allocation

See ticket #3097

* Update NetworkClient.cs

* Update Assets/Mirror/Core/NetworkClient.cs

---------

Co-authored-by: mischa <[email protected]>

Assets/Mirror/Core/NetworkClient.cs

@@ -1107,13 +1107,14 @@ public static bool AddPlayer()
         // spawning ////////////////////////////////////////////////////////////
         internal static void ApplySpawnPayload(NetworkIdentity identity, SpawnMessage message)
         {
+            // add to spawned first because DeserializeClient may need it for SyncVars
+            spawned[message.netId] = identity;
+
             if (message.assetId != 0)
                 identity.assetId = message.assetId;
 
             if (!identity.gameObject.activeSelf)
-            {
                 identity.gameObject.SetActive(true);
-            }
 
             // apply local values for VR support
             identity.transform.localPosition = message.position;
@@ -1124,8 +1125,11 @@ internal static void ApplySpawnPayload(NetworkIdentity identity, SpawnMessage me
             // the below DeserializeClient call invokes SyncVarHooks.
             // flags always need to be initialized before that.
             // fixes: https://github.com/MirrorNetworking/Mirror/issues/3259
-            identity.isOwned = message.isOwner;
             identity.netId = message.netId;
+            identity.isOwned = message.isOwner;
+
+            if (identity.isOwned)
+                connection?.owned.Add(identity);
 
             if (message.isLocalPlayer)
                 InternalAddPlayer(identity);
@@ -1148,9 +1152,6 @@ internal static void ApplySpawnPayload(NetworkIdentity identity, SpawnMessage me
                 }
             }
 
-            spawned[message.netId] = identity;
-            if (identity.isOwned) connection?.owned.Add(identity);
-
             // the initial spawn with OnObjectSpawnStarted/Finished calls all
             // object's OnStartClient/OnStartLocalPlayer after they were all
             // spawned.
@@ -1299,9 +1300,12 @@ internal static void OnObjectSpawnStarted(ObjectSpawnStartedMessage _)
         {
             // Debug.Log("SpawnStarted");
             PrepareToSpawnSceneObjects();
+            pendingSpawns.Clear();
             isSpawnFinished = false;
         }
 
+        static readonly Dictionary<NetworkIdentity, SpawnMessage> pendingSpawns = new Dictionary<NetworkIdentity, SpawnMessage>();
+
         internal static void OnObjectSpawnFinished(ObjectSpawnFinishedMessage _)
         {
             // paul: Initialize the objects in the same order as they were
@@ -1313,10 +1317,25 @@ internal static void OnObjectSpawnFinished(ObjectSpawnFinishedMessage _)
                 // they are destroyed. for safety, let's double check here.
                 if (identity != null)
                 {
+                    // We may have deferred ApplySpawnPayload in OnSpawn
+                    // to avoid cross-reference race conditions with SyncVars.
+                    // Apply payload before invoking OnStartClient etc. callbacks
+                    // so that all data is there when they are invoked.
+                    // Note that Interest Management may not have updated spawned
+                    // dictionary yet, so not all identities may be in pendingSpawns.
+                    // Generally that's user error in their code, so we don't throw
+                    // a warning here, but keep the warning code for debugging if needed.
+                    if (pendingSpawns.TryGetValue(identity, out SpawnMessage message))
+                        ApplySpawnPayload(identity, message);
+                    //else
+                    //    Debug.LogWarning($"Expected pendingSpawns to contain {identity}: {identity.netId} but didn't");
+
                     BootstrapIdentity(identity);
                 }
                 else Debug.LogWarning("Found null entry in NetworkClient.spawned. This is unexpected. Was the NetworkIdentity not destroyed properly?");
             }
+
+            pendingSpawns.Clear();
             isSpawnFinished = true;
         }
 
@@ -1427,7 +1446,18 @@ internal static void OnSpawn(SpawnMessage message)
             // Debug.Log($"Client spawn handler instantiating netId={msg.netId} assetID={msg.assetId} sceneId={msg.sceneId:X} pos={msg.position}");
             if (FindOrSpawnObject(message, out NetworkIdentity identity))
             {
-                ApplySpawnPayload(identity, message);
+                if (isSpawnFinished)
+                {
+                    ApplySpawnPayload(identity, message);
+                }
+                else
+                {
+                    // Defer ApplySpawnPayload until OnObjectSpawnFinished
+                    // add to spawned because later when we ApplySpawnPayload
+                    // there may be SyncVars that cross-reference other objects
+                    spawned[message.netId] = identity;
+                    pendingSpawns[identity] = message;
+                }
             }
         }