What's wrong with this code, part 18, plus a bonus bad code
So the last "What's wrong with this code" article was dead easy, I knew it was likely that people would find it such.
patria found the answer on the 4th comment, and I think that Mike Dimmick put it best:
Well, if you're going to use the Resource Acquisition Is Initialization idiom, use it consistently:
CComPtr (an autoptr for COM objects that auto-addref's and releases the object) uses RAII, but the code didn't consistently use RAII - instead it pretended that it wasn't using RAII. Since CComPtr never throws, it's easy to treat it as a super pointer, but Mike's right - you have to be careful about lifetime issues, and that's exactly what went wrong in this example.
The problem is that when you call CoUninitialize, you need to ensure that you've released all references to any COM objects you might hold, if you don't, the DLL that hosts the COM objects is almost certainly going to have been uninitialized.
So let's present a "fixed" version of the code, using Mike's example of adding an RAII style object to work around the lifetime issue:
class
CCoInitializer
{
public:
CCoInitializer( DWORD dwCoInit )
{
HRESULT hr;
hr = CoInitializeEx( NULL, dwCoInit );
if (FAILED(hr))
{
throw hr;
}
}
~CCoInitializer()
throw()
{
CoUninitialize();
}
};
int
_tmain(int argc, _TCHAR* argv[])
{
HRESULT hr;
CCoInitializer coInitializer(COINIT_APARTMENTTHREADED);
CComPtr<IHTMLDocument2> document;
CComPtr<IHTMLElementCollection> images;
CComPtr<IDispatch> image;
LONG imageCount;
hr = document.CoCreateInstance(CLSID_HTMLDocument);
if (FAILED(hr))
{
exit(hr);
}
:
:
hr = document->get_images(&images);
if (FAILED(hr))
{
exit(hr);
}
hr = images->get_length(&imageCount);
if (FAILED(hr))
{
exit(hr);
}
for (LONG i = 0 ; i < imageCount ; i += 1)
{
hr = images->item(CComVariant(), CComVariant(i), &image);
if (FAILED(hr))
{
exit(hr);
}
}
return 0;
}
While I was fixing the code, I added a bit of additional stuff. Unfortunately, the new code introduced yet another bug.
Btw, for those playing along at home, I know that this doesn't actually work, code to load up the HTML document is in the omitted section :).
In addition, the absence of code to check for the coInitializer object throwing is NOT a bug. There's no way of recovering from this exception, and the exception handling paradigm states that if you don't know how to handle an exception, you let your caller handle it.
Comments
- Anonymous
March 20, 2006
It looks like this version will leak all of the objects returned via images->item() except for the last one. - Anonymous
March 20, 2006
It will assert on the & operator since the internal pointer inside of CComPtr isn't NULL. It is that pesky helper in ATL that makes CComPtr a pain in use with STL.
In the long run, yes it will leak.
IMHO, bugs like this are caused by the old style of declaring variables at the top. - Anonymous
March 20, 2006
CComPtr<IDispatch> image should be declared inside the for-loop.
for () {...
CComPtr<IDispatch> image;
hr = images->item(CComVariant(), CComVariant(i), &image);
}
And isn't it bad to use the temporal object CComVariant(), CComVariant(i) in the function call? In this case, it can be okay, but it can be quite problematic in some cases. - Anonymous
March 20, 2006
Two things:
1. exit() function terminates the process without calling the obejct destructors
2. CComPtr's T** operator&() works different from _com_ptr_t's where it does not release the underlying pointer, so images->item(CComVariant(), CComVariant(i), &image) will cause a debug assert and leak the interface pointer (because the 3rd parameter isn't in, out but out, retval) - Anonymous
March 20, 2006
Well - exit() defeats the whole RAII scheme.
All the calls to exit() should be either returns, throws or gotos -- something the C++ compiler knows about so it can unwind the vars. - Anonymous
March 20, 2006
Full RAII is good, but it makes you write a lot more code if you're dealing with C stuff (like, say, Win32 and COM API). It's perfect in C++ world, but we're in C-based Windows.
Throwing HRESULT is also a bad idea since it's just a basic type. To do it the right way you should create a CComError or something like this (like VC's _com_error). The more C++ish you try to become, more dumb code you need to write (but I've found ATL has a lot of useful little Win32 classes). - Anonymous
March 20, 2006
The same bug has bitten me before with CComBSTR -- pass it as an 'out' parameter, and it will leak memory if it was already holding a reference to a BSTR. - Anonymous
March 20, 2006
Isn't the fact that you are using the STA but not pumping messages also a bug? - Anonymous
March 20, 2006
You don't have to pump messages just to instantiate a COM object and make method calls on it in an STA as long as you aren't waiting on any callbacks to complete from other threads or processes.
In this particular case he purposely left out the HTML doc loading piece which DOES require you to pump messages since it uses async monikers from URLMON under the covers to perform the document loading. And you generally will not get anywhere with async monikers unless you pump messages.
The typical pattern with MSHTML is to start the document load and then go into a message pump. You break out of the message pump when the ReadyState hits a state you are happy with (Interactive or Complete usually). - Anonymous
March 20, 2006
I'm not up on all my COM stuff, but throwing on FAILURE seems wrong. Don't you still need to call CoUninit()?
This seems to back this up...
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/com/html/0f171cf4-87b9-43a6-97f2-80ed344fe376.asp
From the docs: "To close the COM library gracefully, each successful call to CoInitialize or CoInitializeEx, including those that return S_FALSE, must be balanced by a corresponding call to CoUninitialize." - Anonymous
March 20, 2006
Thanks hippietim. I guess since in general you don't know what any COM object you instantiate is doing under the covers it's pretty risky to use the STA if you are not pumping messages {if you are certain that the COM methods you call, and any future versions of them, never do any cross-apartment calls you are technically safe but I'm not sure I'd like to rely on it}. - Anonymous
March 20, 2006
More things you shouldn't do: call exit(). Especially from the main function, where it's completely unnecessary. - Anonymous
March 20, 2006
Why was thread synchronization built into COM, I never understood this. It's sad that callers are forced to pump messages just to support the runtime. How hard is it;acquire a lock around data potentially accessed concurrently. It just seems that a feature designed to simplify things causes so many problems. - Anonymous
March 20, 2006
The comment has been removed - Anonymous
March 21, 2006
hippietim, it's adorned with the throw() statement because the destructor can't throw. - Anonymous
March 21, 2006
The comment has been removed - Anonymous
March 22, 2006
The comment has been removed - Anonymous
March 23, 2006
You can leave image outside the loop (so you don't end up constructing a CComPtr object every single time you enter the loop - perf issue).
Then in order to not leak, I would simply Release it like this (using comma operator):
for (LONG i = 0 ; i < imageCount ; image.Release(), i += 1)
{
hr = images->item(CComVariant(), CComVariant(i), &image);
if (FAILED(hr))
{
exit(hr);
}
}
Comma operator is also very usefull when you have a while instead of a for. I use it all the time when I enumerate a DShow filter using an IEnumXXX::Next call:
CComPtr<ISomeInterface> pItem;
while(pItem.Release(), hr = pEnum->Next(1, &pItem, ...))
{
do smtg here with pItem
}