Del via

Bad COM practices: Returning an AddRef'd pointer

  • Artikel

Recently I had to fix a memory leak in some COM code that was pretty poorly written. The problematic code looked something like this: 

 IMyInterface* GetInterface()
{
    IMyInterface* pRef = NULL;
    pRef = ::CoCreateInstance(...);
    
    // At this point, pRet has a reference count of 1.
    return( pRef );
}

 void UseInterface()
{
    CComPtr<IMyInterface> sp;
    sp = GetInterface();

    // Do work with sp

    // Is the object freed?
    sp = NULL;
}

Why is this "bad" code? Although Don Box allows this with rule A3 in Essential COM, in my experience, I've found code like this to be easy to confuse over ownership of the interface.

For example, without a well-defined pattern, do you know whether it is safe to "chain" calls like is common in C++?

 HRESULT hr = GetInterface()->Foo();

What about the following use of the smart pointer?

 CComPtr<IMyInterface> sp = GetInterface();

It turns out that in both cases, you'll get an interface leak with the implementation of GetInterface() above. The worst is that when people on the team are using smart pointers and are unaware about how the resource (the interface) is allocated; it can lead to hard to debug leaks. "But I used smart pointers! There's no way it will leak!" I've heard that said too many times.

So, what to do?

Different people have different "patterns" that work for them; I've found that the one that works for me (and my old team) is the following rules:

  1. Interfaces that are created or returned and AddRef was called, are only passed through out parameters.
  2. Interfaces returned as return values for functions are not AddRef'd.

This works well for me because it is consistent with COM interface definitions (since all COM methods can only return HRESULT, the only way to "return" an interface is through the out parameter). Furthermore, the "return an interface" can be thought of as "taking advantage of a priori knowledge of interface lifetime" so that you can chain calls and now have to store each call in a local variable.

What do you think? What practice works for you?

I suppose that at the end of the day, any pattern and practice will work well as long as the abstraction doesn't leak and that everyone on the team is on board with the pattern.

Comments

  • Anonymous
    November 18, 2006
    I like consistent naming rules. if something contains "create", "copy", or "new" i the function name, it needs to be released. If it contains anything else (such as Get), it does not. In this case, GetInterface() should probably be renamed to CreateInterface().

  • Anonymous
    November 19, 2006
    The comment has been removed