FIX: InputTestFixture not resetting all global state on start (#1395, case 1329015).

Author: ekcoh

Assets/Tests/InputSystem/CoreTests_Editor.cs

@@ -2618,15 +2618,15 @@ public void Editor_LeavingPlayMode_DestroysAllActionStates()
         var action = new InputAction(binding: "<Gamepad>/buttonSouth");
         action.Enable();
 
-        Assert.That(InputActionState.s_GlobalList.length, Is.EqualTo(1));
+        Assert.That(InputActionState.s_GlobalState.globalList.length, Is.EqualTo(1));
         Assert.That(InputSystem.s_Manager.m_StateChangeMonitors.Length, Is.GreaterThan(0));
         Assert.That(InputSystem.s_Manager.m_StateChangeMonitors[0].count, Is.EqualTo(1));
 
         // Exit play mode.
         InputSystem.OnPlayModeChange(PlayModeStateChange.ExitingPlayMode);
         InputSystem.OnPlayModeChange(PlayModeStateChange.EnteredEditMode);
 
-        Assert.That(InputActionState.s_GlobalList.length, Is.Zero);
+        Assert.That(InputActionState.s_GlobalState.globalList.length, Is.Zero);
         Assert.That(InputSystem.s_Manager.m_StateChangeMonitors[0].listeners[0].control, Is.Null); // Won't get removed, just cleared.
     }
 

Assets/Tests/InputSystem/Plugins/EnhancedTouchTests.cs

@@ -26,7 +26,7 @@ public override void Setup()
         base.Setup();
 
         // Disable() will not reset this so default initialize it here.
-        Touch.s_HistoryLengthPerFinger = 64;
+        Touch.s_GlobalState.historyLengthPerFinger = 64;
 
         if (!TestContext.CurrentContext.Test.Properties.ContainsKey("EnhancedTouchDisabled"))
         {
@@ -45,16 +45,16 @@ public override void TearDown()
         EnhancedTouchSupport.Disable();
 
         // Make sure cleanup really did clean up.
-        Assert.That(Touch.s_Touchscreens.length, Is.EqualTo(0));
-        Assert.That(Touch.s_PlayerState, Is.EqualTo(default(Touch.FingerAndTouchState)));
+        Assert.That(Touch.s_GlobalState.touchscreens.length, Is.EqualTo(0));
+        Assert.That(Touch.s_GlobalState.playerState, Is.EqualTo(default(Touch.FingerAndTouchState)));
         #if UNITY_EDITOR
-        Assert.That(Touch.s_EditorState, Is.EqualTo(default(Touch.FingerAndTouchState)));
+        Assert.That(Touch.s_GlobalState.editorState, Is.EqualTo(default(Touch.FingerAndTouchState)));
         #endif
 
         // Some state is kept alive in-between Disable/Enable. Manually clean it out.
-        Touch.s_OnFingerDown = default;
-        Touch.s_OnFingerUp = default;
-        Touch.s_OnFingerMove = default;
+        Touch.s_GlobalState.onFingerDown = default;
+        Touch.s_GlobalState.onFingerUp = default;
+        Touch.s_GlobalState.onFingerMove = default;
 
         TouchSimulation.Destroy();
         TouchSimulation.s_Instance = m_OldTouchSimulationInstance;

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -17,6 +17,10 @@ however, it has to be formatted properly to pass verification tests.
   * Internally, a control is now again allowed to feed into the same action through more than one binding.
   * However, externally the control will be mentioned on the action's `InputAction.controls` list only once.
 
+### Fixed
+
+- Fixed an issue where mixing test cases based on `InputTestFixture` (using mocked `InputSystem`) and regular test cases (using real `InputSystem`) would lead to static state leaking between test cases causing random failures and unexpected/undefined behavior ([case 1329015](https://issuetracker.unity3d.com/product/unity/issues/guid/1329015)).
+
 ## [1.1.0-pre.6] - 2021-08-23
 
 ### Fixed

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

@@ -127,7 +127,7 @@ internal unsafe class InputActionState : IInputStateChangeMonitor, ICloneable, I
         public void Initialize(InputBindingResolver resolver)
         {
             ClaimDataFrom(resolver);
-            AddToGlobaList();
+            AddToGlobalList();
         }
 
         internal void ClaimDataFrom(InputBindingResolver resolver)
@@ -390,7 +390,7 @@ public void RestoreActionStates(UnmanagedMemory oldState)
             HookOnBeforeUpdate();
 
             // Fire notifications.
-            if (s_OnActionChange.length > 0)
+            if (s_GlobalState.onActionChange.length > 0)
             {
                 for (var i = 0; i < totalMapCount; ++i)
                 {
@@ -1955,7 +1955,7 @@ private void CallActionListeners(int actionIndex, InputActionMap actionMap, Inpu
         {
             // If there's no listeners, don't bother with anything else.
             var callbacksOnMap = actionMap.m_ActionCallbacks;
-            if (listeners.length == 0 && callbacksOnMap.length == 0 && s_OnActionChange.length == 0)
+            if (listeners.length == 0 && callbacksOnMap.length == 0 && s_GlobalState.onActionChange.length == 0)
                 return;
 
             var context = new InputAction.CallbackContext
@@ -1968,7 +1968,7 @@ private void CallActionListeners(int actionIndex, InputActionMap actionMap, Inpu
 
             // Global callback goes first.
             var action = context.action;
-            if (s_OnActionChange.length > 0)
+            if (s_GlobalState.onActionChange.length > 0)
             {
                 InputActionChange change;
                 switch (phase)
@@ -1987,7 +1987,7 @@ private void CallActionListeners(int actionIndex, InputActionMap actionMap, Inpu
                         return;
                 }
 
-                DelegateHelpers.InvokeCallbacksSafe(ref s_OnActionChange, action, change, "InputSystem.onActionChange");
+                DelegateHelpers.InvokeCallbacksSafe(ref s_GlobalState.onActionChange, action, change, "InputSystem.onActionChange");
             }
 
             // Run callbacks (if any) directly on action.
@@ -3607,7 +3607,7 @@ public UnmanagedMemory Clone()
         #region Global State
 
         /// <summary>
-        /// List of weak references to all action map states currently in the system.
+        /// Global state containing a list of weak references to all action map states currently in the system.
         /// </summary>
         /// <remarks>
         /// When the control setup in the system changes, we need a way for control resolution that
@@ -3616,25 +3616,44 @@ public UnmanagedMemory Clone()
         ///
         /// Both of these needs are served by this global list.
         /// </remarks>
-        internal static InlinedArray<GCHandle> s_GlobalList;
-        internal static CallbackArray<Action<object, InputActionChange>> s_OnActionChange;
-        internal static CallbackArray<Action<object>> s_OnActionControlsChanged;
+        internal struct GlobalState
+        {
+            internal InlinedArray<GCHandle> globalList;
+            internal CallbackArray<Action<object, InputActionChange>> onActionChange;
+            internal CallbackArray<Action<object>> onActionControlsChanged;
+        }
+
+        internal static GlobalState s_GlobalState;
+
+        internal static ISavedState SaveAndResetState()
+        {
+            // Save current state
+            var savedState = new SavedStructState<GlobalState>(
+                ref s_GlobalState,
+                (ref GlobalState state) => s_GlobalState = state, // restore
+                () => ResetGlobals()); // static dispose
+
+            // Reset global state
+            s_GlobalState = default;
+
+            return savedState;
+        }
 
-        private void AddToGlobaList()
+        private void AddToGlobalList()
         {
             CompactGlobalList();
             var handle = GCHandle.Alloc(this, GCHandleType.Weak);
-            s_GlobalList.AppendWithCapacity(handle);
+            s_GlobalState.globalList.AppendWithCapacity(handle);
         }
 
         private void RemoveMapFromGlobalList()
         {
-            var count = s_GlobalList.length;
+            var count = s_GlobalState.globalList.length;
             for (var i = 0; i < count; ++i)
-                if (s_GlobalList[i].Target == this)
+                if (s_GlobalState.globalList[i].Target == this)
                 {
-                    s_GlobalList[i].Free();
-                    s_GlobalList.RemoveAtByMovingTailWithCapacity(i);
+                    s_GlobalState.globalList[i].Free();
+                    s_GlobalState.globalList.RemoveAtByMovingTailWithCapacity(i);
                     break;
                 }
         }
@@ -3644,25 +3663,25 @@ private void RemoveMapFromGlobalList()
         /// </summary>
         private static void CompactGlobalList()
         {
-            var length = s_GlobalList.length;
+            var length = s_GlobalState.globalList.length;
             var head = 0;
             for (var i = 0; i < length; ++i)
             {
-                var handle = s_GlobalList[i];
+                var handle = s_GlobalState.globalList[i];
                 if (handle.IsAllocated && handle.Target != null)
                 {
                     if (head != i)
-                        s_GlobalList[head] = handle;
+                        s_GlobalState.globalList[head] = handle;
                     ++head;
                 }
                 else
                 {
                     if (handle.IsAllocated)
-                        s_GlobalList[i].Free();
-                    s_GlobalList[i] = default;
+                        s_GlobalState.globalList[i].Free();
+                    s_GlobalState.globalList[i] = default;
                 }
             }
-            s_GlobalList.length = head;
+            s_GlobalState.globalList.length = head;
         }
 
         internal static void NotifyListenersOfActionChange(InputActionChange change, object actionOrMapOrAsset)
@@ -3671,33 +3690,33 @@ internal static void NotifyListenersOfActionChange(InputActionChange change, obj
             Debug.Assert(actionOrMapOrAsset is InputAction || (actionOrMapOrAsset as InputActionMap)?.m_SingletonAction == null,
                 "Must not send notifications for changes made to hidden action maps of singleton actions");
 
-            DelegateHelpers.InvokeCallbacksSafe(ref s_OnActionChange, actionOrMapOrAsset, change, "onActionChange");
+            DelegateHelpers.InvokeCallbacksSafe(ref s_GlobalState.onActionChange, actionOrMapOrAsset, change, "onActionChange");
             if (change == InputActionChange.BoundControlsChanged)
-                DelegateHelpers.InvokeCallbacksSafe(ref s_OnActionControlsChanged, actionOrMapOrAsset, "onActionControlsChange");
+                DelegateHelpers.InvokeCallbacksSafe(ref s_GlobalState.onActionControlsChanged, actionOrMapOrAsset, "onActionControlsChange");
         }
 
         /// <summary>
         /// Nuke global state we have to keep track of action map states.
         /// </summary>
-        internal static void ResetGlobals()
+        private static void ResetGlobals()
         {
             DestroyAllActionMapStates();
-            for (var i = 0; i < s_GlobalList.length; ++i)
-                if (s_GlobalList[i].IsAllocated)
-                    s_GlobalList[i].Free();
-            s_GlobalList.length = 0;
-            s_OnActionChange.Clear();
-            s_OnActionControlsChanged.Clear();
+            for (var i = 0; i < s_GlobalState.globalList.length; ++i)
+                if (s_GlobalState.globalList[i].IsAllocated)
+                    s_GlobalState.globalList[i].Free();
+            s_GlobalState.globalList.length = 0;
+            s_GlobalState.onActionChange.Clear();
+            s_GlobalState.onActionControlsChanged.Clear();
         }
 
         // Walk all maps with enabled actions and add all enabled actions to the given list.
         internal static int FindAllEnabledActions(List<InputAction> result)
         {
             var numFound = 0;
-            var stateCount = s_GlobalList.length;
+            var stateCount = s_GlobalState.globalList.length;
             for (var i = 0; i < stateCount; ++i)
             {
-                var handle = s_GlobalList[i];
+                var handle = s_GlobalState.globalList[i];
                 if (!handle.IsAllocated)
                     continue;
                 var state = (InputActionState)handle.Target;
@@ -3761,15 +3780,15 @@ internal static void OnDeviceChange(InputDevice device, InputDeviceChange change
                 change == InputDeviceChange.SoftReset || change == InputDeviceChange.HardReset,
                 "Should only be called for relevant changes");
 
-            for (var i = 0; i < s_GlobalList.length; ++i)
+            for (var i = 0; i < s_GlobalState.globalList.length; ++i)
             {
-                var handle = s_GlobalList[i];
+                var handle = s_GlobalState.globalList[i];
                 if (!handle.IsAllocated || handle.Target == null)
                 {
                     // Stale entry in the list. State has already been reclaimed by GC. Remove it.
                     if (handle.IsAllocated)
-                        s_GlobalList[i].Free();
-                    s_GlobalList.RemoveAtWithCapacity(i);
+                        s_GlobalState.globalList[i].Free();
+                    s_GlobalState.globalList.RemoveAtWithCapacity(i);
                     --i;
                     continue;
                 }
@@ -3833,15 +3852,15 @@ internal static void DeferredResolutionOfBindings()
             ++InputActionMap.s_DeferBindingResolution;
             try
             {
-                for (var i = 0; i < s_GlobalList.length; ++i)
+                for (var i = 0; i < s_GlobalState.globalList.length; ++i)
                 {
-                    var handle = s_GlobalList[i];
+                    var handle = s_GlobalState.globalList[i];
                     if (!handle.IsAllocated || handle.Target == null)
                     {
                         // Stale entry in the list. State has already been reclaimed by GC. Remove it.
                         if (handle.IsAllocated)
-                            s_GlobalList[i].Free();
-                        s_GlobalList.RemoveAtWithCapacity(i);
+                            s_GlobalState.globalList[i].Free();
+                        s_GlobalState.globalList.RemoveAtWithCapacity(i);
                         --i;
                         continue;
                     }
@@ -3859,9 +3878,9 @@ internal static void DeferredResolutionOfBindings()
 
         internal static void DisableAllActions()
         {
-            for (var i = 0; i < s_GlobalList.length; ++i)
+            for (var i = 0; i < s_GlobalState.globalList.length; ++i)
             {
-                var handle = s_GlobalList[i];
+                var handle = s_GlobalState.globalList[i];
                 if (!handle.IsAllocated || handle.Target == null)
                     continue;
                 var state = (InputActionState)handle.Target;
@@ -3885,16 +3904,16 @@ internal static void DisableAllActions()
         /// </remarks>
         internal static void DestroyAllActionMapStates()
         {
-            while (s_GlobalList.length > 0)
+            while (s_GlobalState.globalList.length > 0)
             {
-                var index = s_GlobalList.length - 1;
-                var handle = s_GlobalList[index];
+                var index = s_GlobalState.globalList.length - 1;
+                var handle = s_GlobalState.globalList[index];
                 if (!handle.IsAllocated || handle.Target == null)
                 {
                     // Already destroyed.
                     if (handle.IsAllocated)
-                        s_GlobalList[index].Free();
-                    s_GlobalList.RemoveAtWithCapacity(index);
+                        s_GlobalState.globalList[index].Free();
+                    s_GlobalState.globalList.RemoveAtWithCapacity(index);
                     continue;
                 }
 

Packages/com.unity.inputsystem/InputSystem/InputSystem.cs

@@ -14,6 +14,7 @@
 using UnityEngine.InputSystem.XInput;
 using UnityEngine.InputSystem.Utilities;
 using UnityEngine.Profiling;
+
 #if UNITY_EDITOR
 using UnityEditor;
 using UnityEngine.InputSystem.Editor;
@@ -2917,13 +2918,13 @@ public static event Action<object, InputActionChange> onActionChange
             {
                 if (value == null)
                     throw new ArgumentNullException(nameof(value));
-                InputActionState.s_OnActionChange.AddCallback(value);
+                InputActionState.s_GlobalState.onActionChange.AddCallback(value);
             }
             remove
             {
                 if (value == null)
                     throw new ArgumentNullException(nameof(value));
-                InputActionState.s_OnActionChange.RemoveCallback(value);
+                InputActionState.s_GlobalState.onActionChange.RemoveCallback(value);
             }
         }
 
@@ -3519,7 +3520,6 @@ private static void Destroy()
             //       state repeatedly during tests but we want to not create InputSystemObject
             //       over and over.
 
-            InputActionState.ResetGlobals();
             s_Manager.Destroy();
             if (s_RemoteConnection != null)
                 Object.DestroyImmediate(s_RemoteConnection);
@@ -3552,7 +3552,10 @@ internal struct State
             [SerializeField] public InputEditorUserSettings.SerializedState userSettings;
             [SerializeField] public string systemObject;
             #endif
-            ////REVIEW: preserve InputUser state? (if even possible)
+            ////TODO: make these saved states capable of surviving domain reloads
+            [NonSerialized] public ISavedState inputActionState;
+            [NonSerialized] public ISavedState touchState;
+            [NonSerialized] public ISavedState inputUserState;
         }
 
         private static Stack<State> s_SavedStateStack;
@@ -3590,6 +3593,9 @@ internal static void SaveAndReset(bool enableRemoting = false, IInputRuntime run
                 userSettings = InputEditorUserSettings.s_Settings,
                 systemObject = JsonUtility.ToJson(s_SystemObject),
                 #endif
+                inputActionState = InputActionState.SaveAndResetState(),
+                touchState = EnhancedTouch.Touch.SaveAndResetState(),
+                inputUserState = InputUser.SaveAndResetState()
             });
 
             Reset(enableRemoting, runtime ?? InputRuntime.s_Instance); // Keep current runtime.
@@ -3603,11 +3609,20 @@ internal static void Restore()
         {
             Debug.Assert(s_SavedStateStack != null && s_SavedStateStack.Count > 0);
 
+            // Load back previous state.
+            var state = s_SavedStateStack.Pop();
+
+            state.inputUserState.StaticDisposeCurrentState();
+            state.touchState.StaticDisposeCurrentState();
+            state.inputActionState.StaticDisposeCurrentState();
+
             // Nuke what we have.
             Destroy();
 
-            // Load back previous state.
-            var state = s_SavedStateStack.Pop();
+            state.inputUserState.RestoreSavedState();
+            state.touchState.RestoreSavedState();
+            state.inputActionState.RestoreSavedState();
+
             s_Manager = state.manager;
             s_Remote = state.remote;
             s_RemoteConnection = state.remoteConnection;