Event handler crash

[Kolsky]

MRE:

use native_windows_gui as nwg;
use std::cell::Cell;

#[derive(Default)]
struct BasicApp {
    window: nwg::Window,
    flex: nwg::FlexboxLayout,
}

impl Drop for BasicApp {
    fn drop(&mut self) {
        ()
    }
}

struct EventHandler(nwg::EventHandler);

impl nwg::NativeUi<EventHandler> for BasicApp {
    fn build_ui(mut data: Self) -> Result<EventHandler, nwg::NwgError> {
        nwg::Window::builder().build(&mut data.window)?;

        nwg::FlexboxLayout::builder()
            .parent(&data.window)
            .build(&mut data.flex)?;

        let hwnd = data.window.handle;
        let cellopt_data = Cell::new(Some(data));

        Ok(EventHandler(nwg::full_bind_event_handler(
            &hwnd,
            move |evt, _, _| {
                let data = match cellopt_data.take() {
                    Some(x) => x,
                    None => return,
                };
                if evt == nwg::Event::OnWindowClose {
                    nwg::stop_thread_dispatch();
                }
                cellopt_data.set(Some(data));
            },
        )))
    }
}

impl Drop for EventHandler {
    fn drop(&mut self) {
        nwg::unbind_event_handler(&self.0);
    }
}

fn main() {
    nwg::init().expect("Failed to init Native Windows GUI");
    let _ui = nwg::NativeUi::build_ui(BasicApp::default()).expect("Failed to build UI");
    nwg::dispatch_thread_events();
}

Stack trace

nwg_bug.exe!core::cell::Cell::get() Line 443 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\cell.rs:443) nwg_bug.exe!alloc::rc::RcInnerPtr::strong>>(alloc::rc::RcBox> * self) Line 2538 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\alloc\src\rc.rs:2538) nwg_bug.exe!alloc::rc::RcInnerPtr::dec_strong>>(alloc::rc::RcBox> * self) Line 2566 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\alloc\src\rc.rs:2566) nwg_bug.exe!alloc::rc::impl$20::drop>(alloc::rc::Rc> * self) Line 1521 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\alloc\src\rc.rs:1521) nwg_bug.exe!core::ptr::drop_in_place>>(alloc::rc::Rc> *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place(native_windows_gui::layouts::flexbox_layout::FlexboxLayout *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place(nwg_bug::BasicApp *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place, 0, 7, Some>>(enum$, 0, 7, Some> *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place, 0, 7, Some>>>(core::cell::UnsafeCell, 0, 7, Some>> *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place, 0, 7, Some>>>(core::cell::Cell, 0, 7, Some>> *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place(nwg_bug::impl$1::build_ui::closure_env$0 *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!core::ptr::drop_in_place,enum$,enum$>,assoc$>>>>(ptr_mut$,enum$,enum$>,assoc$>>>>) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!alloc::rc::impl$20::drop,enum$,enum$>,assoc$>>>>(alloc::rc::Rc,enum$,enum$>,assoc$>>>> * self) Line 1527 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\alloc\src\rc.rs:1527) nwg_bug.exe!core::ptr::drop_in_place,enum$,enum$>,assoc$>>>>>(alloc::rc::Rc,enum$,enum$>,assoc$>>>> *) Line 486 (c:\rustc\a8314ef7d0ec7b75c336af2c9857bfaf43002bfc\library\core\src\ptr\mod.rs:486) nwg_bug.exe!native_windows_gui::win32::window::process_events(enum$ * hwnd, unsigned int msg, unsigned __int64 w, __int64 l, unsigned __int64 id, unsigned __int64 data) Line 727 (c:\Users\Kolsky\.cargo\git\checkouts\native-windows-gui-b74f684ad4534f77\2c6cf6e\native-windows-gui\src\win32\window.rs:727) comctl32.dll!00007ffa85b8d3b6() (Unknown Source:0) comctl32.dll!00007ffa85b8d26c() (Unknown Source:0) user32.dll!00007ffa9a5be858() (Unknown Source:0) user32.dll!00007ffa9a5be3dc() (Unknown Source:0) user32.dll!00007ffa9a5d0bc3() (Unknown Source:0)

Introduced by: #256 (rev. 2c6cf6e)

Definitely not a solution as it stands currently, maybe it should have increased strong count from the get-go which would then be decremented only after for loop inside unbind_event_handler completes (just as an example).

[Kolsky]

Although it would make sense to clone Rc inside callback itself (for the entire duration of a call) to make sure it holds itself in memory, which is hilarious but sounds just a bit scary (drop is still happening inside of it and a reference to environment dangles)

Edit: yup, it's not okay at all by a more mundane reason, even. playground (check Miri output). Then if there's no guarantee if callback is still in use at the time it's unbound, there's not much to do other than:

1. Change in approach, reworking API in a way that would account for this problem (no thoughts yet).

2. Add a flag indicating the status of callback, if it's running and unbind is requested, then panic or leak. Set the flag before calling and clear after.

3. Does registration happen to accept function instead of callback? If so, cloning becomes viable since function is an external entity. Panic on full_bind if callback is accessed via static variable and occupied. Would be easier if WindowSubclass allows to store additional data itself.

[gabdube]

Oh no... I'll take the time to look into this this week.

[wtodd1]

Possible fix in #258. Not sure if it makes it completely safe, but it at least seems to fix the crash.

If I understand the issue correctly, when the closure is dropped, it drops BasicApp, which drops the window handle, which destroys the window, which sends WM_DESTROY to the window, causing process_events to be called. Since this happens during the callback drop, the reference count is already 0, so it crashes. I made it remove the subclass before dropping the callback, and that seems to have fixed the problem.

As for @Kolsky's playground example, I don't think it matches nwg exactly. In the example, the Rc is incremented/decremented inside the closure. In nwg, it is incremented/decremented in process_events, which calls the closure. When I change

(&*FOO)();

to

Arc::increment_strong_count(FOO);
(&*FOO)();
Arc::decrement_strong_count(FOO);

which I believe is closer to how nwg does it, Miri says it's ok.

[Kolsky]

Can confirm it doesn't crash with #258. If this is the only path to call process_events (no background threads, no issues with event signaling after RemoveWindowSubclass), then I don't see any other problems. Maybe Window should be non_exhaustive (to privatize constructor) or with private fields, but it's outside the scope of the current issue.