Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, Part 18

What's wrong with this code, Part 18

  • Comments 14

This may be the shortest "Bad Code" I've ever done, but it keeps on surprising me how many times I see this problem (people asked me questions about it twice in the past week).

 

// BadCode18.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <windows.h>
#include <tchar.h>
#include <wininet.h>
#include <urlmon.h>
#include <mshtml.h>

int _tmain(int argc, _TCHAR* argv[])
{
    HRESULT hr;
    CComPtr<IHTMLDocument2> document;

    hr = CoInitialize(0);
   
if (FAILED(hr))
    {
        exit(hr);
    }

    hr = document.CoCreateInstance(CLSID_HTMLDocument);
   
if (FAILED(hr))
    {
        exit(hr);
    }

    CoUninitialize();

   
return 0;
}

 

That's all it takes, I consciously chose not to add stuff to obfuscate the problem.

Btw, for those who've been reading this blog for a while, I covered this exact same issue in a different form a while ago.

 

  • I don't do much with COM object stuff, but I would guess that the CoUninitialize should be called before the second exit call.
  • I KNOW! You put the open braces on new lines when they should be at the end of the previous line! What do I win?
  • It exits without calling CoUninitialize? Also, it terminates the process with an HRESULT. I would normally have my own exit code.  CoInitialize returns a WINOLEAPI value and CoCreateInstance returns an HRESULT.  
  • CComPtr<IHTMLDocument2> document outlives the STA apartment created by CoInitialize(0). dtor in CComPtr<IHTMLDocument2> may generate an access violation calling Release() during stack cleanup.

    Also CoUninitialize() should be called before exiting from the failure of CoCreateInstance(CLSID_HTMLDocument);
  • That so easy, not even interesting:

    When CoUninitialize is called, it releases all COM objects, but the 'document' smart pointer (not very smart in this case) will still have the pointer (already invalid) which it will try to release when 'document' goes out of scope and, most likely, cause an access violation exception.
  • Patria nailed the real problem.  Vladimir, you're right it's uninteresting, but people still make this mistake.

    Btw, it's not "may". it's WILL - this test app blows up quite nicely (on my dev machine).

  • The reason I say "may" is that usually in developer's system, there are debuggers (or debugger launcher) to catch those access violations during application cleanup. But without those hooks, application just silently quits without any complaints.
  • I always do:

    int main()
    {
     CoInitialize();
     {
         CComPtr<IUnknown> pUnk;
         ...


      }
      CoUninitialize();
      return 0;
    }

    So I'll not be punched by CComPtr destructor.
  • Well, if you're going to use the Resource Acquisition Is Initialisation idiom, use it consistently:

    class CCoInitializer
    {
      CCoInitializer( DWORD dwCoInit )
      {
         CoInitializeEx( NULL, dwCoInit );
      }

      ~CCoInitializer()
      {
         CoUninitialize();
      }
    };

    int main()
    {
      CCoInitializer init( COINIT_APARTMENTTHREADED );
      CComPtr<IUnknown> pUnk;
     
      ...
    }
  • I don't know anything about CComPtr, but is it a problem that it appears to be a smart pointer to a HTMLDocument2 object but you are asking for a HTMLDocument?  Note the missing 2.
  • Mike, the COM object represented by CLSID_HTMLDocument implements (among other things) the IHTMLDocument2 interface.

    If it didn't, the CoCreateInstance call would fail.
  • Mike Dimmick:  RAII is a good idea, but don't you have to throw an exception from the constructor of the CoInitialize fails?
  • So the last &quot;What's wrong with this code&quot; article was dead easy, I knew it was likely that people would...
  • Mike Dimmick: Auto objects are a nice idea, but I think order of destruction is not guaranteed so depending on compiler optimisation you may run into blow-ups again.

    Also: everyone who understood the cause of the issue has been there, done that.  This was the first COM mistake I ever made. Right after unbalanced Addref/Release calls.
Page 1 of 1 (14 items)