GLXBadWindow creating multiple windows

[justincredible]

Greetings,

I'm working on a PR to get the glium tests runnable. The initial approach worked on Wayland and Windows, but X11 had some additional issues. There is an alternative approach that has no issues on the three platforms (except the Wayland window visibility mentioned in the PR).

I have no experience with X (or Wayland), but the documentation for Window says it implements Send and Sync for all platforms (except wasm32). Clearly, the event loop is not being used correctly in that code, but I'm raising this in case these issues are concerning.

X11 issues

Deadlock

Easily reproduced via cargo test --test attrs. Sometimes the first test passes but usually not.

Bad Window

cargo test --test buffer will reliably fail but the issues are intermittent enough that running with -- --test-threads 1 can result in a short run of repeated successes. Otherwise, there are errors like:

'event_loop_thread' panicked at 'called Result::unwrap() on an Err value: OsError { line: 478, file: "/home/justin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/winit-0.28.6/src/platform_impl/linux/x11/window.rs", error: XError(XError { description: "GLXBadWindow", error_code: 170, request_code: 152, minor_code: 32 }) }', tests/support/mod.rs:78:34

or

thread 'event_loop_thread' panicked at 'Failed to destroy input context: XError { description: "GLXBadWindow", error_code: 170, request_code: 152, minor_code: 32 }', /home/justin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/winit-0.28.6/src/platform_impl/linux/x11/event_processor.rs:547:22

Experimenting with the builder options and some of the extension traits would result in different error messages and line numbers (in event processor) for the GLXBadWindow errors, and the frequency of the errors would change, but nothing eliminated them.

Bad Surface

In addition to the "Bad Window" errors, this error can also crop up when creating the GL context:

thread 'buffer_dynamic_read_slice' panicked at 'called Result::unwrap() on an Err value: Error { raw_code: Some(170), raw_os_message: Some("GLXBadWindow"), kind: BadSurface }', tests/support/mod.rs:124:77

Adding retry mechanisms when creating the window and creating the context can prevent the "Bad Surface" and many of the "Bad Window" errors from ending the test run, but the GLXBadWindow errors inside the event processor still occur.

[lunixbochs]

I use multiple opengl windows in Talon, they're created/destroyed all the time. I'm using this patch for the event processor: https://github.com/talonvoice/winit/commit/ac258bb037f68f381cc50903eac80827ebc9ee0f

I've found winit has way too many expects/panics in general, especially on X11. It's very awkward for a gui app to just disappear.

My talonvoice/0.28 branch has a couple of patches for X11 panics.

[kchibisov]

The issue here is that winit doesn't ever touch GLX the error raises from some other place. X11 errors are usually fatal and can't be just ignored, however GLX errors sometimes are not fatal.

If you can tell what you're using for GLX exactly (if glutin, which version) I can probably help, but that's about it.

[lunixbochs]

Last confirmed on glutin 0.30.3. My linked winit patch was sufficient to fix so I haven't confirmed on a newer glutin.

X11 errors are usually fatal and can't be just ignored

This very much doesn't match my experience. If you pick almost any X11 method and any error it can generate, I can probably explain why I don't want that error to terminate the calling thread in my app. It's actually been quite frustrating to get winit to stop panicking so much. I'd prefer a more fallible API surface that returns Results for way more of the methods. IMO something like resizing a window shouldn't have the option to panic inside winit, regardless of whether it's due to the window size being rejected, or the window handle being entirely bad. (Easy panic on 0.28: window.set_inner_size(winit::dpi::LogicalSize::new(0.0, 0.0));. The X connection and Window are fine otherwise, it's just this one call that X11 didn't like)

[kchibisov]

@lunixbochs if you read glutin changelog, you'll notice that 0.30.4 fixed the GLX handling, because it was a bug in glutin.

Also, if you read some panics in the winit, they are for connection writing, and not for actual error. It's true that some panics might be out of place, but x11 backend change a lot, so before doing claims I'd suggest to open issues against latest master with the actual crash logs.

[lunixbochs]

it was a bug in glutin

good to know. it's pretty awkward that another library can leave stale errors around in a way that panics winit - but that specific reason for panic should be fixed when the x11rb port finishes I think? because xcb actually matches the error to the appropriate request iirc

It's true that some panics might be out of place

I manually audited every unwrap and expect call in winit a bit ago. I do hope the x11rb work helps, but I still see many expects in there for what should be fallible x11 errors.

I'd almost argue that even if the x11 connection completely drops, it should be the application's decision to panic, not winit's. There are use cases where the UI isn't actually the main reason the app is running, so if the UI fails the app doesn't necessarily want to exit.

[kchibisov]

good to know. it's pretty awkward that another library can leave stale errors around in a way that panics winit - but that specific reason for panic should be fixed when the x11rb port finishes I think? because xcb actually matches the error to the appropriate request iirc

nothing will change when it comes to using GLX. GLX demands libX11, the libX11 and it's error handling will be with us. There could be option to use xcb, but you won't have ability to use GLX, obviously.

I manually audited every unwrap and expect call in winit a bit ago. I do hope the x11rb work helps, but I still see many expects in there for what should be fallible x11 errors.

You know that you can send patches. Barely anyone cares about fixing X11.

[lunixbochs]

There could be option to use xcb

There's an x11rb port in progress on master. I know you need libX11 to get involved for GLX, but afaik any xcb-style calls won't be polluted with X11 errors at least.

you can send patches

Yep, I've made PRs for some of my other changes already. I'd be happy to work on making winit calls generally more fallible - preferring to return Result for non-fatal errors (or swallowing/logging non-fatal errors that are e.g. buried in Drop methods rather than panicking on them). I think this applies to more than just X11 anyway.

[justincredible]

@kchibisov, it's using glutin 0.30.4. Yes, I wasn't sure whether to raise this with winit or glutin, but chose winit due to the event_processor.rs panics.

These issues are not much of a concern for me, as the alternative approach is very similar and does not have any of these problems. The winit docs recommend against this kind of API use, so it may not be worth supporting.

That said, handling the errors like in this branch provides a more reproducible panic: buffer tests backtrace