Windows: Changed thread_event_target_callback's WM_DESTROY to WM_NCDESTROY

[VZout]

Our application was not exiting gracefully but crashed on platform_impl/windows/event_loop.rs:1927.
WM_DESTROY destroys subclass_input but WM_NCDESTOY is send after WM_DESTROY (See MS docs) which causes subclass_input.event_loop_runner.clone(); to fail.

Examples don't repro, are the they just lucky that the memory didn't change?
Creating a minimal test case from our project would be quite a lot of work.

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

[VZout]

Actually seems exactly the same as #1745 but thread_event_target_callback instead of public_window_callback

[chrisduerr]

The documentation says WM_DESTROY is for destruction of the main window, while WM_NCDESTROY is for child windows.

Looking at the docs, it seems like winit should be using WM_DESTROY, what makes you believe that WM_NCDESTROY is the better choice?

[VZout]

Because WM_NCDESTROY is also emitted if there are no child windows I believe. Another solution would be returning immediatly if a WM_NCDESTROY event is received and still use WM_DESTROY.

[chrisduerr]

But WM_NCDESTROY being emitted doesn't seem sufficient as a reason to use it? That just seems like a coincidental fix.

[maroider]

I'm (obviously) not @VZout, but...

I think you've misunderstood the docs.
When a window receives WM_DESTROY, three things are promised:

1. The window is being destroyed.
2. Any child windows still exist (but will soon be destroyed).
3. (implicitly) A WM_NCDESTROY message will be dispatched soon.

For WM_NCDESTROY, the following should hold true:

1. Any child windows are now destroyed
2. MSDN "This message frees any memory internally allocated for the window."

This would suggest that the window can't receive any further messages after WM_NCDESTROY since Windows has presumably invalidated the window's handle, message queue, and other associated state (see this SO answer).

EDIT: Yikes, didn't see the activity after looking into this (I apparently started writing this minutes before @VZout answered)

[VZout]

Your answer was more detailed anyway ♥

[maroider]

I think it's worth mentioning the use-after-free (and perhaps how window destruction triggers it) here.

[VZout]

How about:
"On Windows, fix applications not exiting gracefully due to thread_event_target_callback accessing corrupted memory."
or a more detailed version:
"On Windows, fix applications not exiting gracefully due to WM_DESTROY events destroying subclass_input but WM_NCDESTOY is send after WM_DESTROY where the former uses subclass_input."

[VZout]

changed it to my first suggestion :)

[chrisduerr]

I still find it questionable that memory is freed based on an event which hopefully is the last one received, that seems very unsafe to me.

But I have no knowledge of the Windows backend and do not plan to change that.

[maroider]

It is unfortunate that this isn't documented all that well, but it's probably safe to do it this way, provided that no other funny buisness occurs (read: trying to destroy a window after it has received WM_DESTROY).

[chrisduerr]

it's probably safe to do it this way,

I'd say "probable safe" is not good enough personally.

As I've stated previously I'm not familiar with the Windows winit internals, but this error seems to be triggered by trying to handle events after we've already decided that the lifetime was over. Would it not be possible to store this information internally and prevent further events from being processed even if they would come in?

[maroider]

It should be possible to replace the window's "window procedure" (event handler function) with a no-op function after the window receives WM_NCDESTROY, which would make it safe to deallocate subclass_input regardless of what messages are received afterwards.

WM_NCDESTROY => {
    SetWindowLongW(hwnd, GWL_WNDPROC, no_op_window_callback);
    // deallocate `subclass_input`
}

extern "system" fn no_op_window_callback( /* callback args */ ) {
    0
}

I'll have to look closer at sublcass_input's lifetime before I feel confident in a solution, tough, as I'm not too familiar with the assumptions that the event_loop module makes internally.

[msiglreith]

IMO it's fine to move forward with this change - more sophisticated cleanup can be done as followup as well. This would include removing the subclass, which is supposed to be done in WM_NCDESTROY anyway due to reverse ordering.