Skip to content

Commit

Permalink
Fix a rare crash in some X11 implementations (details below).
Browse files Browse the repository at this point in the history
In some X11 implementations, calling XCloseDisplay from any thread
other than the main thread causes heap corruption and an eventual
crash.

Previously, the window proxy struct held a Weak<> ptr to the display,
which was upgraded to a strong arc only during the send of the X
message.

However, if the main thread closes the window *after* the thread
upgrades the weak ref, and this was the last strong reference,
then the only remaining strong reference is in the window proxy
thread, so the XCloseDisplay is executed on the thread.

Instead, use a simpler solution where the display reference used
by the window proxy threads is cleared by the main thread when
the window is closed, and the XCloseDisplay can only be executed
by the main thread.

This has the added benefit of not relying on the unstable Weak<>
library feature, so we will be able to get this upstreamed.
  • Loading branch information
gw3583 committed Jul 19, 2015
1 parent 4cf1537 commit 7ee0318
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 10 deletions.
2 changes: 0 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
//!
//! By default only `window` is enabled.
#![feature(arc_weak)]

#[macro_use]
extern crate lazy_static;

Expand Down
37 changes: 29 additions & 8 deletions src/x11/window/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::cell::Cell;
use std::sync::atomic::AtomicBool;
use std::collections::VecDeque;
use super::ffi;
use std::sync::{Arc, Mutex, Once, ONCE_INIT, Weak};
use std::sync::{Arc, Mutex, Once, ONCE_INIT};

use Api;
use CursorState;
Expand Down Expand Up @@ -46,6 +46,13 @@ fn with_c_str<F, T>(s: &str, f: F) -> T where F: FnOnce(*const libc::c_char) ->
f(c_str.as_ptr())
}

struct WindowProxyData {
display: *mut ffi::Display,
window: ffi::Window,
}

unsafe impl Send for WindowProxyData {}

struct XWindow {
display: *mut ffi::Display,
window: ffi::Window,
Expand All @@ -56,6 +63,7 @@ struct XWindow {
ic: ffi::XIC,
im: ffi::XIM,
colormap: ffi::Colormap,
window_proxy_data: Arc<Mutex<Option<WindowProxyData>>>,
}

unsafe impl Send for XWindow {}
Expand All @@ -67,6 +75,10 @@ unsafe impl Sync for Window {}
impl Drop for XWindow {
fn drop(&mut self) {
unsafe {
// Clear out the window proxy data arc, so that any window proxy objects
// are no longer able to send messages to this window.
*self.window_proxy_data.lock().unwrap() = None;

// we don't call MakeCurrent(0, 0) because we are not sure that the context
// is still the current one
ffi::glx::DestroyContext(self.display as *mut _, self.context);
Expand All @@ -87,26 +99,28 @@ impl Drop for XWindow {

#[derive(Clone)]
pub struct WindowProxy {
x: Weak<XWindow>,
data: Arc<Mutex<Option<WindowProxyData>>>,
}

impl WindowProxy {
pub fn wakeup_event_loop(&self) {
if let Some(x) = self.x.upgrade() {
let window_proxy_data = self.data.lock().unwrap();

if let Some(ref data) = *window_proxy_data {
let mut xev = ffi::XClientMessageEvent {
type_: ffi::ClientMessage,
window: x.window,
window: data.window,
format: 32,
message_type: 0,
serial: 0,
send_event: 0,
display: x.display,
display: data.display,
data: unsafe { mem::zeroed() },
};

unsafe {
ffi::XSendEvent(x.display, x.window, 0, 0, mem::transmute(&mut xev));
ffi::XFlush(x.display);
ffi::XSendEvent(data.display, data.window, 0, 0, mem::transmute(&mut xev));
ffi::XFlush(data.display);
}
}
}
Expand Down Expand Up @@ -643,6 +657,12 @@ impl Window {
}

// creating the window object
let window_proxy_data = WindowProxyData {
display: display,
window: window,
};
let window_proxy_data = Arc::new(Mutex::new(Some(window_proxy_data)));

let window = Window {
x: Arc::new(XWindow {
display: display,
Expand All @@ -654,6 +674,7 @@ impl Window {
is_fullscreen: builder.monitor.is_some(),
xf86_desk_mode: xf86_desk_mode,
colormap: cmap,
window_proxy_data: window_proxy_data,
}),
is_closed: AtomicBool::new(false),
wm_delete_window: wm_delete_window,
Expand Down Expand Up @@ -738,7 +759,7 @@ impl Window {

pub fn create_window_proxy(&self) -> WindowProxy {
WindowProxy {
x: self.x.downgrade()
data: self.x.window_proxy_data.clone()
}
}

Expand Down

0 comments on commit 7ee0318

Please sign in to comment.