"Memory Leak" when using the Vista Audio API notification routines

We recently got an internal report from someone using the internal audio notification APIs that they were leaking memory and they wanted to help from us debugging the problem.

I took a look and discovered that the problem was a circular reference that was created when they called:

CFoo::Initialize(){    <SNIP>     hr = CoCreateInstance(__uuidof(MMDeviceEnumerator), NULL, CLSCTX_INPROC_SERVER, __uuidof(IMMDeviceEnumerator), (void**)&m_pEnumerator);    if (FAILED(hr))        return hr;     if (FAILED(m_pEnumerator->RegisterEndpointNotificationCallback(this)));    <SNIP>}

CFoo::~CFoo()
{
    <SNIP>
    m_pEnumerator->UnregisterEndpointNotificationCallback(this);
    <SNIP>
}

The root cause of the problem is that the IMMDeviceEnumerator::RegisterEndpointNotificationCallback takes a reference to the IMMNotificationClient object passed in.  This shouldn't be a surprise, because the Counted Pointer design pattern requires that every time you save a pointer to an object, you take a reference to that object (and every interface that derives from IUnknown implements the Counted Pointer design pattern).  Since the RegisterEndpointNotificationCallback saves it's input pointer for later consumption (when it generates the notification), it has to take a reference to the object.

At the heart of the problem is the fact that CFoo object only calls UnregisterEndpointNotificationCallback in its destructor (which will never be called).  If the CFoo object had a "Shutdown()" or other form of finalizer, the call to UnregisterEndpointNotificationCallback could be moved to the finalizer, thus removing the circular reference and avoiding the memory leak.  This is by far the best solution - I'm a huge fan of deterministic finalism.

 

Unfortunately, sometimes it's not possible to have a "Shutdown()" method (for instance, if you're implementing an interface that doesn't implement the finalizer design pattern (in fact, this was the case for the person who reported the problem to us).

In that case, you really want to depend on the fact that the reference count reflects your external references, not your internal references.  Effectively, you want to maintain two separate reference counts, one for external clients, the other for internal usage.

One way to achieve this is to use a delegator object - instead of handing "this" to the RegisterEndpointNotificationCallback, you pass a small object that implements IMMNotificationClient.  So:

class CFooDelegator: public IMMNotificationClient{    ULONG   m_cRef;    CFoo *m_pFoo;     ~CFoo()    {    }public:    CFooDelegator(CFoo *pFoo) :        m_cRef(1),        m_pFoo(pFoo)    {    }    STDMETHODIMP OnDeviceStateChanged(LPCWSTR pwstrDeviceId, DWORD dwNewState)     {        if (m_pFoo)        {            m_pFoo->OnDeviceStateChanged(pwstrDeviceId, dwNewState);        }        return S_OK;    }     STDMETHODIMP OnDeviceAdded(LPCWSTR pwstrDeviceId)     {        if (m_pFoo)        {            m_pFoo->OnDeviceAdded(pwstrDeviceId);        }        return S_OK;    }     STDMETHODIMP OnDeviceRemoved(LPCWSTR pwstrDeviceId)    {        if (m_pFoo)        {            m_pFoo->OnDeviceRemoved(pwstrDeviceId);        }        return S_OK;    }     STDMETHODIMP OnDefaultDeviceChanged(EDataFlow flow, ERole role, LPCWSTR pwstrDefaultDeviceId)    {        if (m_pFoo)        {            m_pFoo->OnDeviceAdded(flow, role, pwstrDefaultDeviceId);        }        return S_OK;    }     STDMETHODIMP OnPropertyValueChanged(LPCWSTR pwstrDeviceId, const PROPERTYKEY key)    {        if (m_pFoo)        {            m_pFoo->OnPropertyValueChanged(pwstrDeviceId, key);        }        return S_OK;    }     void OnPFooFinalRelease()    {        m_pFoo = NULL;    }     STDMETHOD(QueryInterface) (REFIID riid,                               LPVOID FAR* ppvObj)    {        *ppvObj = NULL;        if (riid == IID_IUnknown)        {            *ppvObj = static_cast<IUnknown *>(this);        }        else if (riid == IID_IMMNotificationClient)        {            *ppvObj = static_cast<IMMNotificationClient *>(this);        }        else        {            return E_NOINTERFACE;        }        return S_OK;    }    STDMETHOD_(ULONG,AddRef)()    {        return InterlockedIncrement((LONG *)&m_cRef);    }    STDMETHOD_(ULONG,Release) ()    {        ULONG lRet = InterlockedDecrement((LONG *)&m_cRef);        if (lRet == 0)        {            delete this;        }        return lRet;    }};

You then have to change the CFoo::Initialize to construct a CFooDelegator object before calling RegisterEndpointNotification().

You also need to change the destructor on the CFoo:

CFoo::~CFoo()
{
    <SNIP>
    m_pEnumerator->UnregisterEndpointNotificationCallback(m_pDelegator);
    m_pDelegator->OnPFooFinalRelease();
    m_pDelegator->Release();
    <SNIP>
}

It's important to call UnregisterEndpointNotificationCallback before you call OnPFooFinalRelease - if you don't, there's a possibility that the client's final release of the CFoo might occur while a notification function is being called - if that happens, the destructor might complete and you end up calling back into a partially destructed object.  And that's bad :).  The good news is that the UnregisterEndpointNotificationCallback function guarantees that all notification routines have completed before it returns.

It's important to realize that this issue occurs with ALL the audio notification callback mechanisms:IAudioEndpointVolume::RegisterControlChangeNotify, IPart::RegisterControlChangeCallback, and IAudioSessionControl::RegisterAudioSessionNotification.