fix: guard tree rendering against destroyed window actors#507
fix: guard tree rendering against destroyed window actors#507braceyourself wants to merge 1 commit intoforge-ext:mainfrom
Conversation
If a window gets destroyed mid-render (app crash, rapid close, async cleanup), nodes in the tree still reference the finalized GObject. Accessing anything on it segfaults gnome-shell (signal 11). Adds isNodeValid() that probes the actor with get_name() in a try/catch (GJS throws on finalized GObjects rather than segfaulting). Filters dead nodes out before layout, and guards the remaining property accesses that can race with window destruction.
There was a problem hiding this comment.
Pull request overview
This PR hardens the window tree rendering/movement logic in the GNOME Shell extension against windows being destroyed mid-render (e.g., invalid/composited actors), aiming to prevent GJS exceptions during tab/decor rendering and move() application.
Changes:
- Add
Node.isNodeValid()to detect finalized window actors safely. - Skip processing/rendering for invalid window nodes during layout computation.
- Add defensive guards/
try/catcharound window moves, border width lookups, and tab actor attachment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip windows whose actors were destroyed mid-render | ||
| tiledChildren.filter((c) => c.isNodeValid()).forEach((child, index) => { | ||
| // A monitor can contain a window or container child | ||
| if (node.layout === LAYOUT_TYPES.HSPLIT || node.layout === LAYOUT_TYPES.VSPLIT) { |
There was a problem hiding this comment.
In processNode(), sizes is computed from the unfiltered tiledChildren, but the later loop filters with tiledChildren.filter((c) => c.isNodeValid()) and uses the filtered loop’s index to read params.sizes[index]. If any invalid nodes are filtered out, the indices no longer line up and split/tab/stack calculations will apply the wrong sizes to the remaining windows (and can leave stale decoration state when the filtered list becomes empty). Filter invalid nodes before computing sizes and set params.tiledChildren to that same filtered list so indexing and decoration logic stay consistent.
| let tabContents = new St.BoxLayout({ | ||
| style_class: "window-tabbed-tab", | ||
| x_expand: true, | ||
| }); | ||
| // Window tracker may not resolve an app for a dying window | ||
| let app = this.app; | ||
| if (!app) return; | ||
| let labelText = this._getTitle(); |
There was a problem hiding this comment.
The early return when this.app is missing happens after creating tabContents. When app is null (e.g., dying/untracked windows), this allocates an unused St.BoxLayout actor and immediately discards it. Consider checking this.app before creating any St widgets so you avoid unnecessary actor allocations on the hot path.
| this.extWm.move(metaWin, w.renderRect); | ||
| try { | ||
| this.extWm.move(metaWin, w.renderRect); | ||
| } catch (e) { |
There was a problem hiding this comment.
apply() now swallows all exceptions from this.extWm.move() with an empty catch. This makes unrelated failures (bad rects, API changes, etc.) silent and hard to diagnose. If the intention is to ignore “window destroyed mid-render” errors, consider (a) filtering invalid windows before calling move and/or (b) logging at least a debug/warn message (possibly rate-limited) inside the catch so unexpected errors are visible.
| } catch (e) { | |
| } catch (e) { | |
| Logger.debug( | |
| `extWm.move failed for window ${metaWin} with rect ${JSON.stringify( | |
| w.renderRect | |
| )}: ${e}` | |
| ); |
| // Border actor may be gone if the window was destroyed mid-render | ||
| let borderWidth = 0; | ||
| try { | ||
| if (child.actor?.border) { | ||
| borderWidth = child.actor.border.get_theme_node().get_border_width(St.Side.TOP); | ||
| } | ||
| } catch (e) { | ||
| } |
There was a problem hiding this comment.
The try/catch around child.actor.border.get_theme_node() has an empty catch block. If this is expected when windows are destroyed mid-render, please add at least a brief comment or debug-level log in the catch so unexpected theme/actor errors don’t get silently ignored (and consider narrowing the guarded code to only what can throw).
| if (child.tab && !decoration.contains(child.tab)) { | ||
| try { | ||
| decoration.add_child(child.tab); | ||
| } catch (e) { |
There was a problem hiding this comment.
The try/catch around decoration.add_child(child.tab) currently ignores all errors with an empty catch. This can hide real UI issues (e.g., actor already disposed for reasons other than window destruction). Consider logging at debug/warn (or adding a clarifying comment) so unexpected failures are diagnosable, or restructure to avoid calling add_child when the tab actor is no longer valid.
| } catch (e) { | |
| } catch (e) { | |
| Logger.debug(`Failed to add tab actor to decoration: ${e}`); |
Got kicked out of my entire GNOME session on Feb 20. No warning, just landed on an error page with a "log out" button.
journalctl -b 0told the story: signal 11 (SIGSEGV) in gnome-shell, then signal 6 (mutter assertion) right behind it. A double crash.tree.js- the recursiveprocessNode()traversal, line 1555.kill -9one of them, watch gnome-shell die on the next render cycle.child.actor.border.get_theme_node()accessing a dead window's actor. The render pipeline just assumes everything is alive. GJS doesn't stop you from touching a finalized GObject - it lets the access through to C, and C doesn't ask twice.Adds
isNodeValid()that probes the actor withget_name()in a try/catch (GJS throws on finalized GObjects rather than segfaulting). Filters dead nodes out before layout, and guards the remaining property accesses that can race with window destruction.Tested by opening 3 windows in tabbed layout,
kill -9ing one, and confirming gnome-shell stayed up with no SIGSEGV or forge errors injournalctl.