C++ destructors/window subclassing

**sethjackson** · 06-13-2010

I'm trying to make a simple C++ wrapper around the Windows UI library but I'm getting some weird crashes that I can't seem to debug.

I have a base class Widget that does all the RegisterClassEx()/CreateWindowEx()/window subclassing in the class constructor.

Window is derived from Widget which sets up a top level window,
allows you to add a child, etc.

Now the library code is in a DLL, and my test app is linking against it.
So in the test app I add a button to the Window. The button is displayed, sized correctly,
etc, BUT when I close the window the test app crashes. I've traced it to deleting the child widget in the Window destructor, but I have no idea WHY this is causing the test app to crash...

Second thing, since my library lives in a DLL, I can't use GetModuleHandle(0), for when I need a HINSTANCE correct? Because that would return the exe's address space??

Widget.cpp

Code:

// I think this is the correct way of doing this...
HMODULE GetInstance()
{
    HMODULE hModule;
    
    GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
                      (LPCTSTR)&GetInstance,
                      &hModule);
    
    return hModule;
}

Widget::Widget()
{
    CreateWidget("MyClassName");
}

Widget::Widget(LPCTSTR className)
{
    CreateWidget(className);
}

Widget::~Widget()
{

}

void Widget::CreateWidget(LPCTSTR className)
{
    static bool registered = false;
    
    if (!registered)
    {
        WNDCLASSEX wcx;
        
        wcx.cbSize = sizeof(wcx);
        wcx.style = 0;
        wcx.lpfnWndProc = DefWindowProc;
        wcx.cbClsExtra = 0;
        wcx.cbWndExtra = 0;
        wcx.hInstance = GetInstance();
        wcx.hIcon = NULL;
        wcx.hCursor = static_cast<HCURSOR>(LoadImage(NULL, IDC_ARROW, IMAGE_CURSOR, 0, 0, LR_DEFAULTSIZE));
        wcx.hbrBackground = NULL;
        wcx.lpszMenuName = NULL;
        wcx.lpszClassName = "MyClassName";
        wcx.hIconSm = NULL;
        
        if (!RegisterClassEx(&wcx))
            throw Exception("The window class could not be registered.");
        
        registered = true;
    }
    
    m_handle = CreateWindowEx(0,                  // extended styles
                              className,          // class name
                              "",                 // window name
                              0,                  // window styles
                              0,                  // x
                              0,                  // y
                              CW_USEDEFAULT,      // width
                              CW_USEDEFAULT,      // height
                              NULL,               // parent window
                              NULL,               // menu handle
                              GetInstance(),      // instance handle
                              NULL);              // create params
    
    if (m_handle == NULL)
        throw Exception("The widget could not be created.");
    
    SetWindowLongPtr(m_handle, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
    
    // subclass the window
    m_oldWndProc = reinterpret_cast<WNDPROC>(GetWindowLongPtr(m_handle, GWLP_WNDPROC));
    
    SetWindowLongPtr(m_handle, GWLP_WNDPROC, reinterpret_cast<LONG_PTR>(WindowProc));
}

// ... other code snipped

LRESULT CALLBACK Widget::ProcessMessage(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
    return CallWindowProc(m_oldWndProc, hwnd, uMsg, wParam, lParam);
}

LRESULT CALLBACK Widget::WindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
    Widget* widget = reinterpret_cast<Widget* >(GetWindowLongPtr(hwnd, GWLP_USERDATA));
    
    if (widget)
        return widget->ProcessMessage(hwnd, uMsg, wParam, lParam);
    
    return DefWindowProc(hwnd, uMsg, wParam, lParam);
}

Window.cpp

Code:

Window::Window()
{
    // change the window style to a top level window
    SetWindowLongPtr(GetHandle(), GWL_STYLE, WS_OVERLAPPEDWINDOW);
    
    m_child = nullptr;
}

Window::Window(LPCTSTR className)
    : Widget(className)
{
    // change the window style to a top level window
    SetWindowLongPtr(GetHandle(), GWL_STYLE, WS_OVERLAPPEDWINDOW);
    
    m_child = nullptr;
}

Window::~Window()
{
    // with this in it's a guaranteed crash...
    //delete m_child;
}

void Window::SetChild(Widget* child)
{
    if (child == nullptr)
        return;
    
    delete m_child;
    m_child = nullptr;
    
    m_child = child;
    
    m_child->SetParent(this);
    
    // layout the window
    Layout();
}

LRESULT CALLBACK Window::ProcessMessage(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
    switch (uMsg)
    {
        case WM_CLOSE:
            DestroyWindow(hwnd);
        break;
        
        case WM_DESTROY:
            PostQuitMessage(0);
        break;
        
        case WM_SIZE:
            Layout();
        break;
        
        default:
            return Widget::ProcessMessage(hwnd, uMsg, wParam, lParam);
    }
    
    return 0;
}

Test.cpp

Code:

int main(int argc, char* argv[])
{
    Window win;

    Button* btn = new Button();
    btn->Show();

    win.SetChild(btn);
    win.Show();

    // Init common controls
    // Run windows event loop...
    Application::Run();

    return 0;
}

So once I click the close button on the window everything blows up.
Also if I derive a C++ class from the Window object in my test app I get a crash on
startup...

I'm probably doing something insanely stupid somewhere, but I can't seem to find it....

EDIT:

I forgot to mention that my test app *still* crashes even if I don't do "delete m_child;". It just crashes less consistently.

**novacain** · 06-14-2010

At first glance, but not sure if this is the issue....

You are ignoring the classname input to CreateWidget() and all classes are registered as 'MyClassName'.

**sethjackson** · 06-15-2010

The class "MyClassName" is only registered once, the first time the Widget constructor is called. It's lazy initialized (actually this technique isn't thread-safe but I'm not concerned about that right now)...

The class name is being used properly.
I can create a button ("BUTTON" class) just fine.

See below:

Code:

Button::Button()
    : Widget("BUTTON")
{

}

So the class name is passed in for widgets that need it and the other
widgets that aren't system widgets are using the "MyClassName" via the default ctor.

Now as to the crash on app exit.
GDB says this:

Code:

warning: HEAP[test.exe]:
warning: Heap block at 00391428 modified at 00391438 past requested size of 8


Program received signal SIGTRAP, Trace/breakpoint trap.
0x77d78b2f in ntdll!DbgUiConvertStateChangeStructure ()
   from C:\Windows\system32\ntdll.dll

Backtrace gives this:

Code:

(gdb) backtrace
#0  0x77d78b2f in ntdll!DbgUiConvertStateChangeStructure ()
   from C:\Windows\system32\ntdll.dll
#1  0x77de162b in ntdll!RtlpNtMakeTemporaryKey ()
   from C:\Windows\system32\ntdll.dll
#2  0x77dca6d7 in ntdll!RtlInitAnsiStringEx ()
   from C:\Windows\system32\ntdll.dll
#3  0x77db24d0 in ntdll!EtwpNotificationThread ()
   from C:\Windows\system32\ntdll.dll
#4  0x77de267f in ntdll!RtlpNtMakeTemporaryKey ()
   from C:\Windows\system32\ntdll.dll
#5  0x77dab7cd in ntdll!EtwpNotificationThread ()
   from C:\Windows\system32\ntdll.dll
#6  0x77d97545 in ntdll!RtlEqualLuid () from C:\Windows\system32\ntdll.dll
#7  0x76889a26 in KERNEL32!HeapLock () from C:\Windows\system32\kernel32.dll
#8  0x76b49c03 in msvcrt!free () from C:\Windows\system32\msvcrt.dll
#9  0x00390000 in ?? ()
#10 0x6c986781 in operator delete(void*) ()
#11 0x6c981ced in Button::~Button (this=0x391430)
#12 0x6c983ecc in Window::~Window (this=0x22fee0)

But I'm not sure why "delete m_child" is causing the crash... ??

**VirtualAce** · 06-15-2010

Save yourself some time and either use something like MFC or another third party API that makes windowing in Windows a snap or use C#. Other than for pure learning purposes or because you enjoy self inflicted pain there is no reason to write your own wrapper for the Win32 API.

**sethjackson** · 06-15-2010

While I *could* use MFC/Gtk+/Qt/wxWidgets/WinForms/whatever else, I choose not to.
Anyone gather any ideas from the backtrace??

**novacain** · 06-15-2010

Sorry, missed the 'static' declaration.

Why do you use the WS_OVERLAPPEDWINDOW style for all windows rather than the usual WS_CHILD|WS_POPUP?

**sethjackson** · 06-16-2010

base class for all UI elements is Widget. Window represents a top level window, not a button or text entry, those are derived directly from Widget.
So top level windows need the WS_OVERLAPPEDWINDOW style.

There is a Widget::SetParent() method that takes care of reparenting widgets on the fly.

**novacain** · 06-16-2010

You are obviously trashing the memory somewhere, but without the rest of the code these are the best guesses....

Does your SetParent() add the child style, ensuring that the main window's call to DestroyWindow() cleans up the child resources? (as the WIN32 SetParent() does not change the window's styles).

Do you think that creating a button with the OVERLAPPED style (which includes a system menu) but without the child style, might not be cleaning up correctly, causing an issue when you try to cast the user data back to the object?

Will a static cast return a null pointer if there is no user-data? Or will it return an invalid pointer? When in the destruction process does this become a dangling pointer?

Your app will probably stop processing msgs once it gets a WM_QUIT. Which control calls PostQuitMsg() first, the one from the button or the main window?

Does it matter if the button purges the msg queue before the main window has finished processing the cleanup msgs (WM_NCDESTROY etc)?

**sethjackson** · 06-18-2010