Panic with file open dialog and ExtEventSink

[timothyhollabaugh]

If another thread sends a command with an ExtEventSink while the file open dialog is open, Druid panics. It comes from the run_idle function in window.rs trying to mutably borrow the WindowState handler. I'm guessing that this is because the handler is already borrowed while blocking on the file open dialog.

I'm on Arch Linux with the GTK backend, but I have no easy way to test Windows or Mac.

Here is a minimal example:

    let main_window = WindowDesc::new(|| {
        Button::new("Open File").on_click(|ctx: &mut EventCtx, _data: &mut u8, _env: &Env| {
            ctx.submit_command(
                Command::new(druid::commands::SHOW_OPEN_PANEL, FileDialogOptions::new()),
                ctx.window_id(),
            )
        })
    });

    let app = AppLauncher::with_window(main_window);

    let app_handle = app.get_external_handle();

    let _thread = thread::spawn(move || {
        thread::sleep(Duration::from_secs(10));
        println!("Sending Command");
        app_handle
            .submit_command(Selector::<()>::new("Test"), Box::new(()), None)
            .expect("Failed to send command");
    });

    app.launch(0u8).expect("Failed to launch window");

Click the button as soon as the window opens, then wait for the thread to sumbit the command.

The output:

     Running `target/debug/pycut-rs`
Sending Command
thread 'main' panicked at 'already borrowed: BorrowMutError', /home/tim/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/cell.rs:878:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1069
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1504
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:198
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:218
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:511
  11: rust_begin_unwind
             at src/libstd/panicking.rs:419
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:111
  13: core::option::expect_none_failed
             at src/libcore/option.rs:1268
  14: core::result::Result<T,E>::expect
             at /home/tim/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/result.rs:963
  15: core::cell::RefCell<T>::borrow_mut
             at /home/tim/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/cell.rs:878
  16: druid_shell::platform::gtk::window::run_idle
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/window.rs:745
  17: druid_shell::platform::gtk::window::IdleHandle::add_idle_token::{{closure}}
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/window.rs:735
  18: glib::source::trampoline
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/glib-0.9.3/src/source.rs:123
  19: g_main_context_dispatch
  20: <unknown>
  21: g_main_loop_run
  22: gtk_native_dialog_run
  23: <O as gtk::auto::native_dialog::NativeDialogExt>::run
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/gtk-0.8.1/src/auto/native_dialog.rs:152
  24: druid_shell::platform::gtk::dialog::get_file_dialog_path
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/dialog.rs:88
  25: druid_shell::platform::gtk::window::WindowHandle::file_dialog
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/window.rs:696
  26: druid_shell::platform::gtk::window::WindowHandle::open_file_sync
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/window.rs:619
  27: druid_shell::window::WindowHandle::open_file_sync
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/window.rs:181
  28: druid::win_handler::AppState<T>::show_open_panel::{{closure}}
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/win_handler.rs:580
  29: core::option::Option<T>::and_then
             at /home/tim/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/option.rs:670
  30: druid::win_handler::AppState<T>::show_open_panel
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/win_handler.rs:580
  31: druid::win_handler::AppState<T>::handle_cmd
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/win_handler.rs:553
  32: druid::win_handler::AppState<T>::process_commands
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/win_handler.rs:503
  33: druid::win_handler::AppState<T>::do_window_event
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/win_handler.rs:475
  34: <druid::win_handler::DruidHandler<T> as druid_shell::window::WinHandler>::mouse_up
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/win_handler.rs:684
  35: druid_shell::platform::gtk::window::WindowBuilder::build::{{closure}}
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/window.rs:343
  36: <O as gtk::auto::widget::WidgetExt>::connect_button_release_event::button_release_event_trampoline
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/gtk-0.8.1/src/auto/widget.rs:3096
  37: <unknown>
  38: g_closure_invoke
  39: <unknown>
  40: g_signal_emit_valist
  41: g_signal_emit
  42: <unknown>
  43: <unknown>
  44: gtk_main_do_event
  45: <unknown>
  46: <unknown>
  47: g_main_context_dispatch
  48: <unknown>
  49: g_main_context_iteration
  50: g_application_run
  51: <O as gio::application::ApplicationExtManual>::run
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/gio-0.8.1/src/application.rs:23
  52: druid_shell::platform::gtk::application::Application::run
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/platform/gtk/application.rs:65
  53: druid_shell::application::Application::run
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-shell-0.6.0/src/application.rs:153
  54: druid::app::AppLauncher<T>::launch
             at /home/tim/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/druid-0.6.0/src/app.rs:142
  55: pycut_rs::main
             at src/main.rs:258
  56: std::rt::lang_start::{{closure}}
             at /home/tim/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67
  57: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  58: std::panicking::try::do_call
             at src/libstd/panicking.rs:331
  59: std::panicking::try
             at src/libstd/panicking.rs:274
  60: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  61: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  62: std::rt::lang_start
             at /home/tim/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67
  63: main
  64: __libc_start_main
  65: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Process finished with exit code 101

[jneem]

The underlying issue is that druid-shell doesn't like to be called re-entrantly. There's a quick fix (which I'll try to write tomorrow), in which we use try_borrow_mut instead of borrow_mut and ignore the command if there's already a borrow.

But I also think it's worth discussing (and documenting!) in more detail:

• under what circumstances do we expect druid to make a "blocking" call that hands control back to the system's event loop? The obvious ones are open/save and maybe other modal dialogs, but I also remember context menus being an issue at some point (context menu pop up causes crash #735)
• In each of those circumstances, what is the right thing to do with an event that would normally require druid-shell to call back into the WinHandler? In the case of save dialogs, it might make sense to drop them, but maybe it would also make sense to deliver them after the dialog is closed?

[xStrom]

I'm pretty sure some of the file dialog code takes mutable borrows needlessly when regular borrows would work. I have something like that in my notes. Not sure if this is strictly related to this specific crash here.

I skimmed the code but during the minutes I spent I couldn't really grasp where the initial handler borrow takes place which causes the crash in run_idle. 🤔

More generally I'm not sure that file dialogs should be blocking at all. This seems to cause a ton of issues with very minor gains.

[jneem]

The original borrow is taken for druid-shell to call mouse_up on the handler. Druid is opening the dialog synchronously in response to that mouse click.

I had some thoughts about avoiding blocking calls to create dialogs, but I had given up on them as being too complex. Let me open up another issue to discuss the problem.