Do you know when your destructors run? Part 1.


Larry Osterman discussed the importance of knowing when your global destructors run, but this problem is not exclusive to global objects. You need to take care even with local objects. Consider:

void Sample()
{
  if (SUCCEEDED(CoInitialize(NULL))) {
    CComPtr<IXMLDOMDocument> p;
    if (SUCCEEDED(p.CoCreateInstance(CLSID_IXMLDOMDocument))) {
     ...
    }
    CoUninitialize();
  }
}

Easy as pie. And there's a bug here.

When does the destructor for that smart-pointer run?

Answer: When the object goes out of scope, which is at the closing brace of the outer if statement, after the CoUninitialize call.

So you shut down COM, and then try to access a pointer to a COM object. This is not good. (Or as Larry describes it, "Blam!")

To fix this problem, you have to release all your COM pointers before the CoUninitialize. One way would be to insert a p.Release() at the end of the inner if. (But of course, if you're going to do that, then why bother using a smart pointer?)

Another fix would be to introduce a seemingly unnecessary scope:

void Sample()
{
  if (SUCCEEDED(CoInitialize(NULL))) {
    {
      CComPtr<IXMLDOMDocument> p;
      if (SUCCEEDED(p.CoCreateInstance(CLSID_IXMLDOMDocument))) {
       ...
      }
    } // ensure p is destructed before the CoUninit
    CoUninitialize();
  }
}

Make sure you leave that comment there or the next person to come across this code is going to "clean it up" by removing the "redundant" braces.

Of course, this is still too subtle. Here's another solution: Put the CoUninitialize inside a destructor of its own!

class CCoInitialize {
public:
 CCoInitialize() : m_hr(CoInitialize(NULL)) { }
 ~CCoInitialize() { if (SUCCEEDED(m_hr)) CoUninitialize(); }
 operator HRESULT() const { return m_hr; }
 HRESULT m_hr;
};

void Sample()
{
  CCoInitialize init;
  if (SUCCEEDED(init)) {
    CComPtr<IXMLDOMDocument> p;
    if (SUCCEEDED(p.CoCreateInstance(CLSID_IXMLDOMDocument))) {
     ...
    }
  }
} // CoUninitialize happens here

This works even if you put the smart pointer at the same scope, as long as you put it after the CCoInitialize object:

void Sample()
{
  CCoInitialize init;
  CComPtr<IXMLDOMDocument> p;
  if (SUCCEEDED(init) &&
      SUCCEEDED(p.CoCreateInstance(CLSID_IXMLDOMDocument))) {
   ...
  }
}

This works because objects with automatic storage duration are destructed in reverse order of declaration, so the object p wil be destructed first, then the object init.

Mind you, this is basically subtle no matter now you slice it. Nobody said programming was easy.

Tomorrow, part 2.

Comments (11)
  1. Stephane Rodriguez says:

    If the code is made full of implicit construction/destruction that will, because it’s unmanaged code, cause spectacular GPFs for any reason, then I am not sure there is any improvement here. I wonder if this isn’t a pledge for managed code.

    Also wonder of next part will cover smart pointers used in class members. Plenty of funny things here.

    Does anyone if a third party ever came with a COM component manager that would insulate developers from these things without being frightened of GPFs. I know that you can switch to managed code today, yet given that the ROT is of no help without explicit monikers – something that not many people seem to use in the real world – is there any tool to diagnose internal COM life cycle issues?

  2. Jack Mathews says:

    While managed code would stop GPF’s from happening, there are still PLENTY of logic problems that can and will happen from destruction order issues. This just happens to be one that crashes, but in managed code or not, you can make objects go to an invalid state and then try to use them to make bad things happen.

  3. Slight error (I think). Shouldn’t it be p.Release() and p.CoCreateInstance(…). The former is a "well known" gotcha with ATL smart pointers.

  4. Raymond Chen says:

    Oops, thanks Paul. I guess this reveals that I don’t use ATL smart pointers much…

  5. Mike Dunn says:

    p->Release() actually won’t compile. ATL guards against that incorrect usage by having operator-> return a _NoAddRefReleaseOnCComPtr*. The trick is that that class has the AddRef() and Release() methods private, making them inaccessible by the code trying to do p->Release().

  6. Catatonic says:

    After I read Larry Osterman’s post a few weeks ago, I created a class that’s almost identical to your CCoInitialize example. To me it feels like the natural solution.

  7. BeenThere says:

    I know this is just an example, but if CoInit was occasionally failing in my code I would want to know why. Does some unrelated code sometimes set the thread up with a different concurrency model? An assert might be nice. Also, shouldn’t you be using CoInitializeEx?

  8. Raymond Chen says:

    CoInitialize(NULL) is the same as CoInitializeEx(NULL, COINIT_APARTMENTTHREADED).

  9. BeenThere says:

    MSDN says "Applications developed today should call CoInitializeEx rather than CoInitialize," so that is what I always use.

  10. Sometimes they don’t run at all.

  11. Pingback from  How do I perform shell file operations while avoiding shell copy hooks? | The Old New Thing

Comments are closed.