Mixedrealitytoolkit-unity: NullChecking interfaces

[Alexees]

Describe the bug

It's not a bug in that sense, but rather something that could be a problem for the MRTK and I wanted to make staff and contributors aware of it, unless they already know.

Basic premise:

1. Throughout the MRTK, interfaces are used for most if not all services being part of it
2. Unity uses overridden versions of the comparison operators because of it's native and managed side implementation

The problem comes in where an interface is checked for null. Checking an interface for null does not call the implementors version of the comparison operators, which in case of UnityEngine.Object derived classes, are important to call. This means the following:

In the case of a UnityEngine.Object derived class being destroyed, its interface being checked for null before the actual destruction happened and a subsequent access to the class, a NullReferenceException will be thrown.

To reproduce

As an example:

MixedRealityInputModule's ProcessMrtkPointerLost method includes the following line:

https://github.com/microsoft/MixedRealityToolkit-Unity/blob/ac2b8108f85aaef32b7f1d6022c1af1d19b154b2/Assets/MixedRealityToolkit.Services/InputSystem/MixedRealityInputModule.cs#L164

In case of the Hololens 1 with the use of Holographic Remoting and moving hands in and out of view, this will be triggered quite often.
Do a proper null check on the pointer in an if statement and see it not being entered.
VS confuses you with an actual hint in that it displays the value of pointer as being "null" (string). This seems to be the ToString() overloaded version for UnityEngine.Object in cases where it's null in the Unity sense.
The fact this is not throwing here is that ProcessMrtkPointerLost is being called from a try block, which might be another bug as I don't think you'd ever need try blocks in regular Unity code.

[Alexees]

@davidkline-ms what do you think about this?
Looks to me as if either all interfaces that are derived by MonoBehaviours should be converted into one, or we need a system that makes sure we check interfaces like this:
if (interfaceImplementor == null || interfaceImplementor.Equals(null)

That's the page I found this on, there are a couple more on the net dealing with this:
https://answers.unity.com/questions/586144/destroyed-monobehaviour-not-comparing-to-null.html?_ga=2.186670958.713884393.1575376097-2072500192.1561120911

[Alexees]

This is a good example for it too:

https://github.com/microsoft/MixedRealityToolkit-Unity/blob/45d831ecef315191dbd3204cbe37429537c3596b/Assets/MixedRealityToolkit.Providers/WindowsMixedReality/WindowsMixedRealityDeviceManager.cs#L664-L690

• 679: controller.InputSource could be null, is an interface and not checked. Would need the special treatment
• 676: pointer is checked the wrong way, it's an interface too
• 682: controller.Visualizer definitely threw a NullRef during engine shutdown, because it's an interface as well

[Alexees]

Couple of solutions:

1. generic NullCheck helper used throughout the code
2. Proxy-Object passed instead of the real instance performing that check, providing real null

[Alexees]

This is an untested implementation how both of these could look like in combination:

public static class UnityObjectNullChecker
{
    public static T CheckNull<T>(T reference) where T : class => (reference == null || reference.Equals(null)) ? null : reference;
    public static bool IsNull<T>(T reference) where T : class => CheckNull(reference) == null;
    public static bool IsNotNull<T>(T reference) where T : class => CheckNull(reference) != null;
}

public class UnityObjectProxy<T> where T : class
{
    private T UncheckedReference;
    public T CheckedReference => UnityObjectNullChecker.CheckNull(UncheckedReference);
    public UnityObjectProxy(T reference)
    {
        UncheckedReference = reference;
    }

    public bool TryGetReference(out T checkedReference)
    {
        checkedReference = UncheckedReference;
        return UnityObjectNullChecker.IsNotNull(checkedReference);
    }
}

[Troy-Ferrell]

Great bug item @Alexees . I have started running into this with my PR #6822 which could benefit from an MRTK-wide solution as well

[Alexees]

Great bug item @Alexees . I have started running into this with my PR #6822 which could benefit from an MRTK-wide solution as well

Finally someone notices 🤣

[Troy-Ferrell]

Added UnityObjectExtensions.IsNull()

[Alexees]

Added UnityObjectExtensions.IsNull()

This only works for objects, not for interface references. This will do:
public static bool IsNull<T>(T obj) where T : class => obj == null || obj.Equals(null);

[keveleigh]

Also, is the newly added UnityObjectExtensions.IsNull() needed as-is? UnityEngine.Object is the type with the custom equality operator, so I don't think we have to do anything special when we already have that type.

[Alexees]

Also, is the newly added UnityObjectExtensions.IsNull() needed as-is? UnityEngine.Object is the type with the custom equality operator, so I don't think we have to do anything special when we already have that type.

Right, it's not. we're dealing only with not-yet-known-types, like interfaces that could potentially be UnityEngine.Object

[Troy-Ferrell]

Two items here I see

1) Testing interfaces directly

So in my test with the pointer cache PR, @Alexees your isnull function doesn't work. Only after casting it to the monobehaviour type did it pass.

Debug.Log("Start");
IMixedRealityPointer p = pointerCache.Pop();
if (IsNull(p))
{
    Debug.Log("Pointer is null");
}

var pointerComponent = p as MonoBehaviour;
if (pointerComponent == null)
{
    Debug.Log("Pointer mono is null");
}
Debug.Log("End");

2) Simplify UnityObjectExtensions.IsNull()

Yes, this function is pointless now after converting to MonoBehaviour. Because then == on the monobehaviour will fire

[Alexees]

I did a test too and I'm wondering what the difference is, because it does work:
This is the test code:

    IEnumerator Start()
    {
        GameObject test = new GameObject("tester", typeof(Test));
        ICollection @interface = test.GetComponent<ICollection>();
        Destroy(test);
        yield return null;
        Debug.Log(@interface == null);
        Debug.Log(@interface.Equals(null));
        Debug.Log(IsNull(@interface));
    }

This is the MonoBehaviour used:
public class Test : MonoBehaviour, ICollection

And this is the last check performed:
public static bool IsNull<T>(T obj) where T : class => obj == null || obj.Equals(null);

the result is:

false
true
true

1. Since the interface reference holds the managed object in memory, it's not null.
2. the unmanaged object however is gone, therefore the second entry successfully results in true.
3. So does the third, which uses the method I provided

Now the question is, why doesn't this work in your case?

I can only think of the class not being a MonoBehaviour at all, like this one:
public abstract class GenericPointer : IMixedRealityPointer
which means it is indeed not null, and your test fails in that the cast to MonoBehaviour produces null, which is totally correct.