fix: Attachables destroy order of operations

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -26,6 +26,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where either an `AttachableBehaviour` or an `AttachableNode` can throw an exception if they are attached during a scene unload where one of the two persists the scene unload event and the other does not. (#3931)
 - Fixed issue where attempts to use `NetworkLog` when there is no `NetworkManager` instance would result in an exception. (#3917)
 
 ### Security

com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs

@@ -278,6 +278,14 @@ internal void ForceDetach()
         /// <inheritdoc/>
         public override void OnNetworkPreDespawn()
         {
+            // If the NetworkObject is being destroyed and not completely detached, then destroy the GameObject for
+            // this attachable since the associated default parent is being destroyed.
+            if (IsDestroying && m_AttachState != AttachState.Detached)
+            {
+                Destroy(gameObject);
+                return;
+            }
+
             if (NetworkManager.ShutdownInProgress || AutoDetach.HasFlag(AutoDetachTypes.OnDespawn))
             {
                 ForceDetach();
@@ -286,7 +294,7 @@ public override void OnNetworkPreDespawn()
         }
 
         /// <summary>
-        /// This will apply the final attach or detatch state based on the current value of <see cref="m_AttachedNodeReference"/>.
+        /// This will apply the final attach or detach state based on the current value of <see cref="m_AttachedNodeReference"/>.
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private void UpdateAttachedState()
@@ -392,7 +400,8 @@ internal void ForceComponentChange(bool isAttaching, bool forcedChange)
 
             foreach (var componentControllerEntry in ComponentControllers)
             {
-                if (componentControllerEntry.AutoTrigger.HasFlag(triggerType))
+                // Only if the component controller still exists and has the appropriate flag.
+                if (componentControllerEntry.ComponentController && componentControllerEntry.AutoTrigger.HasFlag(triggerType))
                 {
                     componentControllerEntry.ComponentController.ForceChangeEnabled(componentControllerEntry.EnableOnAttach ? isAttaching : !isAttaching, forcedChange);
                 }
@@ -457,7 +466,7 @@ public void Attach(AttachableNode attachableNode)
         /// </summary>
         internal void InternalDetach()
         {
-            if (m_AttachableNode)
+            if (!IsDestroying && m_AttachableNode && !m_AttachableNode.IsDestroying)
             {
                 if (m_DefaultParent)
                 {

com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs

@@ -71,20 +71,20 @@ public override void OnNetworkPreDespawn()
         {
             for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
             {
-                if (!m_AttachedBehaviours[i])
+                var attachable = m_AttachedBehaviours[i];
+                if (!attachable)
                 {
                     continue;
                 }
-                // If we don't have authority but should detach on despawn,
-                // then proceed to detach.
-                if (!m_AttachedBehaviours[i].HasAuthority)
+
+                if (attachable.HasAuthority && attachable.IsSpawned)
                 {
-                    m_AttachedBehaviours[i].ForceDetach();
+                    // Detach the normal way with authority
+                    attachable.Detach();
                 }
-                else
+                else if (!attachable.HasAuthority || !attachable.IsDestroying)
                 {
-                    // Detach the normal way with authority
-                    m_AttachedBehaviours[i].Detach();
+                    attachable.ForceDetach();
                 }
             }
         }

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -643,6 +643,20 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId)
         /// </summary>
         public ulong OwnerClientId { get; internal set; }
 
+        /// <summary>
+        /// Returns true if the NetworkObject is in the middle of being destroyed or
+        /// if there is no valid assigned NetworkObject.
+        /// </summary>
+        /// <remarks>
+        /// <see cref="NetworkObject.IsDestroying"/>
+        /// </remarks>
+        internal bool IsDestroying { get; private set; }
+
+        internal void SetDestroying()
+        {
+            IsDestroying = true;
+        }
+
         /// <summary>
         /// Updates properties with network session related
         /// dependencies such as a NetworkObject's spawned

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -1688,8 +1688,51 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
             }
         }
 
+        /// <summary>
+        /// Returns true if the NetworkObject is in the middle of being destroyed.
+        /// </summary>
+        /// <remarks>
+        /// This is particularly useful when determining if something is being de-spawned
+        /// normally or if it is being de-spawned because the NetworkObject/GameObject is
+        /// being destroyed.
+        /// </remarks>
+        internal bool IsDestroying { get; private set; }
+
+        /// <summary>
+        /// Applies the despawning flag for the local instance and
+        /// its child NetworkBehaviours. Private to assure this is
+        /// only invoked from within OnDestroy.
+        /// </summary>
+        private void SetIsDestroying()
+        {
+            IsDestroying = true;
+
+            // Exit early if null
+            if (m_ChildNetworkBehaviours == null)
+            {
+                return;
+            }
+
+            foreach (var childBehaviour in m_ChildNetworkBehaviours)
+            {
+                // Just ignore and continue processing through the entries
+                if (!childBehaviour)
+                {
+                    continue;
+                }
+
+                // Keeping the property a private set to assure this is
+                // the only way it can be set as it should never be reset
+                // back to false once invoked.
+                childBehaviour.SetDestroying();
+            }
+        }
+
         private void OnDestroy()
         {
+            // Apply the is destroying flag
+            SetIsDestroying();
+
             var networkManager = NetworkManager;
             // If no NetworkManager is assigned, then just exit early
             if (!networkManager)

com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs

@@ -55,7 +55,7 @@ protected override void OnServerAndClientsCreated()
             attachableNetworkObject.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable);
 
             // The target prefab that the source prefab will attach
-            // will be parented under the target prefab.
+            // to will be parented under the target prefab.
             m_TargetNodePrefabA = CreateNetworkObjectPrefab("TargetA");
             m_TargetNodePrefabB = CreateNetworkObjectPrefab("TargetB");
             var sourceChild = new GameObject("SourceChild");
@@ -676,10 +676,13 @@ internal class TestAttachable : AttachableBehaviour
             public GameObject DefaultParent => m_DefaultParent;
             public AttachState State => m_AttachState;
 
+            public bool DestroyWithScene;
+
             public override void OnNetworkSpawn()
             {
                 AttachStateChange += OnAttachStateChangeEvent;
                 name = $"{name}-{NetworkManager.LocalClientId}";
+                NetworkObject.DestroyWithScene = DestroyWithScene;
                 base.OnNetworkSpawn();
             }
 
@@ -780,9 +783,16 @@ public bool CheckForState(bool checkAttached, bool checkEvent)
         /// </summary>
         internal class TestNode : AttachableNode
         {
+            public bool DestroyWithScene;
             public bool OnAttachedInvoked { get; private set; }
             public bool OnDetachedInvoked { get; private set; }
 
+            public override void OnNetworkSpawn()
+            {
+                NetworkObject.DestroyWithScene = DestroyWithScene;
+                base.OnNetworkSpawn();
+            }
+
             public bool IsAttached(AttachableBehaviour attachableBehaviour)
             {
                 return m_AttachedBehaviours.Contains(attachableBehaviour);