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 with large windows on x11 #168

Closed
tronical opened this issue Oct 26, 2023 · 14 comments · Fixed by #170
Closed

Crash with large windows on x11 #168

tronical opened this issue Oct 26, 2023 · 14 comments · Fixed by #170

Comments

@tronical
Copy link
Contributor

With softbuffer master branch at 18c9447 , on X11 a large buffer size causes a loss of the X connection.

How to reproduce:

  1. cargo run --example winit
  2. Make the window very large (or just maximise)

Observe:

XIO:  fatal IO error 9 (Bad file descriptor) on X server ":0"
      after 12025 requests (12023 known processed) with 0 events remaining.
@GnomedDev
Copy link

I am getting this error currently with a small demo app adapted from the existing softbuffer README example to work on latest winit, it doesn't even require resizing too large, it seems resizing quickly does it.

@reisnera
Copy link

reisnera commented Oct 29, 2023

Also having this same issue on 0.3.2 (in WSL2 Ubuntu).

@notgull
Copy link
Member

notgull commented Oct 29, 2023

The only change for X11 we made was migrating to POSIX SHM for shared memory.

Do these issues affect the commit a405e03?

@reisnera
Copy link

Nope! I tried again at that commit and it works without issue.

@reisnera
Copy link

I tried also on the master branch and actually it breaks again with the same error as above. (This is with Wayland-related features disabled on softbuffer and winit but with other default features specified.)

@notgull
Copy link
Member

notgull commented Oct 29, 2023

It's also especially odd that the error is coming from Xlib, since this crate exclusively uses libxcb. It almost feels like a bug in the shared communication channels used by Xlib and XCB.

A gdb backtrace of the error would be helpful. A short term solution would be reverting back to SysV SHM for now.

@reisnera
Copy link

So I was actually trying to obtain one (I'm not the best with gdb) but gdb is not registering this as an error in any way I guess, so the program just exits. I'm not sure where to set a breakpoint for this, but if you have any tips on how I can obtain a backtrace for this let me know!

@ids1024
Copy link
Member

ids1024 commented Oct 29, 2023

strace is somewhat informative:

WAYLAND_DISPLAY= strace ./target/debug/examples/winit 2>&1 | grep EBADF
close(6)                                = -1 EBADF (Bad file descriptor)
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\202\6\3\0\6\0@\0\1\0\0\0\202\2\2\0\5\0@\0\202\3\n\0\1\0@\0\4\0@\0"..., iov_len=64}], msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET, cmsg_type=SCM_RIGHTS, cmsg_data=[6]}], msg_controllen=20, msg_flags=0}, 0) = -1 EBADF (Bad file descriptor)
close(6)                                = -1 EBADF (Bad file descriptor)

These occur with c0e8723, but not the previous commit. This doesn't entirely make sense though. Nothing is shown closing fd 3 before the call getting EBADF?

@ids1024
Copy link
Member

ids1024 commented Oct 29, 2023

I guess shm_attach_fd is supposed to take ownership of the fd? Adding a try_clone there eliminatese the first of these errors, but only that one...

The could also be an issue an issue with how fds are handled in x11rb, possibly?

@ids1024
Copy link
Member

ids1024 commented Oct 29, 2023

Ah, I only changed the shm_attach_fd call in is_shm_available. Changing the other call as well fixes these errors. It's not documented in the man page, but presumably sendmsg returns EBADF when the cmsg fd is invalid.

@ids1024
Copy link
Member

ids1024 commented Oct 30, 2023

@notgull Would it be best to work around this with a try_clone().unwrap() for now, and have a fix back-ported to 0.3.x, so the X11 backend isn't broken there?

I'm not really familiar with exactly how this works in x11rb and the X protocol. Though I'd guess this is an issue in x11rb, and it's shouldn't close or keep the fd, but should just be sent over the socket to the X server which maintains its own reference to the file description.

@ids1024
Copy link
Member

ids1024 commented Oct 30, 2023

Okay, I guess https://docs.rs/x11rb/0.12.0/x11rb/protocol/shm/trait.ConnectionExt.html#method.shm_attach_fd takes Into<RawFdContainer>. So it effectively accepts an OwnedFd. I guess that's correct if x11rb needs to do buffering and not send the fd immediately. Though that API as is is easy to misuse.

ids1024 added a commit that referenced this issue Oct 30, 2023
This function takes `Into<RawFdContainer>`. So it accepts an owned fd,
and closes it. So as long as the API is like this, we need to dup a new
fd it can close when calling it.

`Into<RawFdContainer>` is implemented for anything implementing `IntoRawFd`,
so passing `OwnedFd` works.

Fixes #168. Should be
backported to 0.3.x.
notgull pushed a commit that referenced this issue Oct 31, 2023
This function takes `Into<RawFdContainer>`. So it accepts an owned fd,
and closes it. So as long as the API is like this, we need to dup a new
fd it can close when calling it.

`Into<RawFdContainer>` is implemented for anything implementing `IntoRawFd`,
so passing `OwnedFd` works.

Fixes #168. Should be
backported to 0.3.x.
@tronical
Copy link
Contributor Author

Thank you for fixing this bug.

ids1024 added a commit that referenced this issue Nov 1, 2023
This function takes `Into<RawFdContainer>`. So it accepts an owned fd,
and closes it. So as long as the API is like this, we need to dup a new
fd it can close when calling it.

`Into<RawFdContainer>` is implemented for anything implementing `IntoRawFd`,
so passing `OwnedFd` works.

Fixes #168. Should be
backported to 0.3.x.

Cherry-picked from `master` to 0.3.
@ids1024
Copy link
Member

ids1024 commented Nov 1, 2023

I've tagged and published v0.3.3 which applies this fix without the breaking change in #132.

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

Successfully merging a pull request may close this issue.

5 participants