Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash in unwrap() on MaximizedState #1008

Closed
ids1024 opened this issue Nov 23, 2024 · 2 comments · Fixed by #1060
Closed

Crash in unwrap() on MaximizedState #1008

ids1024 opened this issue Nov 23, 2024 · 2 comments · Fixed by #1060

Comments

@ids1024
Copy link
Member

ids1024 commented Nov 23, 2024

I've been using emacs and Firefox stacked in one workspace, and have been having an issue lately where setting that up causes the compositor to crash.

Nov 22 16:44:48 Superficies cosmic-comp[764794]: thread 'main' panicked at 'called `Option::unwrap()` on a `None` value': src/shell/workspace.rs:988

I've been able to reproduce it consistently by following certain steps, and looking at WAYLAND_DEBUG logs it seems to be related to set_maximized (I guess something about my Emacs configuration is causing it to call that), which I was able to reproduce in a modified sctk example. But I have a simpler reproduction:

  • Super + T
  • Super + M
  • Super + S
  • Super + Y
  • Super + Y

(If there's a magic sequence of 5 key combos to crash the compositor, does that count as a cheat code?)

I guess maximized_state needs to be kept in sync with the maximize status of the Window, or an unwrap like this will panic. Looking at the code I'm not sure whats wrong, but at least it's easy to reproduce.

@ids1024
Copy link
Member Author

ids1024 commented Dec 11, 2024

Okay, I think I (mostly) see what's going on now. After more testing.

  • The window is opened and mapped into the tiling layer
  • Shell::maximize_request() adds it to the floating layer with map_maxmized, but doesn't unmap it from the tiling layer
    • Given the comment elsewhere in the code should still be mapped in tiling, this seems to be intentional?
  • FloatingLayout::toggle_stacking() calls convert_to_stack to make the CosmicMapped a stack
    • But it is still mapped into the stacking layer as a window, not a stack
  • Workspace::set_tiling moves the stack into the floating layer
    • So now the tiling layer has two CosmicMapped, that are maximized, one of which is a stack and one of which is the window (that is also part of the stack)
  • Workspace::set_tiling now tries to move both elements back to the tiling layer
    • This works for the stack, but fails for the window mapped element with the above unwrap panic

I'm not sure exactly why the maximized_state of the window CosmicMapped is None here, but presumably having both a window and a stack containing that window mapped into a layout at the same time is not correct.

@Drakulix
Copy link
Member

Drakulix commented Dec 13, 2024

  • Shell::maximize_request() adds it to the floating layer with map_maxmized, but doesn't unmap it from the tiling layer
  • Given the comment elsewhere in the code should still be mapped in tiling, this seems to be intentional?

Yes it is. This is the only case where we ever map a window twice.

  • FloatingLayout::toggle_stacking() calls convert_to_stack to make the CosmicMapped a stack

Doesn't actually happen, but rather we enumerate the tiling layer first in toggle_stacking_focused, so it ends up calling TilingLayout::toggle_stacking(), but that essentially does the same thing.

The problem with convert_to_stack is, that it converts the CosmicMapped in place. Almost everything in there is reference-counted, except the CosmicWindowInternal, which this function changes. So we end up with two de-synced CosmicMapped-elements that share the same MaximizedState.

Thus this function should never be called when a double-map is in place. I modified toggle_stacking_focused for this reason to first unmaximize and then (potentially) re-maximize the window: #1060

Seems to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants