A common mistake when you try to create a C++ class that wraps a window procedure: Saving the window handle too late
Raymond Chen
A common mistake when you try to create a C++ class that wraps a window procedure is saving the window handle too late.
// Code in italics is wrong.
class MyWindowClass
{
private:
HWND m_hwnd = nullptr;
static LRESULT CALLBACK StaticWndProc(
HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
MyWindowClass *self;
if (uMsg == WM_NCCREATE) {
LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
self = static_cast<MyWindowClass*>(lpcs->lpCreateParams);
SetWindowLongPtr(hwnd, GWLP_USERDATA,
reinterpret_cast<LONG_PTR>(self));
} else {
self = reinterpret_cast<MyWindowClass*>(
GetWindowLongPtr(hwnd, GWLP_USERDATA));
}
if (self) {
return self->WndProc(uMsg, wParam, lParam);
}
return DefWindowProc(hwnd, uMsg, wParam, lParam);
}
LRESULT WndProc(UINT uMsg, WPARAM wParam, LPARAM lParam)
{
switch (uMsg) {
...
default:
return DefWindowProc(m_hwnd, uMsg, wParam, lParam);
}
}
public:
void CreateTheWindow()
{
... RegisterClass() ...
m_hwnd = CreateWindowEx(..., this);
}
};
This code follows the usual pattern for a window procedure wrapper: The this pointer is passed as the creation parameter, and the WM_NCCREATE message handler stashes the creation parameter in the window extra bytes, thereby allowing the this pointer to be recovered from the window handle when handling future messages.
However, there’s a problem with the above code. Can you spot it?
The problem is that it sets the m_hwnd member variable too late.
As written, the code doesn’t set the m_hwnd member variable until the CreateWindowEx function returns. But creating a window involves sending many messages.
For every message received during window creation, The WndProc member function runs with a null m_hwnd. This means that when it calls DefWindowProc(m_hwnd, ...), it’s passing an invalid parameter.
Many of the messages sent during window creation are kind of important to pass through to DefWindowProc. For example, if you neglect to pass WM_NCCREATE to DefWindowProc, your window will not be properly initialized.
The solution is to set m_hwnd as soon as you learn what the window handle is.
if (uMsg == WM_NCCREATE) {
LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
self = static_cast<MyWindowClass*>(lpcs->lpCreateParams);
self->m_hwnd = hwnd; // save the window handle too!
SetWindowLongPtr(hwnd, GWLP_USERDATA,
reinterpret_cast<LONG_PTR>(self));
}
Don’t wait for CreateWindowEx to return. By then, it’s too late.
Light
Dark
18 comments
Is WM_NCCREATE guaranteed by contract to be the first message sent to a window? Its documentation explicitly says it’s send before WM_CREATE, but CreateWindow() documentation also mentions other messages, such as WM_GETMINMAXINFO. I think it’s reasonable to expect the creation messages (WM_NCCREATE and WM_CREATE) to be the very first sent, but when several messages are sent as part of one operation, expecting them in a determinate order has caused compatibility problems in the past, so better safe than sorrow.
As I recall, WM_GETMINMAXINFO is sent before WM_NCCREATE. Raymond’s code doesn’t seem to forward that initial WM_GETMINMAXINFO to MyWindowClass::WndProc.
Strange… Notifying a window of its placement and requesting modifications to it does not seem a sensible move before the window itself is created. But nobody said message ordering had to be intuitive, and Raymond has shown us that sometimes it isn’t.
WM_GETMINMAXINFO queries the constraints on window size. If you asked after WM_NCCREATE, your window could initially be created with the wrong size! Asking beforehand sure seems sensible to me.
Perhaps. But one could also argue that querying for window size/position just after the call to CreateWindow() or CreaateWindowEx(), which lets you explicitly provide that information, is, at the very least, redundant.
It’s not as if you can save it at any point before then without using external resources.
The only way you know how to get the window handle into the class without using external resource is by knowing what class you are talking about. Since this is passed into the window via CREATESTRUCT, the very first time you get to see CREATESTRUCT is in WM_NCCREATE.
I’m sure that if you try very hard, you could devise a way to get the window handle into the class sooner, but remember that by necessity the window procedure must be a standalone function/static member function (StaticWndProc) and not a member function with a this pointer. This means that the window procedure also can’t know about the class that backs the window without extraordinary methods being used. So what the actual window procedure does is save the class pointer when it can and then after this forward it to the class’ window procedure (WndProc) for message handling when it can.
It is important to note that the window procedure that does get the WM_GETMINMAXINFO message has the window handle, by this point it doesn’t have the pointer to the backing class to save the class pointer into it. So if you write the static/standalone window procedure correctly it will use DefWindowProc with the correct window handle while the class pointer hasn’t been saved into the window’s user data via SetWindowLongPtr using GWLP_USERDATA.
Well, WM_GETMINMAXINFO is only really needed if you want to override the certain aspects of the window’s size and it is very rare that anyone wants to do that.
A neat trick with static callbacks is to use a threadlocal variable to pass context. In this case you only need to check it if GWLP_USERDATA is empty and then you can use it to initialize the calling class on the very first message.
That’s not correct. You can register a (thread-local) CBT hook, and get notified about window creation before any messages are being sent out. Since window creation is strictly single-threaded, you can safely transfer a class instance pointer using thread local storage. The CBT hook procedure also has access to the HWND, so you can tuck the instance pointer away right there.
MFC uses this technique to establish the relationship between its C++ class instances and the system-provided HWND’s.
I was unsure of the first line, thank you for showing me that I was right to be unsure. I will edit that post to better reflect what I mean.
By the way, if you read the whole post and not the first line then you would see that I talk about devising ways to get the pointer to the class into the static window procedure sooner. So I did cover things like this.
Another trick is to use a dynamically allocated thunk for the StaticWndProc, and tuck away the class ‘this’ pointer inside the thunk itself before CreateWindow/Ex() is even called. The thunk can access its ‘this’ pointer directly and forward all messages directly to the class WndProc, without relying on CREATESTRUCT or GWLP_USERDATA at all. This is the technique that Borland’s VCL uses to link its classes to the HWNDs they create.
I will object to the idea, that handling WM_GETMINMAXINFO were a rare occurrence. Especially for dialogs it is very common to want to set a minimum tracking size.
It would be good defensive programming to initialize m_hwnd to null in the constructor and then test m_hwnd in any message handler before using it.
It would be much better for performance to make it contractual that m_hwnd must be a valid window handle by the time WM_NCCREATE has finished processing. This means that you could basically assert on it rather than having a branch instruction at the start of every handler.
Assuming that you do proper testing with the debug version of the application, you can assume that the handle is always valid and the assert will compile away to nothing for the release builds.
This is a good example for similar situations, but for this specific wrapper, the safe solution could be to change the signature of WndProc and always pass hwnd to it.
It wouldn’t solve anything. You are just shifting the blame. StaticWndProc() must obtain the handle from somewhere in order to pass it to WndProc, and you can’t obtain it from thin air, since StaticWndProc() is a static method (it has to be, because it has to fit the standard window procedure definition), and has to work with the hwnd provided by the window manager, without knowing beforehand if it’s registered or to which object instance it is. Also, WndProc() isn’t static, so you need a this/self pointer to call it, at which point you would be able to check m_hwnd and initialize it if necessary.
StaticWndProc() doesn’t need to go looking for it, since it received the HWND in its first parameter.
The problem is still the same. If you are building a class, you surely will have more methods than just WndProc() and StaticWndProc(). In fact, the sample code shows at least one: CreateTheWindow(). If you wanted to call WndProc() without having an instance pointer (this or self), WndProc() itself would have to be static, and it could not call any of the non-static methods, at least not directly. That’s why the implementation takes the trouble of dividing the window procedure in static and non-static parts.
In other words: the problem (and the cause of the race condition) is not getting the hwnd, but linking it to the correct instance of the MyWindowClass. Both an orphan hwnd or an uninitialized MyWindowClass object are useless on their own.
Everytime I see this:
default:
return DefWindowProc(m_hwnd, uMsg, wParam, lParam);
it irks me tremendously. This is also used in several WndProc samples.
While itself it’s correct, as soon as some one handles a message but also needs to call DefWindowProc he has to sprinkle the DefWindowProc call everywhere. Or more common, forgets to pass the message on at all.
It’s basically a sample induced bug waiting to happen.