Memory leak introduced in 1.5

[Darkyenus]

There seems to be a memory leak somewhere in file retrieval.

using System;
using System.Linq;
using MediaDevices;

namespace MediaDevicesLeakRepro {
    class Program {
        static void Main(string[] args) {
            foreach (var device in MediaDevice.GetDevices()) {
                device.Connect();
                while (true) {
                    device.EnumerateFileSystemEntries("/").First();
                    GC.Collect(100, GCCollectionMode.Forced, true, false);
                    GC.WaitForPendingFinalizers();
                }
            }
        }
    }
}

When device is attached and has a drive, this code will leak unmanaged memory, in all version from 1.7 to 1.5. In 1.4 this no longer happens.

GC is there just to confirm that there really is a leak and it isn't just a lazy GC.

I suspect that this causes serious problems during certain file operations, especially when the device itself adds/removes/changes files at the same time, but this is currently only a hunch. This leak caused my application to crash with out of memory error, after leaving it running for a while.

It could be somehow related to #13, but that is just a speculation.

[Darkyenus]

I have traced the regression to d55d00a3b14962d303d6bca15c8b713b3bc094b6.

EDIT1: The leak starts happening after including following files of this commit (to the working directory from previous commit):

• MediaDevices/Internal/ComHelper.cs
  • manually editing VarType back to PropVariant, commenting out unused GetVarType and TryByteArrayValue
• MediaDevices/Internal/ObjectProperties.cs

EDIT2: The leak seems to be happening inside ComHelper.HasKeyValue, which, beginning with this commit, is being used before querying anything from IPortableDeviceProperties.

[Darkyenus]

I have found and fixed the problem. It seems that PROPVARIANT from IPortableDeviceValues.GetAt is not being freed correctly, as it needs an explicit call to PropVariantClear, which the marshalling layer does not do, for some reason. Doing this explicitly fixes the problem.

e.g.

public static bool HasKeyValue(IPortableDeviceValues values, PropertyKey findKey)
{
    uint num = 0;
    values.GetCount(ref num);
    for (uint i = 0; i < num; i++) {
        PropertyKey key = new PropertyKey();
        PROPVARIANT val = new PROPVARIANT();
        try {
            values.GetAt(i, ref key, ref val);
            if (key.fmtid == findKey.fmtid && key.pid == findKey.pid) {
                PropVariant pval = val;
                return pval.variantType != PropVariant.VT_ERROR;
            }
        } finally {
            PropVariantClear(ref val);
        }
    }
    return false;
}

// http://www.pinvoke.net/default.aspx/iprop/PropVariantClear.html
// https://social.msdn.microsoft.com/Forums/windowsserver/en-US/ec242718-8738-4468-ae9d-9734113d2dea/quotipropdllquot-seems-to-be-missing-in-windows-server-2008-and-x64-systems?forum=winserver2008appcompatabilityandcertification
[DllImport("ole32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
static extern int PropVariantClear(ref PROPVARIANT pVar);

[Darkyenus]

On my cleanup branch I have made some progress regarding PropVariant leaks - I am no longer aware of any places where they occur. (Though it has probably cost me some sanity.) In addition, I have cleaned up how PortableDeviceApi COM wrapper dependencies are handled, which can serve as a start for real 64bit support. But for now I am focused on bugfixing the current 32bit implementation, as there are still some problems (I think) I have, but had not yet time to reproduce.

[Bassman2]

Hi Darkyenus, thanks for the fix. The fix is merged now.

[Darkyenus]

There are actually more places where this leak occurs, sorry for not being clear about that. Why not merge the whole branch? I put a lot of time in it. Are you worried about licensing or something? Because I don't really care about that, I will put my contributions in public domain if you want. (Technically now they are still in MIT, I guess.)

[Bassman2]

Hi Darkyenus, sorry I missed your branch. I looked at it and am currently torn. On the one hand, I think it's good that you 'PROPVARIANT' no longer marshals. That should save time. On the other hand, I do not like your 'Dispose' function because the name is occupied by 'IDisposable' and the use of 'using' is associated. It would be nice to combine both. So that we can use 'using'.

[Darkyenus]

I agree that it would be nice if we could use using for this, and having a wrapper struct would also simplify ToString, which I had to rename to ToDebugString. However maintaining a marshalled struct is risky and cumbersome. I personally don't see that much of a problem with having Dispose as an extension function - sure, it is not "idiomatic C#", but since the type is used only internally, it is not that hard to remember to always call Dispose manually.

I have three ideas about this:

1. (simplest) Just rename Dispose to Free, so that it is not consufed with IDisposable.Free and leave it as it is. No matter what the solution will be, users of PROPVARIANT will always have to remember something, so why not keep it the simplest it can be.
2. Manually modify COM wrappers so that PROPVARIANT implements IDisposable. It shouldn't be that hard to do and then we can use using for that extra exception safety. Users of PROPVARIANT still need to remember to wrap it in using, but it is idiomatic C# at least.
3. Always wrap PROPVARIANT into a class, which implements IDisposable and contains code so that Dispose is always called, even when user forgets to call it (like what file streams do, and probably other parts of C# as well). I don't like this solution, because it is clunky, adds overhead, and if the rest of the library is correct (as it currently is), brings no additonal benefits.

In my opinion, 3 is crazy overkill. 2 is somewhat better, but will not be easy to implement and maintain. I am not sure if implementing an interface does not change some of the layout characteristics of the struct, so it may not even work. But it would look elegant if it did.

In the end, I prefer 1, as it is vastly simpler than 2 and the result is pretty much the same. (Especially when leaking in some edge cases (if some new code would forget to dispose) is not such a big deal, considering nobody has noticed up until now.)

[Bassman2]

Hi Darkyenus, I think I found a good architecture using the design pattern facade. Take a look at the facade branch. Is only a hull so far but should go like this.

[Darkyenus]

Yeah, that looks like it could work.

[Bassman2]

Implemented in Facade branch