Bug fix #68: Possible fix for failure in client window detection

- Note I'm not in the best state today (bad cold & sleep-deprived). This commit is likely to introduce bugs. - Attempt to fix client window detection failures happening when compton searches for client window before it's ready. - Fix build failure with <libpcre-8.20. Thanks to @pvanek for reporting in #51. - Move client window detection to a new function win_recheck_client(). - Add win_unmark_client(), which unmarks a client window. - Rename a few functions. - Split fetching of values of type-Window properties to a new function wid_get_prop_window(). - Add extra safety checks and assert calls to various functions, to expose potential bugs. - Fix a memory leak that w->role is not freed on window destruction.
2012-12-07 22:38:10 +08:00 · 2012-12-07 22:38:10 +08:00 · 812ba93516
parent a7189263e1
commit 812ba93516
2 changed files with 176 additions and 58 deletions
--- a/src/compton.c
+++ b/src/compton.c
@ -1143,7 +1143,7 @@ border_size(session_t *ps, win *w) {
 */
 static Window
 find_client_win(session_t *ps, Window w) {
-  if (wid_has_attr(ps, w, ps->atom_client)) {
+  if (wid_has_prop(ps, w, ps->atom_client)) {
    return w;
  }

@ -1799,15 +1799,23 @@ map_win(session_t *ps, Window id) {
  win *w = find_win(ps, id);

  // Don't care about window mapping if it's an InputOnly window
-  if (!w || InputOnly == w->a.class) return;
+  // Try avoiding mapping a window twice
+  if (!w || InputOnly == w->a.class
+      || IsViewable == w->a.map_state)
+    return;

-  w->focused_real = false;
-  if (ps->o.track_focus && ps->o.use_ewmh_active_win
-      && w == ps->active_win)
-    w->focused_real = true;
+  assert(!w->focused_real);

  w->a.map_state = IsViewable;

+  // Set focused to false
+  bool focused_real = false;
+  if (ps->o.track_focus && ps->o.use_ewmh_active_win
+      && w == ps->active_win)
+    focused_real = true;
+  win_set_focused(ps, w, focused_real);
+
+
  // Call XSelectInput() before reading properties so that no property
  // changes are lost
  XSelectInput(ps->dpy, id, determine_evmask(ps, id, WIN_EVMODE_FRAME));
@ -1823,31 +1831,11 @@ map_win(session_t *ps, Window id) {
  // Detect client window here instead of in add_win() as the client
  // window should have been prepared at this point
  if (!w->client_win) {
-    w->wmwin = false;
-
-    Window cw = None;
-    // Always recursively look for a window with WM_STATE, as Fluxbox
-    // sets override-redirect flags on all frame windows.
-    cw = find_client_win(ps, w->id);
-#ifdef DEBUG_CLIENTWIN
-    if (cw)
-      printf("find_client_win(%#010lx): client %#010lx\n", w->id, cw);
-#endif
-    // Set a window's client window to itself if we couldn't find a
-    // client window
-    if (!cw) {
-      cw = w->id;
-      w->wmwin = !w->a.override_redirect;
-#ifdef DEBUG_CLIENTWIN
-      printf("find_client_win(%#010lx): client self (%s)\n", w->id,
-          (w->wmwin ? "wmwin": "override-redirected"));
-#endif
-    }
-    mark_client_win(ps, w, cw);
+    win_recheck_client(ps, w);
  }
  else {
    // Re-mark client window here
-    mark_client_win(ps, w, w->client_win);
+    win_mark_client(ps, w, w->client_win);
  }

  assert(w->client_win);
@ -1947,11 +1935,11 @@ unmap_win(session_t *ps, Window id) {

  if (!w || IsUnmapped == w->a.map_state) return;

-  w->a.map_state = IsUnmapped;
-
  // Set focus out
  win_set_focused(ps, w, false);

+  w->a.map_state = IsUnmapped;
+
  // Fading out
  w->flags |= WFLAG_OPCT_CHANGE;
  set_fade_callback(ps, w, unmap_callback, false);
@ -2207,18 +2195,20 @@ calc_shadow_geometry(session_t *ps, win *w) {
 /**
 * Mark a window as the client window of another.
 *
- * @param dpy display to use
+ * @param ps current session
 * @param w struct _win of the parent window
 * @param client window ID of the client window
 */
 static void
-mark_client_win(session_t *ps, win *w, Window client) {
+win_mark_client(session_t *ps, win *w, Window client) {
  w->client_win = client;

  XSelectInput(ps->dpy, client, determine_evmask(ps, client, WIN_EVMODE_CLIENT));

-  // Get the frame width and monitor further frame width changes on client
-  // window if necessary
+  // Make sure the XSelectInput() requests are sent
+  XSync(ps->dpy, False);
+
+  // Get frame widths if needed
  if (ps->o.frame_opacity) {
    get_frame_extents(ps, w, client);
  }
@ -2232,11 +2222,69 @@ mark_client_win(session_t *ps, win *w, Window client) {
  // _NET_WM_WINDOW_TYPE_NORMAL, otherwise as _NET_WM_WINDOW_TYPE_DIALOG.
  if (WINTYPE_UNKNOWN == w->window_type) {
    if (w->a.override_redirect
-        || !wid_has_attr(ps, client, ps->atom_transient))
+        || !wid_has_prop(ps, client, ps->atom_transient))
      w->window_type = WINTYPE_NORMAL;
    else
      w->window_type = WINTYPE_DIALOG;
  }
+
+  win_update_focused(ps, w);
+}
+
+/**
+ * Unmark current client window of a window.
+ *
+ * @param ps current session
+ * @param w struct _win of the parent window
+ */
+static void
+win_unmark_client(session_t *ps, win *w) {
+  Window client = w->client_win;
+
+  w->client_win = None;
+
+  // Recheck event mask
+  XSelectInput(ps->dpy, client,
+      determine_evmask(ps, client, WIN_EVMODE_UNKNOWN));
+}
+
+/**
+ * Recheck client window of a window.
+ *
+ * @param ps current session
+ * @param w struct _win of the parent window
+ */
+static void
+win_recheck_client(session_t *ps, win *w) {
+  // Initialize wmwin to false
+  w->wmwin = false;
+
+  // Look for the client window
+
+  // Always recursively look for a window with WM_STATE, as Fluxbox
+  // sets override-redirect flags on all frame windows.
+  Window cw = find_client_win(ps, w->id);
+#ifdef DEBUG_CLIENTWIN
+  if (cw)
+    printf_dbgf("(%#010lx): client %#010lx\n", w->id, cw);
+#endif
+  // Set a window's client window to itself if we couldn't find a
+  // client window
+  if (!cw) {
+    cw = w->id;
+    w->wmwin = !w->a.override_redirect;
+#ifdef DEBUG_CLIENTWIN
+    printf_dbgf("(%#010lx): client self (%s)\n", w->id,
+        (w->wmwin ? "wmwin": "override-redirected"));
+#endif
+  }
+
+  // Unmark the old one
+  if (w->client_win && w->client_win != cw)
+    win_unmark_client(ps, w);
+
+  // Mark the new one
+  win_mark_client(ps, w, cw);
 }

 static void
@ -2268,6 +2316,11 @@ add_win(session_t *ps, Window id, Window prev) {
    return;
  }

+  // Delay window mapping
+  int map_state = new->a.map_state;
+  assert(IsViewable == map_state || IsUnmapped == map_state);
+  new->a.map_state = IsUnmapped;
+
  new->damaged = 0;
  new->to_paint = false;
  new->pixmap = None;
@ -2342,7 +2395,7 @@ add_win(session_t *ps, Window id, Window prev) {
  new->next = *p;
  *p = new;

-  if (new->a.map_state == IsViewable) {
+  if (map_state == IsViewable) {
    map_win(ps, id);
  }
 }
@ -2655,6 +2708,27 @@ expose_root(session_t *ps, XRectangle *rects, int nrects) {
  add_damage(ps, region);
 }

+/**
+ * Get the value of a type-<code>Window</code> property of a window.
+ *
+ * @return the value if successful, 0 otherwise
+ */
+static Window
+wid_get_prop_window(session_t *ps, Window wid, Atom aprop) {
+  // Get the attribute
+  Window p = None;
+  winprop_t prop = wid_get_prop(ps, wid, aprop, 1L, XA_WINDOW, 32);
+
+  // Return it
+  if (prop.nitems) {
+    p = *prop.data.p32;
+  }
+
+  free_winprop(&prop);
+
+  return p;
+}
+
 /**
 * Get the value of a text property of a window.
 */
@ -3006,15 +3080,32 @@ ev_reparent_notify(session_t *ps, XReparentEvent *ev) {
    add_win(ps, ev->window, 0);
  } else {
    destroy_win(ps, ev->window);
+
    // Reset event mask in case something wrong happens
    XSelectInput(ps->dpy, ev->window,
        determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN));
-    /*
-    // Check if the window is a client window of another
-    win *w_top = find_toplevel2(ps, ev->window);
-    if (w_top && !(w_top->client_win)) {
-      mark_client_win(ps, w_top, ev->window);
-    } */
+
+    // Check if the window is an undetected client window
+    // Firstly, check if it's a known client window
+    if (!find_toplevel(ps, ev->window)) {
+      // If not, look for its frame window
+      win *w_top = find_toplevel2(ps, ev->parent);
+      // If found, and its frame may not have a correct client, continue
+      if (w_top && w_top->client_win == w_top->id) {
+        // If it has WM_STATE, mark it the client window
+        if (wid_has_prop(ps, ev->window, ps->atom_client)) {
+          w_top->wmwin = false;
+          win_unmark_client(ps, w_top);
+          win_mark_client(ps, w_top, ev->window);
+        }
+        // Otherwise, watch for WM_STATE on it
+        else {
+          XSelectInput(ps->dpy, ev->window,
+              determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN)
+              | PropertyChangeMask);
+        }
+      }
+    }
  }
 }

@ -3059,18 +3150,10 @@ ev_expose(session_t *ps, XExposeEvent *ev) {
 */
 static void
 update_ewmh_active_win(session_t *ps) {
-  // Get the attribute firstly
-  winprop_t prop = wid_get_prop(ps, ps->root, ps->atom_ewmh_active_win,
-      1L, XA_WINDOW, 32);
-  if (!prop.nitems) {
-    free_winprop(&prop);
-    return;
-  }
-
  // Search for the window
-  Window wid = *prop.data.p32;
+  Window wid =
+    wid_get_prop_window(ps, ps->root, ps->atom_ewmh_active_win);
  win *w = NULL;
-  free_winprop(&prop);

  if (wid && !(w = find_toplevel(ps, wid)))
    if (!(w = find_win(ps, wid)))
@ -3107,6 +3190,24 @@ ev_property_notify(session_t *ps, XPropertyEvent *ev) {
    return;
  }

+  // If WM_STATE changes
+  if (ev->atom == ps->atom_client) {
+    // Check whether it could be a client window
+    if (!find_toplevel(ps, ev->window)) {
+      // Reset event mask anyway
+      XSelectInput(ps->dpy, ev->window,
+          determine_evmask(ps, ev->window, WIN_EVMODE_UNKNOWN));
+
+      win *w_top = find_toplevel2(ps, ev->window);
+      if (w_top && w_top->client_win == w_top->id
+          && wid_has_prop(ps, ev->window, ps->atom_client)) {
+        w_top->wmwin = false;
+        win_unmark_client(ps, w_top);
+        win_mark_client(ps, w_top, ev->window);
+      }
+    }
+  }
+
  // If _NET_WM_OPACITY changes
  if (ev->atom == ps->atom_opacity) {
    win *w = NULL;
--- a/src/compton.h
+++ b/src/compton.h
@ -58,7 +58,10 @@

 // For compatiblity with <libpcre-8.20
 #ifndef PCRE_STUDY_JIT_COMPILE
-#define PCRE_STUDY_JIT_COMPILE 0
+#define PCRE_STUDY_JIT_COMPILE    0
+#define LPCRE_FREE_STUDY(extra)   pcre_free(extra)
+#else
+#define LPCRE_FREE_STUDY(extra)   pcre_free_study(extra)
 #endif

 #endif
@ -1039,7 +1042,7 @@ free_wincond(wincond_t *cond) {
    free(cond->pattern);
 #ifdef CONFIG_REGEX_PCRE
  if (cond->regex_pcre_extra)
-    pcre_free_study(cond->regex_pcre_extra);
+    LPCRE_FREE_STUDY(cond->regex_pcre_extra);
  if (cond->regex_pcre)
    pcre_free(cond->regex_pcre);
 #endif
@ -1077,6 +1080,7 @@ free_win_res(session_t *ps, win *w) {
  free(w->name);
  free(w->class_instance);
  free(w->class_general);
+  free(w->role);
 }

 /**
@ -1149,15 +1153,15 @@ static inline bool is_normal_win(const win *w) {
 }

 /**
- * Determine if a window has a specific attribute.
+ * Determine if a window has a specific property.
 *
 * @param session_t current session
 * @param w window to check
- * @param atom atom of attribute to check
+ * @param atom atom of property to check
 * @return 1 if it has the attribute, 0 otherwise
 */
 static inline bool
-wid_has_attr(const session_t *ps, Window w, Atom atom) {
+wid_has_prop(const session_t *ps, Window w, Atom atom) {
  Atom type = None;
  int format;
  unsigned long nitems, after;
@ -1451,6 +1455,10 @@ win_update_focused(session_t *ps, win *w) {
 */
 static inline void
 win_set_focused(session_t *ps, win *w, bool focused) {
+  // Unmapped windows will have their focused state reset on map
+  if (IsUnmapped == w->a.map_state)
+    return;
+
  w->focused_real = focused;
  win_update_focused(ps, w);
 }
@ -1480,7 +1488,13 @@ static void
 calc_shadow_geometry(session_t *ps, win *w);

 static void
-mark_client_win(session_t *ps, win *w, Window client);
+win_mark_client(session_t *ps, win *w, Window client);
+
+static void
+win_unmark_client(session_t *ps, win *w);
+
+static void
+win_recheck_client(session_t *ps, win *w);

 static void
 add_win(session_t *ps, Window id, Window prev);
@ -1516,6 +1530,9 @@ static bool
 wid_get_text_prop(session_t *ps, Window wid, Atom prop,
    char ***pstrlst, int *pnstr);

+static Window
+wid_get_prop_window(session_t *ps, Window wid, Atom aprop);
+
 static bool
 wid_get_name(session_t *ps, Window w, char **name);