BUGFIX: ATL's CWindowImpl crashes when OnFinalMessage contains code to destroy the class instance

ATL's main support class for implementing general-purpose windows is CWindowImpl. It has a feature that allows the class instance to be dynamically allocated and automatically manage its lifetime. All the derived class has to do is implement a method named OnFinalMessage: p>

virtual void OnFinalMessage(HWND hWnd);

Typical implementations is as follows:

virtual void OnFinalMessage(HWND /*hWnd*/)
{
    delete this;
}

This method is called when the window is destroyed - when processing the last message sent to any window - WM_NCDESTROY. This happens within DestroyWindow. So far so good. The problem arises when you call DestroyWindow from within a message handler within the same class:

LRESULT OnClose(UINT /*uMsg*/, WPARAM /*wParam*/, LPARAM /*lParam*/, BOOL& /*bHandled*/)
{
    DestroyWindow();
}

What happens is your OnFinalMessage is called from within DestroyWindow when the windowproc processes WM_NCDESTROY. At that moment your code happily destroys the class instance, so when DestroyWindow returns, you have no instance anymore. So far so good, as long as you don't have any code that references your instance after DestroyWindow you are fine. This may not be so easy to ensure if DestroyWindow is called indirectly from within a message handler though. Even if you don't have that concern, however, the next thing after you return from your message handler is the windowproc performs some housekeeping and happily accesses the class' this pointer. Oops, you got a crash!

The above sequence of events holds true for ATL 3.0 and has been fixed in ATL 7.0. So if you have upgraded to VC 7.0, or even better to VC 7.1, you don't get the crash. The fix has introduced another subtler bug though. The original code in ATL 3.0 cleared the class' m_hWnd member immediately before firing up OnFinalMessage when handling WM_NCDESTROY. And that is the correct behavior - m_hWnd is NULL after DestroyWindow returns. The fix in ATL 7 doesn't do that though, so you have an invalid HWND stored in m_hWnd. This wouldn't be a big problem if all programmers always checked if the window is valid via IsWindow(). Unfortunately, it's a mass practice to write that test as m_hWnd == NULL. The possibilities for subtle bugs not even related to OnFinalMessage boggle the mind...

I've written a small header SafeWinImpl.h, which defines a class to be used in place of CWindowImpl - CWindowImplSafe. It adds 4 bytes overhead to the window class instance data, otherwise it's completely equivalent to CWindowImpl - you can simply #include it in StdAfx.h after atlwin.h, replace all instances of CWindowImpl with CWindowImplSafe, and not notice a thing. Except for this kind of crashes disappearing in ATL 3.0. The code continues to work after upgrading to ATL 7 as well. In ATL 7 this class fixes the other bug - invalid m_hWnd after DestroyWindow. However, I'd advise anybody not to rush replacing CWindowImpl unless the problems outlined above have manifested themselves.

Download the code here: SafeWinImpl.h