UnityEngine.UI.Selectable memory leak

Hi Unity UI team and @karl_jones,

Applies to Unity UI package 1.0.0

I have been struggling with memory issues in my app and just found out that part of it is at least due to UnityEngine.UI.Selectable keeping things in memory long after they should be removed.
It's due to m_Selectables being static and keeping everything in memory.

In OnDisable function the object is set to be removed. However the function to actually do the remove is never called.
In my opinion it should at least be called in OnDestroy, there is no implementation of that in UnityEngine.UI.Selectable

I was able to fix my memory issue calling the remove method from OnDestroy. That makes sure remove the object when it's fully destroyed. However that should not be needed if the remove function would actually be called from OnDisable.

I think this is happening because there is high possibility, specially on new scenes load that the item we are clearing from m_Selectables and the item next to it, one index lower are both marked with m_WillRemove = true.
Consider the case where you go through the loop in RemoveInvalidSelectables.
i = s_SelectableCount - 1
If you are going through the m_Selectables array, and you start with the last item which has m_WillRemove = true, and you see that, then you set the m_Selectables of the last item to the second last item.
If you do that and set m_SelectedableCount as m_SelectableCount = m_SelectebleCount - 1 that means you will never check the validity of that object as we now have one item less in the array. If m_Selectables was not null then m_Selectebles and m_Selectables[i - 1] have the same object.
I changed the function to actually set the m_Selectables to null before replacing the objects, the replacement then not beeing needed.

Code (CSharp):

1.  
2. // Remove from the list.
3.         protected override void OnDisable()
4.         {
5.             m_WillRemove = true;
6.             s_IsDirty = true;
7.             //SS call RemoveInvalidSelectes here?
8.  
9.             InstantClearState();
10.             base.OnDisable();
11.         }
12.  
13.         public override void OnDestroy() {
14.             RemoveInvalidSelectables();
15.         }
16.  
17.         private static void RemoveInvalidSelectables()
18.         {
19.             for (int i = s_SelectableCount - 1; i >= 0; --i)
20.             {
21.                 // Swap last element in array with element to be removed
22.                 //SS Original code commented out
23.                 //if (s_Selectables[i] == null || s_Selectables[i].m_WillRemove)
24.                 //    s_Selectables[i] = s_Selectables[--s_SelectableCount];
25.                 //SS Working code which removes the item
26.                 if (s_Selectables[i] == null || s_Selectables[i].m_WillRemove) {
27.                     s_Selectables[i] = null; //SS just remove the object as it's no longer in use.
28.                     s_Selectables[i] = s_Selectables[--s_SelectableCount];
29.                 }
30.             }
31.             s_IsDirty = false;
32.         }
33.  

One final thing, there is a comment in the code mentioning that you use static array instead of List due to performance. Wouldn't it be more suitable to use Dictionary or HashSet in this case to keep track of all the m_Selectables which would make removing of the object in question available without going through the array at all times and moving things between indexes.

best regard,
Snorri