X11: Harden window tree discovery

src/platform/x11/event_loop.rs

@@ -283,10 +283,12 @@ impl EventLoop {
             }
             WindowThreadRequest::Show => {
                 self.window.xcb_window.map_window()?.check()?;
+                self.window.visibility_state.window_mapped(self.window.xcb_window.id());
                 Ok(())
             }
             WindowThreadRequest::Hide => {
                 self.window.xcb_window.unmap_window()?.check()?;
+                self.window.visibility_state.window_unmapped(self.window.xcb_window.id());
                 Ok(())
             }
         }
@@ -323,6 +325,7 @@ impl EventLoop {
     }
 
     pub fn run(mut self, mut inner: calloop::EventLoop<Self>) -> Result<(), PlatformError> {
+        self.drain_xcb_events()?;
         inner.run(None, &mut self, Self::handle_idle)?;
 
         self.handle_event(Event::Window(WindowEvent::WillClose));
@@ -410,7 +413,9 @@ impl EventLoop {
                 // These are coalesced and then handled asynchronously at the end of the event loop
                 if event.window == self.window.raw_id() {
                     self.new_size = Some(PhysicalSize::new(event.width, event.height));
-                } else if Some(event.window) == self.window.visibility_state.parent_id() {
+                } else if Some(event.window)
+                    == self.window.visibility_state.parent_id().map(|i| i.get())
+                {
                     // Also resize the window if the parent is resized
                     // This works around some hosts that might not call set_size() right away (or at all...)
                     self.new_parent_size = Some(PhysicalSize::new(event.width, event.height));
@@ -503,35 +508,46 @@ impl EventLoop {
             }
 
             XEvent::MapNotify(e) => {
-                if e.window == self.window.raw_id() {
-                    self.window.is_mapped.set(true);
-                }
+                if let Some(window_id) = NonZero::new(e.window) {
+                    if window_id == self.window.xcb_window.id() {
+                        self.window.is_mapped.set(true);
+                    }
 
-                let became_viewable = self.window.visibility_state.window_mapped(e.window);
+                    let became_viewable = self.window.visibility_state.window_mapped(window_id);
 
-                if became_viewable {
-                    self.exposed = true;
+                    if became_viewable {
+                        self.exposed = true;
+                    }
                 }
             }
 
             XEvent::UnmapNotify(e) => {
-                if e.window == self.window.raw_id() {
-                    self.window.is_mapped.set(false)
-                }
+                if let Some(window_id) = NonZero::new(e.window) {
+                    if window_id == self.window.xcb_window.id() {
+                        self.window.is_mapped.set(false)
+                    }
 
-                self.window.visibility_state.window_unmapped(e.window);
+                    self.window.visibility_state.window_unmapped(window_id);
+                }
             }
 
-            XEvent::ReparentNotify(e) => self.window.visibility_state.window_reparented(
-                e.window,
-                e.parent,
-                &self.window.connection.conn,
-            ),
+            XEvent::ReparentNotify(e) => {
+                if let Some(window_id) = NonZero::new(e.window) {
+                    self.window.visibility_state.window_reparented(
+                        window_id,
+                        NonZero::new(e.parent),
+                        &self.window.connection,
+                    )
+                }
+            }
 
-            XEvent::DestroyNotify(e) => self
-                .window
-                .visibility_state
-                .window_destroyed(e.window, &self.window.connection.conn),
+            XEvent::DestroyNotify(e) => {
+                if let Some(window_id) = NonZero::new(e.window) {
+                    self.window
+                        .visibility_state
+                        .window_destroyed(window_id, &self.window.connection)
+                }
+            }
 
             _ => {}
         }

src/platform/x11/visibility_tree.rs

@@ -1,41 +1,43 @@
+use crate::platform::X11Connection;
 use std::cell::{Cell, RefCell};
+use std::num::NonZeroU32;
 use x11rb::errors::ReplyError;
-use x11rb::protocol::xproto::{ConnectionExt, MapState, QueryTreeReply, Window};
+use x11rb::protocol::xproto::{ConnectionExt, MapState, QueryTreeReply};
 use x11rb::protocol::ErrorKind;
 use x11rb::x11_utils::X11Error;
-use x11rb::xcb_ffi::XCBConnection;
 
 pub struct AncestorVisibilityState {
     ancestry: AncestryList,
     own_window_viewable: Cell<bool>,
+    root_id: Cell<Option<NonZeroU32>>,
 }
 
 struct AncestryList {
     inner: RefCell<Vec<Ancestor>>,
 }
 
 impl AncestryList {
-    pub fn new(own_window: Window) -> Self {
+    pub fn new(own_window: NonZeroU32) -> Self {
         Self { inner: RefCell::new(vec![Ancestor { id: own_window, mapped: false.into() }]) }
     }
 
-    pub fn pop_id(&self) -> Option<Window> {
+    pub fn pop_id(&self) -> Option<NonZeroU32> {
         self.inner.borrow_mut().pop().map(|a| a.id)
     }
 
-    pub fn last_id(&self) -> Option<Window> {
+    pub fn last_id(&self) -> Option<NonZeroU32> {
         self.inner.borrow().last().map(|a| a.id)
     }
 
     pub fn push(&self, ancestor: Ancestor) {
         self.inner.borrow_mut().push(ancestor);
     }
 
-    pub fn parent_id(&self) -> Option<Window> {
+    pub fn parent_id(&self) -> Option<NonZeroU32> {
         self.inner.borrow().get(1).map(|a| a.id)
     }
 
-    pub fn remove_window(&self, id: Window) -> bool {
+    pub fn remove_window(&self, id: NonZeroU32) -> bool {
         let mut inner = self.inner.borrow_mut();
         let Some(index) = inner.iter().position(|a| a.id == id) else {
             return false;
@@ -46,7 +48,7 @@ impl AncestryList {
         true
     }
 
-    pub fn remove_after_window(&self, id: Window) -> bool {
+    pub fn remove_after_window(&self, id: NonZeroU32) -> bool {
         let mut inner = self.inner.borrow_mut();
         let Some(index) = inner.iter().position(|a| a.id == id) else {
             return false;
@@ -61,7 +63,7 @@ impl AncestryList {
         self.inner.borrow().iter().all(|a| a.mapped.get())
     }
 
-    pub fn set_mapped(&self, window: Window, mapped: bool) -> bool {
+    pub fn set_mapped(&self, window: NonZeroU32, mapped: bool) -> bool {
         let inner = self.inner.borrow();
         let Some(ancestor) = inner.iter().find(|a| a.id == window) else {
             return false;
@@ -74,15 +76,18 @@ impl AncestryList {
 
 #[cfg_attr(debug_assertions, derive(Debug))]
 struct Ancestor {
-    id: Window,
+    id: NonZeroU32,
     mapped: Cell<bool>,
 }
 
 impl AncestorVisibilityState {
-    pub fn discover(connection: &XCBConnection, own_window_id: Window) -> Result<Self, ReplyError> {
+    pub fn discover(
+        connection: &X11Connection, own_window_id: NonZeroU32,
+    ) -> Result<Self, ReplyError> {
         let this = Self {
             ancestry: AncestryList::new(own_window_id),
             own_window_viewable: Cell::new(false),
+            root_id: Cell::new(NonZeroU32::new(connection.default_screen().root)),
         };
 
         this.try_regenerate_from_last_window(connection)?;
@@ -94,12 +99,12 @@ impl AncestorVisibilityState {
         self.own_window_viewable.get()
     }
 
-    pub fn parent_id(&self) -> Option<Window> {
+    pub fn parent_id(&self) -> Option<NonZeroU32> {
         self.ancestry.parent_id()
     }
 
     /// Returns `true` if this operation made our own window visible.
-    pub fn window_mapped(&self, window_id: Window) -> bool {
+    pub fn window_mapped(&self, window_id: NonZeroU32) -> bool {
         if !self.ancestry.set_mapped(window_id, true) {
             return false;
         }
@@ -116,15 +121,15 @@ impl AncestorVisibilityState {
         all_mapped
     }
 
-    pub fn window_unmapped(&self, window_id: Window) {
+    pub fn window_unmapped(&self, window_id: NonZeroU32) {
         if !self.ancestry.set_mapped(window_id, false) {
             return;
         }
 
         self.own_window_viewable.set(false);
     }
 
-    pub fn window_destroyed(&self, window_id: Window, connection: &XCBConnection) {
+    pub fn window_destroyed(&self, window_id: NonZeroU32, connection: &X11Connection) {
         if !self.ancestry.remove_window(window_id) {
             return;
         }
@@ -133,30 +138,39 @@ impl AncestorVisibilityState {
     }
 
     pub fn window_reparented(
-        &self, window_id: Window, new_parent: Window, connection: &XCBConnection,
+        &self, window_id: NonZeroU32, new_parent: Option<NonZeroU32>, connection: &X11Connection,
     ) {
         if !self.ancestry.remove_after_window(window_id) {
             return;
         }
 
-        self.ancestry.push(Ancestor { id: new_parent, mapped: Cell::new(false) });
+        if let Some(new_parent) = new_parent {
+            if Some(new_parent) == self.root_id.get() {
+                return;
+            }
 
-        self.regenerate_from_last_window(connection);
+            self.ancestry.push(Ancestor { id: new_parent, mapped: Cell::new(false) });
+
+            self.regenerate_from_last_window(connection);
+        }
     }
 
-    pub fn regenerate_from_last_window(&self, connection: &XCBConnection) {
+    pub fn regenerate_from_last_window(&self, connection: &X11Connection) {
         if let Err(e) = self.try_regenerate_from_last_window(connection) {
             crate::warn!("Failed to generate window ancestry list: {}", e)
         }
     }
 
     fn try_regenerate_from_last_window(
-        &self, connection: &XCBConnection,
+        &self, connection: &X11Connection,
     ) -> Result<(), ReplyError> {
         let Some(mut current_window) = self.ancestry.pop_id() else { return Ok(()) };
 
+        let mut shitlist = Vec::new();
+        let mut rechecked_children = Vec::new();
+
         loop {
-            let Some((mapped, tree)) = fetch_window_info(connection, current_window)? else {
+            let Some((mut mapped, tree)) = fetch_window_info(connection, current_window)? else {
                 // We got a BadWindow while trying to get a window's info, it must have been destroyed.
                 // Try to go back a layer and fetch the window's state and parent again
 
@@ -167,34 +181,68 @@ impl AncestorVisibilityState {
                     break;
                 };
 
+                if shitlist.contains(&previous_parent) {
+                    crate::warn!(
+                        "Failed to get info for window {} in the past already. Stopping.",
+                        previous_parent
+                    );
+                    break;
+                }
+
+                if shitlist.len() > 10 {
+                    crate::warn!(
+                        "Too many failures while trying to build X ancestry tree. Stopping."
+                    );
+                    break;
+                }
+
+                shitlist.push(previous_parent);
+
                 current_window = previous_parent;
                 continue;
             };
 
-            if tree.parent == current_window {
+            if tree.parent == current_window.get() {
                 // Weird, but that might also mean we're at the end of the tree (or the window has no parent yet)
                 break;
             }
 
             // Sanity check if the current parent is actually registered to have the child in its children list
             if let Some(child_id) = self.ancestry.last_id() {
-                if !tree.children.contains(&child_id) {
+                if !tree.children.contains(&child_id.get()) {
                     // The child has been orphaned, it must have been reparented between our server queries.
                     // Go back a step and check again.
 
-                    crate::warn!(
-                        "Children of parent {} does not contain {}: {:?}",
-                        current_window,
-                        child_id,
-                        &tree.children
-                    );
+                    if rechecked_children.contains(&child_id) {
+                        crate::warn!(
+                            "Children of parent {} does not contain {}: {:?}",
+                            current_window,
+                            child_id,
+                            &tree.children
+                        );
+                    } else {
+                        rechecked_children.push(child_id);
+                    }
 
                     let Some(_) = self.ancestry.pop_id() else { unreachable!() };
                     current_window = child_id;
                     continue;
                 }
             }
 
+            // Despite what's documented, all windows down the parent tree must have the event mask
+            // bit set, otherwise events are not propagated through to us.
+            if let Err(e) =
+                connection.register_tree_structure_events_for_window(current_window)?.check()
+            {
+                crate::warn!(
+                    "Could not register SubstructureNotify event for window {}: {}",
+                    current_window,
+                    e
+                );
+                mapped = true; // Assume it is mapped, since we'll possibly not get any events from this window
+            }
+
             // All checks succeeded, now register the current window info and fetch info from the parent
             self.ancestry.push(Ancestor { id: current_window, mapped: mapped.into() });
 
@@ -203,7 +251,18 @@ impl AncestorVisibilityState {
                 break;
             }
 
-            current_window = tree.parent;
+            // If parent == 0, assume there's no parent and just break
+            if let Some(parent) = NonZeroU32::new(tree.parent) {
+                current_window = parent;
+            } else {
+                break;
+            }
+
+            if let Some(root) = NonZeroU32::new(tree.root) {
+                if Some(root) != self.root_id.get() {
+                    self.root_id.set(Some(root))
+                }
+            }
         }
 
         self.own_window_viewable.set(self.ancestry.check_all_mapped());
@@ -214,10 +273,10 @@ impl AncestorVisibilityState {
 
 /// Returns Ok(None) on BadWindow.
 fn fetch_window_info(
-    connection: &XCBConnection, window: Window,
+    connection: &X11Connection, window: NonZeroU32,
 ) -> Result<Option<(bool, QueryTreeReply)>, ReplyError> {
-    let attrs_cookie = connection.get_window_attributes(window)?;
-    let tree_cookie = connection.query_tree(window)?;
+    let attrs_cookie = connection.conn.get_window_attributes(window.get())?;
+    let tree_cookie = connection.conn.query_tree(window.get())?;
 
     let mapped = match attrs_cookie.reply() {
         Ok(attr) => attr.map_state != MapState::UNMAPPED,

src/platform/x11/window_shared.rs

@@ -103,8 +103,7 @@ impl WindowInner {
 
         connection.register_tree_structure_events()?.check()?;
 
-        let visibility_state =
-            AncestorVisibilityState::discover(&connection.conn, xcb_window.id().get())?;
+        let visibility_state = AncestorVisibilityState::discover(&connection, xcb_window.id())?;
 
         let cookies = [
             xcb_window.set_title(&options.title)?,