Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

  • Comments 17

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.

  • Friday, November 02, 2007 4:57 AM by stegus

    > Norman, you are not listening.

    I was but I'll quit soon.

    > You originally said that using a cast to (void**) in a call to CoCreateInstance was incorrect.

    Because it is.  I gave an example of why.

    > You even accused people writing code that way of doing ship-it programming and you use this as an example of how incompetent Microsoft programmers are.

    Because we see it in blogs of people who should know better, AND in textbooks published by Microsoft Press after being authored by people who should know better (including one on device drivers).

    > Your comments about 36-bit pointers, varying pointer sizes depending on datatype, old IBM mainframes and such have absolutely nothing to do with what I am talking about.

    > I am talking about calling CoCreateInstance on Windows - nothing else!

    You're talking about abusing casts in calls to CoCreateInstance on 32-bit Intel-based Windows using a limited set of compilers where the abusers have gotten away with it for a while.  Abusers on 64-bit Intel-based Windows should have seen their stuff crash.  As for my comments about 36-bit pointers, try again, look at what you said:

    > As a final comment: I have read K&R ("The C programming language" from 1972),

    Then you should know that C can run on machines where (void*) and (struct x*) don't have the same bit-representation but only happen to have the same number of bits.  And you should be aware of machines where (void*) and (struct x*) would have different numbers of bits.

  • Sigh, there you go again!

    You keep repeating that there might be a problem on esoteric machines where pointer sizes or pointer representations might differ depending on pointed-to datatype. THIS IS TOTALLY IRRELEVANT for this particular situation!

    Please give a concrete  example of a machine/compiler combination where the cast to (void**) in the call to CoCreateInstance would cause a problem.

    Note the following important constraints:

    • The system is running Windows with full support for COM (this excludes 16-bit windows among other things)

    • The compiler is a C++ compiler

    • CoCreateInstance is an API function only available in binary form

    • The pointer whose address is passed in to CoCreateInstance is a COM interface pointer

    • It should be possible to call COM functions through the returned pointer using standard C++ syntax, like pEnum->DoInterestingStuff()

    I dont believe that such a machine/compiler combination exists, but if there is such a beast it would be interesting to know about it.

Page 2 of 2 (17 items) 12