vsgWin32::Win32_Window continues using HWND after it's been destroyed

[AnyOldName3]

vsgWin32::Win32_Window attempts to destroy its window handle, _window, in its destructor by calling DestroyWindow(_window). However, because vsgWin32::Win32WindowProc (the window message handling CALLBACK) calls DefWindowProc (the default window message handler), when a WM_CLOSE message is received, the window's closed and destroyed before the destructor runs.

This is especially bad because HWNDs are reused for new windows, so the destructor could potentially destroy another process' unrelated window. Thankfully, it's unlikely as Windows actively defends against this kind of bug, using some of a HWND's bits for the window table index and some for a generation number so the same index won't turn into the same HWND when it's reused until those bits have been exhausted.

I used this patch to identify the unwanted window murderer, based on this OldNewThing post: https://devblogs.microsoft.com/oldnewthing/20111026-00/?p=9263
who-killed-my-window.patch

[robertosfield]

Is the WM_CLOSE message a signal that the window is closed or is it a request to close the window?

The Win32_Window::handleWin32Messages(..) method currently treats the WM_CLOSE as a request to close rather than attempting to handle anything more.

Is the ::DefWindowProc(hwnd, msg, wParam, lParam) call in Win32WindowProc responding to the WM_CLOSE and doing the window destruction?

Can we prevent the window from being destroyed by anyone else other than the destructor. If ::DefWindowProc() is the one doing the window destruction then can we just not pass on the message to ::DefWindowProc() when Win32_Window::handleWin32Messages(..) handles the event?

[AnyOldName3]

Is the WM_CLOSE message a signal that the window is closed or is it a request to close the window?

It's a request to close, fired by things like pressing the X in the corner of a window.

Is the ::DefWindowProc(hwnd, msg, wParam, lParam) call in Win32WindowProc responding to the WM_CLOSE and doing the window destruction?

Yep. The default handling for the close message is to close and destroy the window.

Can we prevent the window from being destroyed by anyone else other than the destructor. If ::DefWindowProc() is the one doing the window destruction then can we just not pass on the message to ::DefWindowProc() when Win32_Window::handleWin32Messages(..) handles the event?

That's the typical way to deal with it, and is even what the example for WM_CLOSE does: https://learn.microsoft.com/en-us/windows/win32/winmsg/wm-close#example

[robertosfield]

I have created a Win32_Window_Refinements branch that implements the suggested approach and also added checks against GetClientRect to avoid possibly invalid values being used:

https://github.com/vsg-dev/VulkanSceneGraph/tree/Win32_Window_Refinements

This branch was made directly from VSG master rather than your modifications with the error reporting, I need to spend some time with that branch to understand the APIs used.

[AnyOldName3]

The gist of the API usage in my branch is that lots of (but not all) Win32 functions set or reset a thread-local error state when they fail that can be queried with GetLastError(). That gives you a numerical code, and if you pass that code to the message-formatting function with particular flags that explain that that's what you've given it, it can return a human-readable string. On my branch, when one of the functions that could fail did fail, I'd call a function which would call GetLastError to get the numerical code and query the corresponding text, then throw an exception with that information so there'd be some way to figure out what might have gone wrong other than guessing.

[robertosfield]

I've reviewed the reportWin32Error() implementation and am currently inclined towards this having a similar getWin32Error() function that returns a std::string or std::wstring, with the code that calls this function decided how to report the error, and whether throwing an exception is appropriate for each case.

If std::wstring is really required then perhaps a vsg::WideException would be appropriate.

[robertosfield]

I've done a online search about the FormatMessageA function and came across this stackoverflow discussion:

https://stackoverflow.com/questions/65774399/should-formatmessagea-formats-the-message-in-correct-way-even-on-a-unicode-os

General response seems to be don't use FormatMessageA use FormatMessageW instead and if you really need to a ascii string then use RtlUnicodeStringToAnsiString:

https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-rtlunicodestringtoansistring

I've re-read the FormatMessageA docs and it just talks about string, it doesn't ever talk about cstring or Ascii, but does the A in the function name mean Ascii? I presume the W in FormatMessageW is a wide string.

If so then use FormatMessageA should be fine and the commenters on stackoverflow and just being unhelpful.

[AnyOldName3]

The A basically means ASCII (technically it's the initial for ANSI but the phrase ANSI string doesn't mean anything specific outside of the Win32 docs where it means eight-bit strings), but as 'proper' ASCII is a seven-bits-padded-to-eight encoding, it's not ever actually ASCII. It uses the system's configured eight-bit code page, which will usually be a fixed-width eight-bit encoding like CP1252 (the default for English Windows which has 7-bit ASCII plus a bunch of extra characters), but could be a variable-width encoding like Shift-JIS (well, technically CP932, which is subtly different) that may not be fully ASCII-compatible, or could be regular UTF-8 on modern Windows if the user's changed a setting. If you take the output of an A-suffixed function and feed it to another A-suffixed function, it'll usually end up displayed fine as long as it's written in English or the user's configured language, but some characters could end up replaced by ?. If you do anything else with it and don't either ask Windows to convert it to a specific encoding or query the current encoding and use encoding-specific behaviour, things can break.

The W is for wide string variants of functions, and for all intents and purposes, they always use UTF-16. There are a few that work with opaque sequences of sixteen-bit numbers, so they can consume or emit invalid UTF-16 strings, but you can more-or-less ignore this.

As Windows uses UTF-16 internally basically everywhere, nearly every A-suffixed function is implemented by calling RtlAnsiStringToUnicodeString on any arguments, then calling the W-suffixed function, then calling RtlUnicodeStringToAnsiString on anything it returns (substituting ? for any character the current eight-bit encoding doesn't support).

To make things even more complicated, there's a preprocessor define for each A-or-W-suffixed function that expands to the W-suffixed variant when UNICODE is defined, and the A-suffixed variant otherwise, and defines that could either be char/char* or wchar_t/wchar_t* depending on whether UNICODE is defined, too, so you can have extra opportunities to accidentally trigger encoding mismatches but with bonus ABI mismatches for added chaos.

In principle, when FormatMessage is told to return the language-neutral message (as in my PR), it should be representable by the current eight-bit codepage no matter what it's currently set to, which would suggest that it's restricted to the common subset of ASCII that all possible Windows code pages use, which is in turn a subset of the overlap between ASCII and UTF-8. However, it's not guaranteed that there's a language-neutral message for every error code, so sometimes it might return something in the user's system language, and that could use any character in their local eight-bit codepage, and end up as mojibake if it's interpreted as UTF-8.

So with all that in mind, I think the only thing that definitely would always give a sensible string and never replace anything with question marks or nonsense would be to use the W-suffixed function and then convert to UTF-8, and ensure it's always interpreted as UTF-8.

[robertosfield]

Thanks for the explanation, it makes more sense now.

I used your reportWin32Error() function as a basis for getLastErrorAsException() function that returns a vsg::Exception rather than throw an exception:

46768af

This is checked into the branch:

https://github.com/vsg-dev/VulkanSceneGraph/tree/Win32_Window_Refinements

I also added stream output support for vsg::Exception so you can now easily write it to an ostream or use it with the vsg::Logger functions like vsg::warn(..) etc. I have changed the Win32_Window destructor to use this when reporting the issue detected:

        if (!::DestroyWindow(_window))
        {
            vsg::warn("Win32_Window::~Win32_Window() ::DestroyWindow(_window) failed : ", getLastErrorAsException());
        }

If you wanted to throw an exception then you could just write:

        if (!::DestroyWindow(_window)) throw getLastErrorAsException();

In this case I don't think an exception is warranted. Other places there are checks against success of running Win32 functions that throw vsg::Exceptions, these could be modified to include these.

I'm now wondering if it might be best to pass in a stringview into getLastErrorAsException that is not null could be used as a prefix for the vsg::Exception.message string.

I prefer this approach of making throwing an exception under the control of the original code that attempted the Win32 API call that failed, rather than having an implicit throw happening in a function as this requires developers to look at the implementation of the function to understand the behaviour.

Could you review the approach I've taken and suggest further refinements if required. We should be able reimplement the places where you added extra checks against Win32 API return values using this new scheme. I think the original code that was already throwing exception would probably be well served by updating them as well.

[robertosfield]

I have added the std::string_view prefix to the getLastErrorAsException(..) and this cleans up usage so that writing to std::ostream or using vsg::Logger or throwing an exception can all work in a very similar way: b54eadf

I'm now pretty happy with the branch - it fixes the issue of the HWMD getting destroyed while the Win32_Window still has a handle to it and streamlines the reporting of the Win32 API error code and message.

[AnyOldName3]

If it's going to be a utility function, I'd have thought it would make most sense to return something like a std::pair<int, std::string>, then the call site could decide whether it wanted to convert it to an exception or just log the string or check the status code against one of the constants. It seems weird to construct an exception if it's not necessarily going to be thrown. It would also make it simpler to get the string and put it in an exception with a custom prefix.

[robertosfield]

I considered using a std::pair<> but decided it would make the usage more verbose. I tried to keep the API easy to slot into existing code and make it straightforward to use rather than attempt to make it a "general purpose" style API.

[robertosfield]

@AnyOldName3 I have added throw where your report function calls were: 981aa0b

I don't have access to my office right now as the household is building site, this means I've just set up my Linux desktop temporarily so can't test Windows right now. Could you test the Win32_Window_Refinements branch under Windows and if everything goes smoothly I'll merge with VSG master and start looking at making another developer release.

[AnyOldName3]

It's now throwing an unhandled exception on startup when calling SetWindowLongPtr. The documentation for that function says:

Return value

Type: LONG_PTR

If the function succeeds, the return value is the previous value of the specified offset.

If the function fails, the return value is zero. To get extended error information, call GetLastError.

If the previous value is zero and the function succeeds, the return value is zero, but the function does not clear the last error information. To determine success or failure, clear the last error information by calling SetLastError with 0, then call SetWindowLongPtr. Function failure will be indicated by a return value of zero and a GetLastError result that is nonzero.

The userdata for a window defaults to zero, so it returns zero, and the if (!SetWindowLongPtr(...)) fails. I'll give their recommended fix a try.

[AnyOldName3]

I got rid of the incorrect exception, but now I'm seeing a validation error. I don't know what I was doing when I triggered it, so I don't know if it's likely to be related to one of these changes. I'll see if I can reproduce it.

VUID-vkQueueSubmit-pCommandBuffers-00065(ERROR / SPEC): msgNum: 1901485743 - Validation Error: [ VUID-vkQueueSubmit-pCommandBuffers-00065 ] Object 0: handle = 0xead9370000000008, type = VK_OBJECT_TYPE_SEMAPHORE; Object 1: handle = 0x2711455de40, type = VK_OBJECT_TYPE_QUEUE; Object 2: handle = 0x2711455de40, type = VK_OBJECT_TYPE_QUEUE; | MessageID = 0x71565eaf | vkQueueSubmit(): pSubmits[0].pSignalSemaphores[0] (VkSemaphore 0xead9370000000008[]) is being signaled by VkQueue 0x2711455de40[], but it was previously signaled by VkQueue 0x2711455de40[] and has not since been waited on. The Vulkan spec states: Any calls to vkCmdSetEvent, vkCmdResetEvent or vkCmdWaitEvents that have been recorded into any of the command buffer elements of the pCommandBuffers member of any element of pSubmits, must not reference any VkEvent that is referenced by any of those commands in a command buffer that has been submitted to another queue and is still in the pending state (https://vulkan.lunarg.com/doc/view/1.3.283.0/windows/1.3-extensions/vkspec.html#VUID-vkQueueSubmit-pCommandBuffers-00065)
    Objects: 3
        [0] 0xead9370000000008, type: 5, name: NULL
        [1] 0x2711455de40, type: 4, name: NULL
        [2] 0x2711455de40, type: 4, name: NULL