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

Incorrect handling of xdg-shell events resulting in zed crashing on wayfire (wayland compositor on linux). #10976

Closed
1 task done
marcusbritanicus opened this issue Apr 25, 2024 · 7 comments · Fixed by #13243
Labels
bug [core label] linux panic / crash [core label]

Comments

@marcusbritanicus
Copy link

marcusbritanicus commented Apr 25, 2024

Check for existing issues

  • Completed

Describe the bug / provide steps to reproduce it

An attempt at starting Zed on wayfire gives the following error:

xdg_surface@26: error 3: xdg_surface has never been configured 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object xdg_surface@26: 
Protocol error 3 on object^C⏎  

Running WAYLAND_DEBUG=1 ./Zed give the following log:
wayland-debug-zed.log

From Lines 564 to 571, one can see that upon receiving a configure event, a commit() is performed first and then the configure is acked(). This is a violation of the protocol. Relevant section of the protocol:

	When a configure event is received, if a client commits the
	surface in response to the configure event, then the client
	must make an ack_configure request sometime before the commit
	request, passing along the serial of the configure event.

The protocol requires that the client ack the event first and then commit.

Secondly, here, an attempt is made to resize the surface in xdg_toplevel.configure event handler.
This is a protocol violation again. Revelant section of the protocol:

    <event name="configure">
      <description summary="suggest a surface change">
	This configure event asks the client to resize its toplevel surface or
	to change its state. The configured state should not be applied
	immediately. See xdg_surface.configure for details.

I had a short discussion regarding this with @ammen99 (Wayfire Developer), and he suggested that the new state needs to be remembered, and should not be set until the next xdg_surface.configure event.

Environment

OS: Arch Linux (updated)
DE: Wayfire Wayland Compositor
Kernel: Linux 6.8.7-artix1-1 #1 SMP PREEMPT_DYNAMIC

Please feel free to ask for any further tests and information regarding this.

If applicable, add mockups / screenshots to help explain present your vision of the feature

No response

If applicable, attach your ~/Library/Logs/Zed/Zed.log file to this issue.

No response

@marcusbritanicus marcusbritanicus added admin read Pending admin review bug [core label] triage Maintainer needs to classify the issue labels Apr 25, 2024
@JosephTLyons JosephTLyons added panic / crash [core label] linux and removed triage Maintainer needs to classify the issue admin read Pending admin review labels Apr 25, 2024
@apricotbucket28
Copy link
Contributor

apricotbucket28 commented Apr 27, 2024

Hello @marcusbritanicus! #11081 should fix the first issue. I don't really fully understand the second issue, but there's something clearly wrong since resize is super slow on some compositors (like KWin or Mutter) and really fast on others (like Sway).

@mikayla-maki
Copy link
Contributor

For the second issue, there are two events called 'Configure', one in xdg_toplevel, which should be buffered, and one in xdg_surface, which should clear that buffer. It's a change I meant to apply during the refactor but cut from scope.

@marcusbritanicus
Copy link
Author

@apricotbucket28 Thank you. I will test it sometime today or tomorrow.

@marcusbritanicus
Copy link
Author

Hi. Sorry for the late reply. Although I was able to test #11081, I was unable to reply here. I still get the same output/crash. It seems that wl_surface.commit() is called before wl_surface.ack_configure() somewhere. To be clear, this happens when the windows is first shown. If you want, I can provide a fresh log with WAYLAND_DEBUG=1, but it's mostly identical to original one attached.

@apricotbucket28
Copy link
Contributor

apricotbucket28 commented May 1, 2024

Hello! A single wl_surface.commit() before acknowledging is expected, or else the program will get stuck. Could you share the new log?

@marcusbritanicus
Copy link
Author

marcusbritanicus commented May 1, 2024

Hello! A single wl_surface.commit() before acknowledging is expected, or else the program will get stuck. Could you share the new log?

@apricotbucket28 Please cancel my previous comment. I may have made a mistake in checking out the PR. I directly cloned your branch this time to be sure. I can confirm that there is no crash.

@marcusbritanicus
Copy link
Author

I would also like to add that resizing the window is also very smooth on wayfire. I might add that if I manage to set up the key-bindings the way I have in pulsar, I would be very happy to switch to full-time. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug [core label] linux panic / crash [core label]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants