Mutable closures (FnMut) in js! macros are unsound

[michiel-de-muynck]

The stdweb runtime handles regular closures (that only implement Fn) and mutable closures (that implement FnMut) the same way. The wrong way, for FnMut. It doesn't check that you're not calling a FnMut closure while a call to that same FnMut closure is already going on. Essentially, it allows multiple mutable references to the state of the closure (and all variables in it) to exist at the same time.

Here's an example demonstrating the bug:

#[macro_use]
extern crate stdweb;

use std::sync::atomic::{fence, Ordering};

fn main() {
	// this int gets moved into the closure f below
	let mut int = 0;

	let f = move |operation: String, callback: ::stdweb::Value| {
		if operation == "print_and_call_back" {
			// This &mut i32 is valid for the whole scope, nothing re-borrows it.
			// If we don't modify the i32 it points to, its value should
			// stay the same as long as we borrow it.
			let mut_ref : &mut i32 = &mut int;
			
			console!(log, "Before callback: ", *mut_ref); // prints 0 on my machine
			
			// Without this line, the compiler actually relies on this fact that
			// the i32 pointed to shouldn't be able to change
			// and it prints 0 both times.
			// With this line, it could still do that (it could do whatever it wants,
			// it's undefined behavior after all), but at least on my machine
			// it makes it print 1 after the callback.
			fence(Ordering::SeqCst);

			// callback is just a Value, and doesn't borrow anything in this scope.
			js! {
				@{callback}();
			}
			
			console!(log, "After callback: ", *mut_ref); // prints 1 on my machine
		} else if operation == "increment" {
			int += 1;
		}
	};
	
	js! {
		let f = @{f};
		
		f("print_and_call_back", function () {
			f("increment", null);
		});
	
		f.drop();
	}
}

Since JavaScript is mostly single-threaded and you can only use closures that are valid for the 'static lifetime inside the js! macro, this is unlikely to cause any real-world bugs, but still it's unsound and should be fixed.

If I'm not mistaken, the js! macro can't actually distinguish between Fns and FnMuts right now, at least not without a modifier like Once. And I don't think we want to prohibit all closures from being called recursively like in the code above (for Fns, such recursion is fine). So one possible solution would be to make a Mut modifier similar to the Once one, and if you want to use a FnMut in a js! macro you need to wrap it in that Mut modifier that would panic on illegal recursion.

Alternatively, we could not bother with that Mut modifier, and only allow Fn and FnOnce closures (with the Once modifier) to be used in the js! macro. Since closures used in js! need to be 'static, the benefit of FnMut over Fn is very small, and in the cases where it's still needed, I think a Fn with a RefCell as state can do everything that FnMut can.

I can implement either of these solutions if you want me to. I have a few more ideas for handling closures (generic function handles to help with the memory leak issue), but this is probably not the right place to discuss those.

EDIT: while implementing a fix for this, I noticed that IEventTarget::add_event_listener and MutationObserver::new both accept FnMut+'static closures as arguments. A decision will have to be made for these as well, to either

• Change these to accept only Fn+'static closures, and force any users of the function that want mutation in the state of their closure to use RefCell or similar.

• Still accept FnMut+'static closures, and change the implementation to use RefCell or the proposed Mut modifier to panic on illegal recursion. These functions will be a tiny bit slower as a result of the added overhead.

• Make 2 versions of the functions, one for Fns and one for FnMuts. I don't think this is worth it.

[koute]

Good catch!

Hmmmm.... quiet frankly I'm not sure which option would actually be better in practice. I think I prefer the idea of completely disallowing FnMut as that's something that will rear its head at compile time as opposed to surprising the user at runtime.

Perhaps the ideal way to deal with this would be to disallow FnMut by default but have a wrapper like Once (Mut?) which would allow FnMuts but would disallow recursion (so it'd be opt-in). I wonder if that's actually possible?

Also, yeah, whatever we do here this will definitely be a significant breaking change which will warrant a major version bump.

[michiel-de-muynck]

Fixed by #279 except for the part that disables FnMuts in js! (as that's a breaking change), so I guess I leave this open until version 0.5.0 is released.