fix: [Up-Port] handle NetworkManager Awake being overridden (#4244)

* fix: handle NetworkManager Awake being overridden

(cherry picked from commit e6693e9)

* Fix the XML doc and the double destroy

(cherry picked from commit 6f907ed)

* Fix malformed XML doc and simplify logger API

(cherry picked from commit ddc074e)

* dotnet-fix

(cherry picked from commit 5fe1822)

* Bump to minor version due to new API

* Update CHANGELOG

* Fix compilation error

* Update package version in ValidationExceptions.json

Updating validation exceptions NGO version number to match that of the package.

---------

Co-authored-by: Noel Stephens <noel.stephens@unity3d.com>

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -18,6 +18,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed a NullReferenceException that would be thrown if a MonoBehaviour derived from NetworkManager defines it's own Awake or OnEnable. (#4244)
 - Issue where the unified bootstrap created a second world that ran a duplicate of every default system next to the default world. (#4243)
 - Ensured a useful and human readable error is logged when a NetworkVariable is trying to serialize a private or protected type. (#4239)
 - Issue where `NetworkAnimator` logged an error for a trigger-conditioned transition into an Exit node, and a client that joined during that transition, or during one into a sub-state machine, did not synchronize with it. Transitions that pass through Exit, Entry or sub-state machine nodes now resolve to the state they reach, and a late-joining client cross-fades from the matching originating state when several states transition into the same destination. (#4238)

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

@@ -1100,11 +1100,18 @@ public void SetSingleton()
             OnSingletonReady?.Invoke();
         }
 
-        private void Awake()
+        /// <summary>
+        /// Unity calls Awake when loading an instance of a script component.
+        /// </summary>
+        /// <remarks>
+        /// Override this method to implement custom initialization logic for a derived <see cref="NetworkManager"/>.
+        /// Ensure you call <c>base.Awake()</c> in your override to allow the base initialization to happen.
+        /// </remarks>
+        protected virtual void Awake()
         {
             if (Log == null)
             {
-                Log = new ContextualLogger(this, false);
+                Log = new ContextualLogger(this);
             }
 
             NetworkConfig?.InitializePrefabs();
@@ -1124,7 +1131,14 @@ private void Awake()
             }
         }
 
-        private void OnEnable()
+        /// <summary>
+        /// Called when a component of an active GameObject is first enabled.
+        /// </summary>
+        /// <remarks>
+        /// Override this method to implement custom initialization logic for a derived <see cref="NetworkManager"/>.
+        /// Ensure you call  <c>base.OnEnable()</c> in your override to allow the base initialization to happen.
+        /// </remarks>
+        protected virtual void OnEnable()
         {
             if (RunInBackground)
             {
@@ -1485,6 +1499,11 @@ private void UnifiedApplyNetcodeConfig()
         /// <returns>returns true if <see cref="NetworkManager"/> started in server mode successfully; otherwise false</returns>
         public bool StartServer()
         {
+            if (Log == null)
+            {
+                Log = new ContextualLogger(this);
+                Log.Warning(new Context(LogLevel.Normal, "No logger defined. Ensure base.Awake() and base.OnEnable is called in any derived NetworkManager classes."));
+            }
             Log.CaptureFunctionCall();
 
             if (!CanStart(StartType.Server))
@@ -1578,6 +1597,11 @@ internal bool InternalStartServer()
         /// <returns>(<see cref="true"/>/<see cref="false"/>) returns true if <see cref="NetworkManager"/> started in client mode successfully.</returns>
         public bool StartClient()
         {
+            if (Log == null)
+            {
+                Log = new ContextualLogger(this);
+                Log.Warning(new Context(LogLevel.Normal, "No logger defined. Ensure base.Awake() and base.OnEnable is called in any derived NetworkManager classes."));
+            }
             Log.CaptureFunctionCall();
 
             if (!CanStart(StartType.Client))
@@ -1663,6 +1687,11 @@ internal bool InternalStartClient()
         /// <returns>(<see cref="true"/>/<see cref="false"/>) returns true if <see cref="NetworkManager"/> started in host mode successfully.</returns>
         public bool StartHost()
         {
+            if (Log == null)
+            {
+                Log = new ContextualLogger(this);
+                Log.Warning(new Context(LogLevel.Normal, "No logger defined. Ensure base.Awake() and base.OnEnable is called in any derived NetworkManager classes."));
+            }
             Log.CaptureFunctionCall();
 
             if (!CanStart(StartType.Host))
@@ -1848,7 +1877,7 @@ private void OnSceneUnloaded(Scene scene)
         {
             if (gameObject != null && scene == gameObject.scene)
             {
-                OnDestroy();
+                DestroyInternal();
             }
         }
 
@@ -1991,7 +2020,7 @@ internal void ShutdownInternal()
             }
         }
 
-        // Ensures that the NetworkManager is cleaned up before OnDestroy is run on NetworkObjects and NetworkBehaviours when quitting the application.
+        // Ensures that the NetworkManager is cleaned up before DestroyInternal is run on NetworkObjects and NetworkBehaviours when quitting the application.
         private void OnApplicationQuit()
         {
             // Abrupt shutdown (or immediate exit of play mode).
@@ -2005,7 +2034,7 @@ private void OnApplicationQuit()
             {
                 return;
             }
-            OnDestroy();
+            DestroyInternal();
 #if UNITY_EDITOR
             if (Singleton != null)
             {
@@ -2016,15 +2045,34 @@ private void OnApplicationQuit()
 
         private bool m_IsDestroyed = false;
 
+        /// <summary>
+        /// Called when a GameObject or component is about to be destroyed.
+        /// </summary>
+        /// <remarks>
+        /// Override this method to implement custom teardown logic for a derived <see cref="NetworkManager"/>.
+        /// Ensure you call <c>base.OnDestroy()</c> in your override to allow the base teardown to happen.
+        /// </remarks>
+        protected virtual void OnDestroy()
+        {
+            DestroyInternal();
+        }
+
+        // Internal destroy method that can't be overridden by derived classes.
         // Note that this gets also called manually by OnSceneUnloaded and OnApplicationQuit
-        private void OnDestroy()
+        private void DestroyInternal()
         {
             if (m_IsDestroyed)
             {
                 return;
             }
             m_IsDestroyed = true;
 
+            if (Log == null)
+            {
+                Log = new ContextualLogger(this);
+                Log.Warning(new Context(LogLevel.Normal, "No logger defined. Ensure base.Awake() and base.OnEnable is called in any derived NetworkManager classes."));
+            }
+
             try
             {
                 ShutdownInternal();
@@ -2120,7 +2168,7 @@ internal void OnValidate()
 
             if (Log == null)
             {
-                Log = new ContextualLogger(this, false);
+                Log = new ContextualLogger(this);
             }
 
             // Do a validation pass on NetworkConfig properties

com.unity.netcode.gameobjects/ValidationExceptions.json

@@ -3,12 +3,12 @@
     {
       "ValidationTest": "NDA Keyword Validation",
       "ExceptionMessage": "#if UNITY_7000_0_OR_NEWER",
-      "PackageVersion": "3.2.2"
+      "PackageVersion": "3.3.0"
     },
     {
       "ValidationTest": "NDA Keyword Validation",
       "ExceptionMessage": "File 'Editor/InScenePlacedProcessor.cs' contains deny-listed keyword pattern '(?<!BestPracticeUnderstandingPerformanceIn)unity[ _]*7' It must not be published",
-      "PackageVersion": "3.2.2"
+      "PackageVersion": "3.3.0"
     }
   ],
   "WarningExceptions": []

com.unity.netcode.gameobjects/package.json

@@ -2,7 +2,7 @@
   "name": "com.unity.netcode.gameobjects",
   "displayName": "Netcode for GameObjects",
   "description": "Netcode for GameObjects is a high-level netcode SDK that provides networking capabilities to GameObject/MonoBehaviour workflows within Unity and sits on top of underlying transport layer.",
-  "version": "3.2.2",
+  "version": "3.3.0",
   "unity": "6000.7",
   "unityRelease": "0b8",
   "dependencies": {
@@ -17,4 +17,4 @@
       "path": "Samples~/Bootstrap"
     }
   ]
-}
+}

testproject/Assets/Tests/Runtime/NetworkManagerTests.cs

@@ -198,4 +198,45 @@ private void OnServerStopped(bool obj)
             m_ServerNetworkManager.Shutdown();
         }
     }
+
+    internal class DerivedNetworkManagerTests
+    {
+        private class TestNetworkManager : NetworkManager
+        {
+            public void Awake() { }
+            public void OnEnable() { }
+        }
+
+        [UnityTest]
+        public IEnumerator ValidateDerivedNetworkManager()
+        {
+            var gameObject = new GameObject("TestNetworkManager");
+            var server = gameObject.AddComponent<TestNetworkManager>();
+            var mockTransport = server.gameObject.AddComponent<MockTransport>();
+            server.NetworkConfig ??= new NetworkConfig();
+            server.NetworkConfig.NetworkTransport = mockTransport;
+            var client = Object.Instantiate(server);
+
+            var isListening = server.StartServer();
+            Assert.That(isListening, Is.True, "Server failed to start!");
+
+            client.StartClient();
+
+            var timeoutHelper = new TimeoutHelper();
+            yield return NetcodeIntegrationTest.WaitForConditionOrTimeOut(() => client.IsListening, timeoutHelper);
+            Assert.That(timeoutHelper.TimedOut, Is.False, "Client timed out waiting to connect!");
+
+            Object.Destroy(client.gameObject);
+
+            server.Shutdown();
+            timeoutHelper = new TimeoutHelper();
+            yield return NetcodeIntegrationTest.WaitForConditionOrTimeOut(() => !server.IsListening, timeoutHelper);
+            Assert.That(timeoutHelper.TimedOut, Is.False, "Timed out waiting for server to shutdown!");
+
+            isListening = server.StartHost();
+            Assert.That(isListening, Is.True, "Host failed to start!");
+
+            Object.Destroy(gameObject);
+        }
+    }
 }