Firestore: Potential memory leak in snapshot listener callbacks.

[dconeybe]

There is a memory leak in Firestore that should be fixed regarding snapshot callbacks.

Here are the two problematic code snippets:

DocumentReference.DocumentSnapshotsHandler()

firebase-unity-sdk/firestore/src/DocumentReference.cs

Lines 254 to 261 in f948c75

	[MonoPInvokeCallback(typeof(ListenerDelegate))]
	private static void DocumentSnapshotsHandler(int callbackId, IntPtr snapshotPtr,
	FirestoreError errorCode, string errorMessage) {
	Action<DocumentSnapshotProxy, FirestoreError, string> callback;
	if (snapshotListenerCallbacks.TryGetCallback(callbackId, out callback)) {
	callback(new DocumentSnapshotProxy(snapshotPtr, true), errorCode, errorMessage);
	}
	}

Query.QuerySnapshotsHandler()

firebase-unity-sdk/firestore/src/Query.cs

Lines 741 to 748 in f948c75

	[MonoPInvokeCallback(typeof(ListenerDelegate))]
	private static void QuerySnapshotsHandler(int callbackId, IntPtr snapshotPtr,
	FirestoreError errorCode, string errorMessage) {
	Action<QuerySnapshotProxy, FirestoreError, string> callback;
	if (snapshotListenerCallbacks.TryGetCallback(callbackId, out callback)) {
	callback(new QuerySnapshotProxy(snapshotPtr, true), errorCode, errorMessage);
	}
	}

The IntPtr passed to DocumentSnapshotsHandler()/QuerySnapshotsHandler() is an owning pointer to the underlying C++ object, meaning that C# is responsible for deleting the underlying C++ object. This deletion is looked after by the DocumentSnapshotProxy/QuerySnapshotProxy objects because their cMemoryOwn constructor argument is being specified a value of true. However, these proxy object are created in an "if" block. Therefore, if the "if" block is not entered then ownership of the C++ object will not be claimed and it will never be deleted, creating a memory leak.

Thankfully, this would be a very rare situation that the "if" block would not be entered; however, it should still be fixed for robustness. The fix is to simply create the DocumentSnapshotProxy/QuerySnapshotProxy objects unconditionally, before the "if" block, so that ownership of the C++ object will be claimed and deleted regardless of whether or not the "if" block is executed.

[dconeybe]

Fixed by cl/391282781, which is, unfortunately, only visible to Googlers. It will eventually be ported to this repository though.