[Bug: Monitor] Unwinding into C is undefined behavior

[abonander]

Currently if we panic in callbacks or in the FFI glue generated for each callback impl, that can cause undefined behaviour as the stack will unwind into C code. It doesn't look like we have a problem right now as panicking works just fine, but once thread::catch_panic() is stabilized (planned for 1.1 stable), we should probably try to catch panics in the FFI glue and safely exit the IUP event loop before re-panicking (or should we prefer to abort?).

[Andi-K]

If you can easily avoid a panic you should do it.
I think show_gui() should just exit the loop and return Err().

[abonander]

Well it's not really advisable to catch a panic in a thread and continue like nothing happened. Panicking can leave various structures in inconsistent states, and since RefCell doesn't poison on-panic (it was deemed unnecessary since a panic during a RefCell borrow should make it inaccessible), it's best if we re-panic after exiting the IUP event loop.

On the panic_safety branch I just pushed, I set up a guard in the callback FFI glue that re-panics if it's dropped during a panic. In current Rust, causing another panic during a panic will initiate a process abort. Until thread::catch_panic() stabilizes and I've had some time to work on a more conservative error-handling scheme, this is probably the best course of action to avoid undefined behaviour.

On that note, I'm also working on reducing the risk of panicking during the IUP event loop altogether; there are some edge cases where a RefCell re-borrow would have caused a panic and I'm working on solving them, namely these two:

• replacing the callback for a widget while the existing callback is executing
• trying to set a callback for the same action and the same widget type as the one currently executing

This is because the RefCell<HashMap<*mut Ihandle, Box<Callback<T>>> map would be mutably borrowed for the duration of a callback. So now, we just store a raw trait object pointer instead: RefCell<HashMap<*mut Ihandle, *mut Callback<T>>. This way, the map only has to be borrowed to copy out the pointer or set one in, and can be released before the callback itself is invoked.

To avoid leaking memory, callback boxes are pushed to a vector in the KISSContext struct. I hacked a solution around transforming all callback boxes to a single trait object type; previously I was moving them into a no-op FnOnce closure and boxing that but that seemed horribly inefficient.