Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What’s wrong with this code, Part 23 – The Answers

What’s wrong with this code, Part 23 – The Answers

Rate This
  • Comments 21

My last post was all about a problem with what appeared to be some really simple ATL code.

It turns out that the problem was easier than I had expected.  James Skimming came up with the answer on the second comment.  The problem here was that the following code (snipped a bit)

IMMDeviceEnumerator *GetDeviceEnumerator()
{
    CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));
    if (FAILED(hr))
    {
        return NULL;
    }
    return deviceEnumerator.Detach();

}
int _tmain(int argc, _TCHAR* argv[])
{
    CoInitialize(NULL);
    {
        CComPtr<IMMDeviceEnumerator> deviceEnumerator(GetDeviceEnumerator());
        CComPtr<IMMDeviceCollection> deviceCollection;

        if (deviceEnumerator != NULL)
        {
            HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection);
            if (SUCCEEDED(hr))
            {

That’s because the CComPtr<T> constructor takes a reference to the input T* object.  As a result, the code leaks a reference to the MMDeviceEnumerator object.  The correct fix for the problem generated a fair amount of discussion in the comments and on email internally.

The root cause of the problem  is that the code in question mixes raw interface pointers and smart pointers.  In this case there’s an impedance mismatch between the GetDeviceEnumerator (which returns a raw pointer) and it’s caller (which is expecting a smart pointer).

My preferred solution to the problem is:

CComPtr<IMMDeviceEnumerator> GetDeviceEnumerator()
{
    CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));
    if (FAILED(hr))
    {
        return NULL;
    }
    return deviceEnumerator;

}

The reason I prefer this solution is that it consistently uses the smart pointer class.  There are two downsides to it.  The first is that it loses the result of the call to CoCreateInstance.  The other downside is that it constructs a temporary object on the stack and has two additional calls to AddRef()/Release().

 

There are other possible solutions.  The second possible solution changes the GetDeviceEnumerator function:

HRESULT GetDeviceEnumerator(IMMDeviceEnumerator *Enumerator)
{
    CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    if (Enumerator == NULL)
    {
        return E_POINTER;
    }
    HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));
    if (FAILED(hr))
    {
        *Enumerator = NULL;
        return hr;
    }
    *Enumerator = deviceEnumerator.Detach();

    return S_OK;
}

With this change, the caller looks like:

    {
        CComPtr<IMMDeviceEnumerator> deviceEnumerator;
        if (SUCCEEDED(GetDeviceEnumerator(&deviceEnumerator))
        {
            CComPtr<IMMDeviceCollection> deviceCollection;
            HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection);
            if (SUCCEEDED(hr))

This solution fits closely with the COM usage pattern so it is quite attractive.  You also don’t lose the result of the CoCreateInstance API call, which can be extremely useful for diagnostics.  The major negative with this is that it depends on the fact that the CComPtr<T> operator& returns a raw pointer to the underlying object.

 

The third possible solution keeps the “return a value” idea of the original code but works around the additional reference applied in the constructor.  Keep the original implementation of GetDeviceEnumerator and change the tmain function as follows:

int _tmain(int argc, _TCHAR* argv[])
{
    CoInitialize(NULL);
    {
        CComPtr<IMMDeviceEnumerator> deviceEnumerator;
        deviceEnumerator.Attach(GetDeviceEnumerator());
        CComPtr<IMMDeviceCollection> deviceCollection;

The big problem with this solution is that to me it’s “unnatural” – the whole point of using smart pointers is that their use is supposed to be intuitive, however in this case there’s nothing intuitive about the use – it certainly fixes the problem but it relies on internal behaviors of the smart pointer class.  To me this is the least attractive solution to the problem.

 

As I mentioned, Kudos to James Skimming for figuring the problem out quickly.

  • PingBack from http://informationsfunnywallpaper.cn/?p=4363

  • The solution that returns a CComPtr<IMMDeviceEnumerator> by value doesn't necessarily have to construct a temporary object, or have multiple unnecessary calls to AddRef/Release.  In C++, the compiler is allowed to (but not required to) elide temporary objects that would normally be created when returning an object by value, even if the constructor and/or destructor have side effects.  In Visual C++ 2008, that optimization only kicks in with optimizations enabled (/Ox works), and if the function returns the same local variable along all control paths.  Try changing the line "return NULL;" to "deviceEnumerator = NULL;", and look at the generated code with optimizations enabled.  You shouldn't see any calls to AddRef or Release along the success path in GetDeviceEnumerator(), nor should you see a temporary object getting created at the call site in _tmain().

  • Having the return value be a CComPtr<> will likely add the extra overhead of AddRef/Release.  But using this as a reason for not returning a CComPtr<> seems like premature optimization.  It seems unlikely this will be the source of a siginificant performance issue.  

    The upside is, if this does turn out to be a problem, it's easily fixed by changing the function signature to a version that doesn't require AddRef/Release.  

  • CComPtr<>::operator& is specifically designed to be passed as a parameter to functions.  Otherwise how could you do this:

    CComPtr<IFoo> spFoo;

    HRESULT hr = CoCreateInstance(..., &spFoo, ...)

  • "The major negative with this is that it depends on the fact that the CComPtr<T> operator& returns a raw pointer to the underlying object."

    Do you mean it's bad because it's unintuitive and requires you to know about the internals of the smart pointer class? Presumably what it depends on is true and will remain true (else a lot of things will break) so the dependency itself isn't bad, or have I misunderstood?

    Whenever I use smart pointers with COM I find myself looking at the smart pointer source to understand exactly when it will and won't AddRef/Release and/or take/detach the pointer. (I guess I don't use them enough to remember and I get confused because each smart pointer implementation has slightly different semantics and I can't remember which is which.) That seems to be the price smart pointers charge in return for not having to worry (as much) about early returns and exceptions.

    Another thing I find helpful is when function/API comments/docs are explicit about whether the pointers they return need Releasing. They almost always do but there are some exceptions and, either way, it's good to have that reminder whenever anyone looks up the docs so they don't forget to think about AddRef/Release.

  • The best solution is to use a decently designed smart pointer class rather than CComPtr crap.

    The constructor ref_counted_smart_ptr(T *) is evil because there is no 'default' conversion from raw pointer to such a smart pointer. This topic has been discussed on Usenet in the past.

    Then if your smart pointer has a ctor that creates the pointee it should report errors in the canonical way: by throwing an exception. This would solve your HRESULT problem.

    Of course I can already hear that you cannot use exceptions b/c of [list of bogus reasons].

  • As I said on the other thread, I'd always stick with the second implementation, because it will look most natural in place. If you're writing ATL COM code you won't be able to write much without using the HRESULT Func(ISomething** ppSomething) pattern. Indeed, you use it in your first solution already with the call to EnumAudioEndpoints. You've now got two patterns of usage for CComPtr, which introduces the possibility of confusion later on.

  • This is why Mozilla's smart COM pointer class has a helper macro (getter_AddRefs) to pass a smart pointer by reference to routines that return addref'ed objects:

    http://developer.mozilla.org/en/Using_nsCOMPtr/Reference_Manual#nsCOMPtr.3cT.3e_.3d_dont_AddRef(_T*_).2cnsCOMPtr.3cT.3e_.3d_getter_AddRefs(_T*_)

  • The "correct" idiom seems to be:

    * If you're given a smart pointer, your work is done in all cases.

    * If you're given a raw pointer as an argument, you assume (by default) you do not own a reference to the object; you're borrowing one from your caller.

    * If you're given a raw pointer as a return value / out parameter, you assume (by default) you own one reference to the object.

    Reading this from the other side, after passing the raw pointer to the CComPtr constructor, you still own one reference to the returned pointer, so the "idiomatic" process would be as follows:

           IMMDeviceEnumerator *result = GetDeviceEnumerator();

           CComPtr<IMMDeviceEnumerator> deviceEnumerator(result);

           result->Release();

    The "Attach()" method is weird, because it violates my second guideline, so I would prefer solutions which don't use it. Incidentally, the "idiomatic" process uses as many AddRef()s and Release()s as returning the CComPtr would (or more, as Dave Bartolomeo points out).

    I personally think the first method, second method (with the argument type corrected to be IMMDeviceEnumerator**), or second method but with an argument of "CComPtr<...> &" are all fine (with the choice depending on whether you want the HRESULT, and whether you want to interoperate with code which doesn't use CComPtr<>s).

  • Small typo.  For your 2nd solution:

    HRESULT GetDeviceEnumerator(IMMDeviceEnumerator *Enumerator)

    ...should be...

    HRESULT GetDeviceEnumerator(IMMDeviceEnumerator **Enumerator)

  • Hey Larry,

    Is there something important you're trying to abstract by having the GetDeviceEnumerator function there?  Because right now it doesn't seem to do anything worth having a separate function for.  The code in _tmain then just looks like:

       CoInitialize(NULL);

       {

           CComPtr<IMMDeviceEnumerator> deviceEnumerator;

           CComPtr<IMMDeviceCollection> deviceCollection;

           hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));

           if (SUCCCEEDED(hr))

           {

               HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection);

    If you still prefer the separate function, couldn't you just declare it like this (as Richard suggested above)?

    HRESULT GetDeviceEnumerator(CComPtr<IMMDeviceEnumerator> &deviceEnumerator)

    {

           return (deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator)));

    }

  • Blake: The function is the smallest function I could come up with that expresed the bug.  

  • @Marvin,

    "The best solution is to use a decently designed smart pointer class rather than CComPtr crap."

    It's easy to criticize but it's a lot more effective if you have specific problems to point out.  

  • Here's my preferred solution:

    HRESULT GetDeviceEnumerator(IMMDeviceEnumerator** pp)

    {

       CComPtr<IMMDeviceEnumerator> deviceEnumerator;

       HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));

       if (FAILED(hr)) return hr;

       return deviceEnumerator.CopyTo(pp);

    }

  • After some thought, I'm not convinced the function adds value at all.  You're effectively wrapping a one-line call in a multi-line function, and then you still have to /call/ the function.

    Here's my proposed way of doing this:

    int _tmain(int argc, _TCHAR* argv[])

    {

       CoInitialize(NULL);

       {

           CComPtr<IMMDeviceEnumerator> deviceEnumerator;

           CComPtr<IMMDeviceCollection> deviceCollection;

           if (SUCCEEDED(deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator))))

           {

               HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection);

               if (SUCCEEDED(hr))

               {

    Any problem can be solved by adding a layer of abstraction; except for "too many layers of abstraction".

Page 1 of 2 (21 items) 12