[Windows] Improve handling of window destruction #1798

maroider · 2020-12-11T11:10:37Z

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

This is a follow-up to #1780 and #1746.

chrisduerr · 2020-12-11T21:00:49Z

src/platform_impl/windows/event_loop.rs

            drop(subclass_input);
-            Box::from_raw(subclass_input_ptr as *mut SubclassInput<T>);
+            remove_window_subclass(
+                window,
+                Box::from_raw(subclass_input_ptr as *mut SubclassInput<T>),
+            );


Dropping subclass input first and then trying to do something with a pointer to it seems strange to me.

The subclass_input that's dropped has a type of &SubclassInput<T> (introduced here). Dropping it is necessary to avoid UB.

hmm I don't believe drop will do provide anything in this case as references are copy types, but I'm not sure about the subtile aspects of lifetimes in this case. It's even a bit trickier as we are in a callback executed on the runner inside the subclass input. Explicit calls to Box::from_raw and Box::into_raw should lead to a safer approach in this case without relying on the dereference+borrow from outside the callback.

references are copy types

Ah, I had completely forgotten, but that then makes me wonder why it was ever there in the first place.

It's even a bit trickier as we are in a callback executed on the runner inside the subclass input

Yeah, I'm kind of at a loss here. The EventLoopRunner<T> needs to be kept alive until the catch_unwind method returns, but the Rc we have to it is dropped before we return. ~~This is admittedly worse in thread_event_target_callback, since that function's subclass_input is seemlingly the last thing that will hold on to an Rc to the EventLoopRunner<T>.~~

EDIT: Actually, it may be fine since thread_event_target_callback clones the Rc before calling catch_unwind and returning. Windows which use public_window_callback seem to then rely on thread_event_target_callback to not drop its Rc to the EventLoopRunner<T> before the windows get destroyed.

I've dug into the code some more, and it seems like there isn't anything guaranteeing that the event target window gets destroyed after all other windows. This means that catch_unwind will try to access the contents of the EventLoopRunner<T> after its destructor has been run. Fixing this would require temporarily cloning and holding onto the Rc until we don't need it.
The slightly more ambiguous issue is to do with pointer aliasing. It feels like having a & reference to something while also having a *mut pointer to it is insta-UB, but I'll have to consult with someone more knowledgeable on this before reaching a conclusion.

The slightly more ambiguous issue is to do with pointer aliasing. It feels like having a & reference to something while also having a *mut pointer to it is insta-UB, but I'll have to consult with someone more knowledgeable on this before reaching a conclusion.

I talked with someone on the Rust community discord, and it seems like this is indeed UB.

chrisduerr · 2020-12-11T21:03:14Z

src/platform_impl/windows/event_loop.rs

+            WINDOW_SUBCLASS_ID,
+        )
+    };
+    assert_eq!(removal_result, 1);


I'm not sure how I feel about these assertions. I feel like the most common way for this to fail is if the subclass is already gone and I'm not sure if we should tear down the entire application in that case.

With applications running multiple windows this could lead to a crash of the entire application, assuming this subclass info is added separately for each window (I'm not sure about that?).

The subclass is registered individually for every window, so I don't see how it could be "already gone" when event_loop is the only module with enough information (the callback function pointer and subclass ID) to remove it.

Additionally, it seems like the only "always safe" place to deregister subclasses is during window destruction.

maroider · 2020-12-13T22:39:35Z

The code in question now relies on Box::from_raw and Box::into_raw rather than casting to references, as previously suggested.
Dropping subclass_input has been moved to the very end of the callback functions.

I realized as I was writing this that the potential panics due to the added asserts is no longer covered by catch_unwind. Admittedly, if a panic does occur, then that means that one of the assumptions we're making to maintain memory safety has been violated.

maroider · 2020-12-14T13:48:05Z

Panicky bits are now inside catch_unwind again.

msiglreith · 2020-12-15T18:53:24Z

src/platform_impl/windows/event_loop.rs

+        .catch_unwind(callback)
+        .unwrap_or(-1);
+    if ncdestroy {
+        remove_event_target_window_subclass(window, subclass_input);


I'm not really happy with that solution tbh - unfortunately the topic is quite complex to get right due to the recursive call of the window proc (e.g DestroyWindow will invoke the callback again for th NCDESTROY case etc) and this approach makes it harder to maintain imo.
To be fair I currently don't have a clear opinion on the best way either. I played a bit around with local dereferencing but it doesn't feel robust either: msiglreith@9fd2cee

Would returning (LRESULT, Option<Box<SubclassInput<T>>>) from the callback closure and calling Box::into_raw in the (_, Some(_)) case be better? WM_NCDESTROY would then get to take ownership of the box and deregister the subclass.

Or (assuming that I've understood the recursive nature of WndProc correctly): The subclass_input: SubclassInput<T> could be changed to be Rc<SubclassInput<T>> which always has one "fake" reference. At the beginning of each call to *_callback we'd deref the subclass_input_ptr and clone the Rc, which would ensure that we never drop the subclass_input before all calls to the callback are done. WM_NCDESTROY would then deregister the subclass and remove the "fake" reference, in that order.

maroider · 2020-12-19T17:18:55Z

public_window_callback now tracks how deep the recursion is, and only drops the subclass_input when we're certain that there is no recursion. thread_event_target_callback does not seem to get called recursively at the moment, so I've omitted tracking it for now. Tracking recursion in thread_event_target_callback may be advisable, but I'm not 100% certain of the necessity of it.

The window subclass removal happens immediately after matching WM_NCDESTROY, since doing it later causes it to unconditionally fail in public_window_callback.

msiglreith

Thanks!

chrisduerr suggested changes Dec 11, 2020

View reviewed changes

msiglreith self-requested a review December 11, 2020 21:52

msiglreith added the DS - windows label Dec 11, 2020

maroider force-pushed the remove-subclass-on-ncdestroy branch from 3e9681e to 3d00b63 Compare December 13, 2020 22:29

maroider force-pushed the remove-subclass-on-ncdestroy branch from 3d00b63 to ed32d57 Compare December 14, 2020 13:46

msiglreith reviewed Dec 15, 2020

View reviewed changes

maroider force-pushed the remove-subclass-on-ncdestroy branch 3 times, most recently from 8b89c94 to c44fb62 Compare December 19, 2020 17:17

maroider force-pushed the remove-subclass-on-ncdestroy branch from c44fb62 to 4c76594 Compare December 19, 2020 19:07

On Windows, improve handling of window destruction

4c76594

msiglreith approved these changes Dec 20, 2020

View reviewed changes

msiglreith merged commit c05952b into rust-windowing:master Dec 20, 2020

maroider deleted the remove-subclass-on-ncdestroy branch December 20, 2020 12:36

maroider mentioned this pull request May 13, 2021

Add support for WM_COMMAND messages in order to handle HMENU user input on Windows #1928

Open

maroider mentioned this pull request Jun 1, 2021

Glium Display Does Not Close After Drop, Windows glium/glium#1932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Improve handling of window destruction #1798

[Windows] Improve handling of window destruction #1798

maroider commented Dec 11, 2020

chrisduerr Dec 11, 2020

maroider Dec 11, 2020

msiglreith Dec 12, 2020

maroider Dec 12, 2020 •

edited

Loading

maroider Dec 13, 2020 •

edited

Loading

maroider Dec 19, 2020 •

edited

Loading

chrisduerr Dec 11, 2020

maroider Dec 11, 2020 •

edited

Loading

maroider Dec 13, 2020

maroider commented Dec 13, 2020

maroider commented Dec 14, 2020

msiglreith Dec 15, 2020

maroider Dec 15, 2020 •

edited

Loading

maroider Dec 15, 2020

maroider commented Dec 19, 2020

msiglreith left a comment

[Windows] Improve handling of window destruction #1798

[Windows] Improve handling of window destruction #1798

Conversation

maroider commented Dec 11, 2020

chrisduerr Dec 11, 2020

Choose a reason for hiding this comment

maroider Dec 11, 2020

Choose a reason for hiding this comment

msiglreith Dec 12, 2020

Choose a reason for hiding this comment

maroider Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

maroider Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

maroider Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

chrisduerr Dec 11, 2020

Choose a reason for hiding this comment

maroider Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

maroider Dec 13, 2020

Choose a reason for hiding this comment

maroider commented Dec 13, 2020

maroider commented Dec 14, 2020

msiglreith Dec 15, 2020

Choose a reason for hiding this comment

maroider Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

maroider Dec 15, 2020

Choose a reason for hiding this comment

maroider commented Dec 19, 2020

msiglreith left a comment

Choose a reason for hiding this comment

maroider Dec 12, 2020 •

edited

Loading

maroider Dec 13, 2020 •

edited

Loading

maroider Dec 19, 2020 •

edited

Loading

maroider Dec 11, 2020 •

edited

Loading

maroider Dec 15, 2020 •

edited

Loading