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.

Sign up for GitHub

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

Jump to bottom

panic after dropping window on X11 #414

Closed
wez opened this issue Mar 4, 2018 · 2 comments
Closed

panic after dropping window on X11 #414

wez opened this issue Mar 4, 2018 · 2 comments
Assignees
@francesca64
Labels
DS - x11

Comments

@wez
Copy link

wez commented Mar 4, 2018

Possibly related to #79, but I'm raising this separately because there is no way for an application to trap this fault.

The issue is that the internal maps are no longer consistent / are invalidated after the drop and the event processing uses unwrap when accessing them.

Example program:

extern crate winit;

fn main() {
    let mut events_loop = winit::EventsLoop::new();

    let _window1 = Some(winit::Window::new(&events_loop).unwrap());
    let mut window2 = Some(winit::Window::new(&events_loop).unwrap());
    let _window3 = Some(winit::Window::new(&events_loop).unwrap());

    let mut num_windows = 3;

    drop(window2.take());

    events_loop.run_forever(|event| {
        match event {
            winit::Event::WindowEvent { event: winit::WindowEvent::Closed, .. } => {
                num_windows -= 1;
                if num_windows == 0 {
                    return winit::ControlFlow::Break;
                }
            },
            _ => (),
        }
        winit::ControlFlow::Continue
    })
}

Stack trace:

Breakpoint 1, rust_panic () at libstd/panicking.rs:433
433	libstd/panicking.rs: No such file or directory.
(gdb) bt
#0  rust_panic () at libstd/panicking.rs:433
#1  0x00005555556d369f in std::panicking::rust_panic_with_hook () at libstd/panicking.rs:418
#2  0x00005555556d3423 in std::panicking::begin_panic_fmt () at libstd/panicking.rs:349
#3  0x00005555556d3353 in rust_begin_unwind () at libstd/panicking.rs:325
#4  0x0000555555712be1 in core::panicking::panic_fmt () at libcore/panicking.rs:72
#5  0x0000555555712b07 in core::panicking::panic () at libcore/panicking.rs:51
#6  0x00005555555784aa in <core::option::Option<T>>::unwrap (self=...) at /checkout/src/libcore/macros.rs:20
#7  0x000055555557a2cb in winit::platform::platform::x11::EventsLoop::process_event (self=0x7fffffffda40, xev=0x7fffffffd8b8, callback=0x7fffffffd980)
    at /home/wez/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.11.1/src/platform/linux/x11/mod.rs:360
#8  0x0000555555578e57 in winit::platform::platform::x11::EventsLoop::run_forever (self=0x7fffffffda40, callback=...)
    at /home/wez/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.11.1/src/platform/linux/x11/mod.rs:188
#9  0x000055555556c428 in winit::platform::platform::EventsLoop::run_forever (self=0x7fffffffda38, callback=...)
    at /home/wez/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.11.1/src/platform/linux/mod.rs:399
#10 0x000055555556fb68 in winit::EventsLoop::run_forever (self=0x7fffffffda38, callback=...)
    at /home/wez/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.11.1/src/lib.rs:231
#11 0x0000555555575881 in winitbug::main () at src/main.rs:14
wez added a commit to wezterm/wezterm that referenced this issue Mar 4, 2018
@wez
We can now spawn additional windows
6623e1f 
However, rust-windowing/winit#414 makes this unsafe.
@francesca64 francesca64 self-assigned this Mar 4, 2018
@francesca64
Copy link
Member

I've created a branch that should solve both this and #79.

The way I implemented Drop for Window is sort of surprising, since all it does is send a WM_DELETE_WINDOW ClientMessage, which the event loop then handles to do the actual cleanup and destroying. I'm not sure how kosher that is on a conceptual level (in terms of what users expect Drop to be responsible for), but it behaves the best of anything I've tried.

I've also found that the old Drop implementation freezes the event loop, which I've determined to be related to the mutex locking. I still don't understand exactly why that was an issue, but now that the locking is done in the event loop, everything works fine.

francesca64 added a commit to francesca64/winit that referenced this issue Mar 4, 2018
@francesca64
x11: Destroy dropped windows; handle WM_DELETE_WINDOW
7965d0d 
Fixes rust-windowing#79 rust-windowing#414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
francesca64 added a commit to francesca64/winit that referenced this issue Mar 4, 2018
@francesca64
x11: Destroy dropped windows; handle WM_DELETE_WINDOW
6e21c18 
Fixes rust-windowing#79 rust-windowing#414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
@francesca64 francesca64 mentioned this issue Mar 4, 2018
Merged
francesca64 added a commit to francesca64/winit that referenced this issue Mar 11, 2018
@francesca64
x11: Destroy dropped windows; handle WM_DELETE_WINDOW
95d0f51 
Fixes rust-windowing#79 rust-windowing#414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
francesca64 added a commit to francesca64/winit that referenced this issue Mar 15, 2018
@francesca64
x11: Destroy dropped windows; handle WM_DELETE_WINDOW
8a14a75 
Fixes rust-windowing#79 rust-windowing#414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
@francesca64 francesca64 mentioned this issue Mar 15, 2018
Closed
francesca64 added a commit to francesca64/winit that referenced this issue Mar 15, 2018
@francesca64
x11: Destroy dropped windows; handle WM_DELETE_WINDOW
8ef30cd 
Fixes rust-windowing#79 rust-windowing#414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
tomaka pushed a commit that referenced this issue Mar 23, 2018
@francesca64 @tomaka
x11: Destroy dropped windows; handle WM_DELETE_WINDOW (#416)
d667a39 
Fixes #79 #414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
@francesca64
Copy link
Member

Fixed via #416

francesca64 added a commit to francesca64/winit that referenced this issue Mar 26, 2018
@francesca64
x11: Destroy dropped windows; handle WM_DELETE_WINDOW (rust-windowing…
0aa70c2 
…#416)

Fixes rust-windowing#79 rust-windowing#414

This changes the implementation of Drop for Window to send a WM_DELETE_WINDOW ClientMessage,
offloading all the cleanup and window destruction to the event loop. Unsurprisingly, this
entails that the event loop now handles WM_DELETE_WINDOW using the behavior that was
previously contained in Window's Drop implementation, along with destroying the Window.
Not only does this mean that dropped windows are closed, but also that clicking the × button
on the window actually closes it now.

The previous implemention of Drop was also broken, as the event loop would be (seemingly
permenanently) frozen after its invocation. That was caused specifically by the mutex
locking, and is no longer an issue now that the locking is done in the event loop.

While I don't have full confidence that it makes sense for the Drop implementation to behave
this way, this is nonetheless a significant improvement. The previous behavior led to
inconsistent state, panics, and event loop breakage, along with not actually destroying the
window.

This additionally makes the assumption that users don't need Focused or CursorLeft events
for the destroyed window, as Closed is adequate to indicate unfocus, and users may not
expect to receive events for closed/dropped windows. In my testing, those specific events
were sent immediately after the window was destroyed, though this sort of behavior could be
WM-specific. I've opted to explicitly suppress those events in the case of the window no
longer existing.
This was referenced Mar 12, 2020
Closed
Closed
tmfink pushed a commit to tmfink/winit that referenced this issue Jan 5, 2022
@bors-servo
Auto merge of rust-windowing#414 - pcwalton:radial-gradient-epsilon, …
a05270f 
…r=pcwalton

Allow radial gradients to be evaluated with any nonzero discriminant, not just ones with magnitude above `EPSILON`.

Closes rust-windowing#399.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@francesca64 francesca64

Labels
DS - x11
Milestone
No milestone
Development

No branches or pull requests
2 participants
@wez @francesca64