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

Article
09/08/2008

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

Anonymous
September 08, 2008
PingBack from http://informationsfunnywallpaper.cn/?p=4363
Anonymous
September 08, 2008
The comment has been removed
Anonymous
September 08, 2008
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.
Anonymous
September 08, 2008
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, ...)
Anonymous
September 08, 2008
The comment has been removed
Anonymous
September 08, 2008
The comment has been removed
Anonymous
September 08, 2008
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.
Anonymous
September 08, 2008
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*)
Anonymous
September 08, 2008
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).
Anonymous
September 09, 2008
Small typo. For your 2nd solution: HRESULT GetDeviceEnumerator(IMMDeviceEnumerator *Enumerator) ...should be... HRESULT GetDeviceEnumerator(IMMDeviceEnumerator **Enumerator)
Anonymous
September 09, 2008
The comment has been removed
Anonymous
September 09, 2008
Blake: The function is the smallest function I could come up with that expresed the bug.
Anonymous
September 09, 2008
The comment has been removed
Anonymous
September 09, 2008
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); }
Anonymous
September 10, 2008
The comment has been removed
Anonymous
September 10, 2008
@Maurits, but then it would rather fail in it's purpose. i.e. to be a bit of example code demonstrating the problem.
Anonymous
September 10, 2008
@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.
Anonymous
September 11, 2008
@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)
Anonymous
September 11, 2008
@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 :)
Anonymous
September 11, 2008
@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.
Anonymous
September 15, 2008
"Kudos to James Skimming" Don't you spell that QDOS?

Partager via

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

Comments

Ressources supplémentaires