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


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.

Comments (21)

  1. Anonymous says:

    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().

  2. 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.  

  3. Anonymous says:

    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, …)

  4. Anonymous says:

    "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.

  5. Anonymous says:

    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].

  6. Wound says:

    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.

  7. KJK::Hyperion says:

    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*_)

  8. Anonymous says:

    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).

  9. Anonymous says:

    Small typo.  For your 2nd solution:

    HRESULT GetDeviceEnumerator(IMMDeviceEnumerator *Enumerator)

    …should be…

    HRESULT GetDeviceEnumerator(IMMDeviceEnumerator **Enumerator)

  10. Anonymous says:

    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)));

    }

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

  12. @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.  

  13. Anonymous says:

    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);

    }

  14. 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".

  15. Wound says:

    @Maurits, but then it would rather fail in it’s purpose. i.e. to be a bit of example code demonstrating the problem.

  16. Anonymous says:

    @Jared

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

    I did just that in the rest of my comment.

    Digging through my links, here is another longish discussion of problems with CComPtr and other similar classes:

    http://www.gershnik.com/articles/refcnt_ptr.asp

    It also gives a better smart pointer class that would have prevented Larry’s problem by forcing him to stop and think what exactly he wants to accomplish when assigning from raw pointer.

  17. @Marvin,

    I don’t feel like you did.  You made one comment about ref_counted_smart_ptr.  I’m not sure if you were refering to CComPtr<> or not but comment lacked a good example.  Mind you I’m not saying CComPtr<> is perfect (I’ve made several posts to the contrary).  

    I enjoyed the article wanting to add a parameter to the constructor of the CComPtr class indicating whether or not an add ref was requested.  I highly dislike the use of a bool though.  Take the following

    CComPtr<IFoo> spFoo(p,true)

    To an un-educated user what does true mean?  If you say they’ll look at the documentation that’s a bad answer.  Because if looking at the documentation was an answer then there should be no problem having a single argument constructor that will AddRef (after all if everyone read the documentation, there would be no ambiguity)

    On the other hand if you made in an enum it becomes much clearer.

    enum SmartPointerInit { SmartPointerInit_AddRef; SmartPointerInit_DontAddRef}

    CComPtr<IFoo> spFoo(p,SmartPointerInit_AddRef)

  18. @Myself

    Accidentally navigated to submit trying to dismiss an annoying dialog.

    The author of the article didn’t believe that a non-loud name could be found for an enum.  The problem he’s trying to solve is a subtle issue.  Making the issue loud removes the subtle problem 🙂

  19. Anonymous says:

    @Jared

    I don’t think you’ve read the article through since it makes exact same argument about enum vs. bool.

    But at the end it doesn’t want to use a second parameter at all, be it bool or enum. The syntax it forces is something like

    p_smart = ref(p_raw);

    p_smart = noref(p_raw);

    which to me looks much better than two parameter constructor. Especially b/c you don’t need to repeat the pointer type.

  20. Anonymous says:

    "Kudos to James Skimming"

    Don’t you spell that QDOS?