A safe-ish way to deal with window proc reentrancy at compile time

[Vlad-Shcherbina]

Maybe this could be added to this crate. But even if it's out of scope, I thought it's kind of interesting and wanted to share.

Problem

Since window_proc() function of win_win::WindowProc trait takes &self, mutable state has to be wrapped in RefCell<>.

In window_proc() we use .borrow_mut() to access it.

However, if we forget to release this borrow when calling Windows API functions that cause window proc reentrancy (e.g. SendMessage() or MessageBox()), second invocation of window_proc() will fail with a runtime error "already borrowed".

This failure to account for reentrancy could be easily missed by manual testing if .borrow_mut() only happens on some paths in window_proc().

It would be nice to have some kind of compile-time enforcement.

Solution

Instead of passing RefCell<State> to window_proc(), we will pass some opaque type StateRef<State> with the following public interface:

impl StateRef<State> {
    pub fn state_mut(&mut self) -> impl DerefMut<Target=State>;
    pub fn reent<'a>(&'a mut self) -> Reent<'a>;
}

Reent is a zero-sized type that proves that we are not holding mut borrow of State at the moment.

Windows API calls that are known to cause reentrancy should have safe wrappers that take a Reent argument:

pub fn message_box(
    _reent: Reent,
    hwnd: HWND,
    title: &str,
    message: &str,
    u_type: UINT,
) -> c_int {
    unsafe {
        MessageBoxW(
            hwnd,
            message.to_wide_null().as_ptr(),
            title.to_wide_null().as_ptr(),
            u_type)
    }
}

Usage example:

fn window_proc(
    sr: &mut StateRef<State>,
    hwnd: HWND, msg: UINT, _wparam: WPARAM, _lparam: LPARAM,
)-> Option<LRESULT> {
    let mut state = sr.state_mut();
    // do stuff with state

    if msg == WM_LBUTTONDOWN {
        drop(state);  // Release state borrow.
                      // Borrow checker will compain if we forget to do it.

        message_box(sr.reent(), hwnd, "hello", "title", MB_OK);

        let mut state = sr.state_mut();  // borrow it again if needed
        // do stuff
    }
    None
}

---

Complete working example: https://github.com/Vlad-Shcherbina/win-win-reent-demo

[raphlinus]

That's quite fascinating. I do see the argument and think it is sound. I'm not sure it can readily be adapted to something like druid, where I think it would be quite difficult to drop the borrow to the window state from, eg, inside a widget handling method. Another major concern is that you have to catch all entry points that can cause reentrancy, while the RefCell approach does guarantee coverage of all cases where you get reentrancy.

I'll point to https://github.com/linebender/druid/issues/1068 as an example that we're still running into serious reentrancy issues.

Thanks for sharing this! If other people would find it useful, I can see adding it. (If it's a parametrized type, possibly it can be done in a way that would cause minimal impact if not actually used).

[Vlad-Shcherbina]

you have to catch all entry points that can cause reentrancy, while the RefCell approach does guarantee coverage of all cases where you get reentrancy

My approach is an additional layer on top of the runtime protection RefCell provides. I'm not suggesting to replace RefCell with UnsafeCell or anything (that would indeed require exhaustively annotating all winapi calls that might cause reentrancy).

The impl DerefMut<Target=State> that .state_mut() returns is actually std::cell::RefMut<State>.

If you forget to annotate message_box() as taking Reent, the compiler will allow you to call it without releasing state borrow, but it will panic because of the RefCell check.