Udostępnij za pośrednictwem

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

  • Artykuł

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.

Comments

  • Anonymous
    October 24, 2007
    PingBack from http://www.soundpages.net/computers/?p=4530

  • Anonymous
    October 24, 2007
    Sorry this is a tangent, but it shows yet again yet again yet again how important it seems to be in Microsoft for programmers to learn how to shut up a compiler warning which would have been accurate, instead of making their code correct.  Why not teach programmers how to program correctly instead of doing ship-it like this: (void**)&m_pEnumerator Sure, in the compiler someone used yesterday, it doesn't crash.  But tomorrow's compiler might be better.  Henry Spencer wrote something like "If you lie to the compiler, it will get revenge."

  • Anonymous
    October 24, 2007
    Hmmm... That's too much work for a simple circular reference. Couldn't one just look at own ref count, and un-register when it drops to 1? Unregistering will put refcount to 0 and cause destruction of the instance. (Hmmm... This also means ::Release must be re-entrant, but it usually is, no?) Not really advisable for anything but simplest cases, though.

  • Anonymous
    October 24, 2007
    wouldn't it be easier to put: m_pEnumerator->UnregisterEndpointNotificationCallback(m_pDelegator); m_pDelegator->OnPFooFinalRelease(); in the IUnknown::release of the delegator like: ULONG lRet = InterlockedDecrement((LONG *)&m_cRef);        if (lRet == 0)        {             m_pEnumerator->UnregisterEndpointNotificationCallback(m_pDelegator);            m_pDelegator->OnPFooFinalRelease();            delete this;        }        return lRet;

  • Anonymous
    October 25, 2007
    BG: Of course - they're functionally equivilant.  ATL replaces your code with a call to the "FinalRelease" method. Goran: That is a ...  bad idea - it relies on the fact that you know that there will be one and only one reference applied by the notification logic.  That can be quite tricky.  In addition, it's very hard to get it totally right in a multithreaded environment. Norman: You're right - I had a friend once who hated any warning level greater than 2 because it forced him to add casts that could cause errors later on.

  • Anonymous
    October 25, 2007
    What's happened to pUnkOuter? That seemed like an elegant solution to this type of problem.

  • Anonymous
    October 25, 2007
    Norman: I understand that you do not like the cast to (void**). I am not really sure exactly why you think that this is so bad in this particular case. What is your alterantive ? If you say that (void**) is bad, you must have something better in mind ? Or, in other words, when you are accusing other people of doing "ship-it" programming instead of programming "correctly", then you should really have a good idea yourself of what the "correct" code should be. Please enlight us with your infinite wisdom. Do not forget to motivate why the code you propose is more "correct" than using (void**).

  • Anonymous
    October 25, 2007
    Norman: I understand that you do not like the cast to (void**). I am not really sure exactly why you think that this is so bad in this particular case. What is your alterantive ? If you say that (void**) is bad, you must have something better in mind ? Or, in other words, when you are accusing other people of doing "ship-it" programming instead of programming "correctly", then you should really have a good idea yourself of what the "correct" code should be. Please enlight us with your infinite wisdom. Do not forget to motivate why the code you propose is more "correct" than using (void**).

  • Anonymous
    October 25, 2007
    Norman: I understand that you do not like the cast to (void**). I am not really sure exactly why you think that this is so bad in this particular case. What is your alterantive ? If you say that (void**) is bad, you must have something better in mind ? Or, in other words, when you are accusing other people of doing "ship-it" programming instead of programming "correctly", then you should really have a good idea yourself of what the "correct" code should be. Please enlight us with your infinite wisdom. Do not forget to motivate why the code you propose is more "correct" than using (void**).

  • Anonymous
    October 28, 2007
    The comment has been removed

  • Anonymous
    October 29, 2007
    The comment has been removed

  • Anonymous
    October 29, 2007
    The comment has been removed

  • Anonymous
    October 31, 2007
    The comment has been removed

  • Anonymous
    November 01, 2007
    The comment has been removed

  • Anonymous
    November 01, 2007
    Norman, you are not listening. You originally said that using a cast to (void**) in a call to CoCreateInstance was incorrect. 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. I strongly disagree with your statement as I have explained in my comment above. 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! If you want to continue discussing this, please stay on topic - the discussion is about calling CoCreateInstance on Windows from a C++ program. So far, it seems that your argument is really that casting to (void**) can be wrong if you are writing pure C++ code that calls another C++ function compiled with the same compiler and you want the code to be portable to several esoteric environments like old mainframes, microcontrollers, and such. That might or might not be a valid argument, but that is simply not the situation I am talking about! As a final comment: I have read K&R ("The C programming language" from 1972), I have read the ANSI C++ standard, I have over 20 years of experience with programming a large variety of different systems, you have absolutely no idea of my skills.

  • Anonymous
    November 04, 2007
    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.

  • Anonymous
    November 04, 2007
    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.