perform_action of CloseCurrentTab can crash wezterm in certain sequences/timings

neogeographica commented 2 months ago

What Operating System(s) are you seeing this problem on?

Linux X11, macOS

Which Wayland compositor or X11 Window manager(s) are you using?

No response

WezTerm version

20240203-110809-5046fc22

Did you try the latest nightly build to see if the issue is better (or worse!) than your current version?

No, and I'll explain why below

Describe the bug

After using a new window to attach to an ssh domain that previously had no persisted windows/panes/tabs, immediately invoking CloseCurrentTab on a tab of that window can reliably cause wezterm to crash.

To Reproduce

I won't go too deeply into why I'm trying this, but broadly speaking:

I have some Lua code in my config that can be triggered for attaching to a domain. The behavior I want is for the domain's existing windows/tabs/panes to open in new windows, not putting any of them in an existing window. (And if there is no existing pane in the domain, just start a new one in the tab of a new window.)

It seems like the only ways to do this involve creating a new window which may start with a tab, then attaching the desired domain there, and then closing that original tab. Which is fine, but it appears that if I try to close that tab too quickly then wezterm may crash. There's a particular situation where it crashes extremely reliably. This happens both on Linux and macOS.

My entire wezterm config is at https://gist.github.com/neogeographica/ee94d8510e7623ca7f8d5473e59e5a4d (note: now updated as per comments below; original code at https://gist.github.com/neogeographica/ee94d8510e7623ca7f8d5473e59e5a4d/29f16fb29922769bd4c6ce628a70aa288da218fe ) with the relevant code specifically in domains.lua there. But the crash can be reproduced with one of two small code snippets below that were extracted/simplified from that config code.

The snippets are a bit artificial as a result! The code in domains.lua is the complete (and commented) stuff for doing what I actually need to do. These snippets are just for easily exercising the crash case.

Both of these snippets will crash (on both Linux and macOS) in this situation:

mux_domain is the name of an ssh domain
that domain is not currently attached
that domain currently does not have any persisted windows/tabs/panes

The way I exercise this test scenario is:

Attach to that domain (in any non-crashy way).
Use "exit" to quit out of all current panes in the domain.
Quit and restart wezterm to make sure the domain is detached + clean slate in general.
Use the wezterm debug console to run one of the code snippets.

The crash will happen at the CloseCurrentTab action.

But if I (for example) add a wezterm.sleep_ms(2000) before I try to do anything with the new window's tabs, then it won't crash.

Snippet 1:

mux_domain = 'some-ssh-domain-name'
new_tab, new_pane, new_mux_window = wezterm.mux.spawn_window {}
new_window = new_mux_window:gui_window()
new_window:perform_action(
  wezterm.action.AttachDomain(mux_domain),
  new_pane
)
new_tab:activate()
new_pane:activate()
new_window:perform_action(
  wezterm.action.CloseCurrentTab { confirm = false },
  new_pane
)

Snippet 2:

mux_domain = 'some-ssh-domain-name'
new_tab, new_pane, new_mux_window = wezterm.mux.spawn_window {
  domain = { DomainName = mux_domain },
}
all_tabs = new_mux_window:tabs()
if #all_tabs > 1 then
  new_window = new_mux_window:gui_window()
  new_tab:activate()
  new_pane:activate()
  new_window:perform_action(
    wezterm.action.CloseCurrentTab { confirm = false },
    new_pane
  )
end

Another interesting and possibly related quirk observed in the second code snippet above: when exercising this particular test case, #all_tabs has an unexpected-to-me value of 2. But if the 2-second sleep is added before all_tabs = new_mux_window:tabs(), then #all_tabs has a value of 1 as expected (which means in that snippet it won't even attempt to do the tab close).

So both the action of closing a tab and fetching the window's tabs list behave differently if a preceding sleep is added.

I've done some extensive log-dumping of the new_tab, new_pane, new_mux_window, and new_window object attributes and I don't see anything invalid about them, but obviously some race is happening.

So, ideally one of the following would be true:

I could run the above code and get the expected behavior (and no crash).
Or, there would be some condition I could check to see when it is "safe" to call CloseCurrentTab.
Or... there's some other way to do what I want?

But I'm also more or less OK with just adding in the 2-second sleep. Since this is a crash scenario though, it seemed like I should report it.

BTW I haven't tried this on a nightly build because at the moment I can't change the wezterm version running on the remote server... I don't want to introduce possible confusion of the issue from a version mismatch.

Configuration

see above for config details

Expected Behavior

The tab specified by CloseCurrentTab would be closed and wezterm would not crash.

Logs

macOS crash report when running snippet 1: wezterm-crash-snippet-1.txt macOS crash report when running snippet 2: wezterm-crash-snippet-2.txt

Anything else?

No response

loops commented 2 months ago

Hey neogeographica,

You provided a lot of great detail, and hopefully the crash can be removed. But until then, you might want to take a look at:

https://wezfurlong.org/wezterm/config/lua/MuxDomain/attach.html

Since using something like:

  wezterm.mux.get_domain(mux_domain):attach()

Should avoid the need to close a spurious pane.

neogeographica commented 2 months ago

Thanks, I'll give it a spin.

edit: Seems promising, except in the no-current-panes situation you don't end up with any window at all. And if the domain is not attached yet I can't detect that situation until after attaching. I guess I could subsequently do a pass over all panes to see if any domain panes were created... looks like attach() is synchronous so it should be OK to check that after it returns.

edit 2: Yep I think that works & is an improvement, especially since no-current-panes is the rarest case. Updated domains.lua in that linked config gist if anyone's curious. Thanks again!

wez / wezterm