Skip to content

Commit 8c6542b

Browse files
ZviBaratzclaude
andcommitted
feat(ego-lint): add stage/messageTray actor leak detection (R-LIFE-22, R-LIFE-24)
R-LIFE-22 (blocking): Detects global.stage.add_child(), addTopChrome(), and addChrome() without matching removal or .destroy() in cleanup methods. Supersedes R-LIFE-19 (advisory pattern rule) with smarter Tier 2 check. R-LIFE-24 (advisory): Detects Main.messageTray.add() without matching .destroy() in cleanup methods. Both checks extract cleanup method bodies (disable/destroy/_destroy*/ onDestroy) and verify each tracked this._xxx variable has proper cleanup. Null assignment alone is NOT valid cleanup for stage actors. Closes #36 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 6c235c4 commit 8c6542b

File tree

11 files changed

+216
-22
lines changed

11 files changed

+216
-22
lines changed

field-tests/annotations/clipboard-indicator.yaml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,10 @@ findings:
7676
- id: "resource-tracking/ownership::orphan detected"
7777
classification: tp
7878
notes: "1 orphan detected — correct"
79-
# ego-review blocking issues
80-
- id: "lifecycle::_historyLabel on global.stage never removed"
79+
# stage/messageTray actor leaks (now detected by ego-lint)
80+
- id: "lifecycle/stage-actor-leak::this._historyLabel"
8181
classification: tp
82-
notes: "B1: Actor leak on every enable/disable cycle — ego-review finding"
83-
- id: "lifecycle::_notifSource never destroyed in disable()"
82+
notes: "B1: Actor leak on every enable/disable cycle — now detected by R-LIFE-22"
83+
- id: "lifecycle/messagetray-source-leak::this._notifSource"
8484
classification: tp
85-
notes: "B2: Notification source persists — ego-review finding"
85+
notes: "B2: Notification source persists — now detected by R-LIFE-24"

rules/patterns.yaml

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -790,14 +790,6 @@
790790
# --- Lifecycle (advisory) ---
791791
# Pattern-based lifecycle checks (complement script-based checks in check-lifecycle.py).
792792

793-
- id: R-LIFE-19
794-
pattern: "global\\.stage\\.add_child\\s*\\("
795-
scope: ["*.js"]
796-
severity: advisory
797-
message: "global.stage.add_child() — ensure matching remove_child() in disable()/destroy()"
798-
category: lifecycle
799-
fix: "In disable(), call global.stage.remove_child(this._actor); this._actor = null;"
800-
801793
# --- GNOME 44-46 migration (version-gated) ---
802794
# APIs removed or renamed in GNOME 44-46 transition to ESM.
803795

skills/ego-lint/references/rules-reference.md

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2049,13 +2049,22 @@ Rules for APIs removed or changed in specific GNOME Shell versions. These rules
20492049
- **Fix**: Add `this._session.abort()` before nullifying the session reference in `disable()`.
20502050
- **Tested by**: `tests/fixtures/soup-session-no-abort@test/`
20512051

2052-
### R-LIFE-19: global.stage.add_child() without matching remove_child()
2052+
### R-LIFE-22: Stage/chrome actor leak (add without remove)
2053+
- **Severity**: blocking
2054+
- **Checked by**: check-lifecycle.py
2055+
- **Rule**: `global.stage.add_child()`, `Main.layoutManager.addTopChrome()`, and `Main.layoutManager.addChrome()` must have a matching removal (`remove_child`/`removeTopChrome`/`removeChrome`) or `.destroy()` in `disable()`/`destroy()`. Only tracks `this._xxx` patterns.
2056+
- **Rationale**: Actors added to `global.stage` or the layout manager chrome persist on the compositor stage across enable/disable cycles. Unlike container widgets that are destroyed with their parent, stage children must be explicitly removed or they leak — causing visible artifacts and memory growth. Nulling the reference (`this._actor = null`) does NOT remove the actor from the stage.
2057+
- **Fix**: In `disable()`, call `global.stage.remove_child(this._actor); this._actor.destroy(); this._actor = null;` or just `this._actor.destroy()` (destroying a Clutter actor removes it from its parent).
2058+
- **Tested by**: `tests/fixtures/stage-add-child@test/`, `tests/fixtures/stage-add-child-clean@test/` (negative)
2059+
- **Supersedes**: R-LIFE-19 (advisory pattern rule)
2060+
2061+
### R-LIFE-24: MessageTray source leak (add without destroy)
20532062
- **Severity**: advisory
2054-
- **Checked by**: apply-patterns.py
2055-
- **Rule**: `global.stage.add_child()` must have a matching `remove_child()` in `disable()` or `destroy()`.
2056-
- **Rationale**: Actors added to `global.stage` persist on the compositor stage across enable/disable cycles. Unlike container widgets that are destroyed with their parent, stage children must be explicitly removed or they leak — causing visual artifacts and memory growth.
2057-
- **Fix**: In `disable()`, call `global.stage.remove_child(this._actor); this._actor = null;`.
2058-
- **Tested by**: `tests/fixtures/stage-add-child@test/`
2063+
- **Checked by**: check-lifecycle.py
2064+
- **Rule**: `Main.messageTray.add(this._source)` must have a matching `this._source.destroy()` in `disable()`/`destroy()`. Only tracks `this._xxx` patterns.
2065+
- **Rationale**: Notification sources added to the message tray persist if not explicitly destroyed. While lower impact than stage actor leaks (notifications expire and sources may be GC'd), they can accumulate across enable/disable cycles.
2066+
- **Fix**: In `disable()`, call `this._source.destroy(); this._source = null;`.
2067+
- **Tested by**: `tests/fixtures/messagetray-source-leak@test/`
20592068
20602069
### R-LIFE-20: Bus name ownership without release
20612070
- **Severity**: advisory

skills/ego-lint/scripts/check-lifecycle.py

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
- R-LIFE-18: Subprocess without cancellation in disable/destroy
2525
- R-LIFE-20: Bus name ownership without release
2626
- R-LIFE-21: GSettings signal leak (bare connect without disconnect)
27+
- R-LIFE-22: Stage actor leak (add_child/addChrome without remove)
28+
- R-LIFE-24: MessageTray source leak (add without destroy)
2729
- R-SEC-16: Clipboard + keybinding cross-reference
2830
- R-FILE-07: Missing export default class
2931
@@ -1010,6 +1012,137 @@ def check_widget_lifecycle(ext_dir):
10101012
f"All {len(created_widgets)} widget(s) properly cleaned up")
10111013

10121014

1015+
def check_stage_actor_lifecycle(ext_dir):
1016+
"""R-LIFE-22: Stage actor add without matching remove in disable()/destroy()."""
1017+
js_files = find_js_files(ext_dir, exclude_prefs=True)
1018+
if not js_files:
1019+
return
1020+
1021+
# Patterns that add actors to stage/chrome
1022+
add_patterns = [
1023+
(r'global\.stage\.add_child\s*\(\s*this\.(_\w+)', 'global.stage.remove_child'),
1024+
(r'Main\.layoutManager\.addTopChrome\s*\(\s*this\.(_\w+)', 'removeTopChrome'),
1025+
(r'Main\.layoutManager\.addChrome\s*\(\s*this\.(_\w+)', 'removeChrome'),
1026+
]
1027+
1028+
added_actors = [] # (var_name, file_rel, lineno, remove_method)
1029+
1030+
for filepath in js_files:
1031+
content = strip_comments(read_file(filepath))
1032+
rel = os.path.relpath(filepath, ext_dir)
1033+
for lineno, line in enumerate(content.splitlines(), 1):
1034+
for add_re, remove_method in add_patterns:
1035+
m = re.search(add_re, line)
1036+
if m:
1037+
added_actors.append((m.group(1), rel, lineno, remove_method))
1038+
1039+
if not added_actors:
1040+
return
1041+
1042+
# Extract cleanup method bodies across all files
1043+
cleanup_re = re.compile(
1044+
r'\b(?:disable|destroy|_destroy\w*|onDestroy)\s*\(')
1045+
1046+
cleanup_text = ''
1047+
for filepath in js_files:
1048+
content = strip_comments(read_file(filepath))
1049+
lines = content.splitlines()
1050+
i = 0
1051+
while i < len(lines):
1052+
if cleanup_re.search(lines[i]):
1053+
depth = 0
1054+
started = False
1055+
for j in range(i, len(lines)):
1056+
depth += lines[j].count('{') - lines[j].count('}')
1057+
if '{' in lines[j]:
1058+
started = True
1059+
cleanup_text += lines[j] + '\n'
1060+
if started and depth <= 0:
1061+
break
1062+
i = j + 1 if started else i + 1
1063+
else:
1064+
i += 1
1065+
1066+
leaked = []
1067+
for var_name, rel, lineno, remove_method in added_actors:
1068+
escaped = re.escape(var_name)
1069+
has_remove = bool(re.search(
1070+
rf'{re.escape(remove_method)}\s*\(\s*this\.{escaped}', cleanup_text))
1071+
has_destroy = bool(re.search(
1072+
rf'this\.{escaped}\.destroy\s*\(', cleanup_text))
1073+
if not has_remove and not has_destroy:
1074+
leaked.append(f"this.{var_name} ({rel}:{lineno})")
1075+
1076+
if leaked:
1077+
result("FAIL", "lifecycle/stage-actor-leak",
1078+
f"Actor(s) added to stage/chrome but not removed in "
1079+
f"disable()/destroy(): {', '.join(leaked[:5])}")
1080+
else:
1081+
result("PASS", "lifecycle/stage-actor-leak",
1082+
f"All {len(added_actors)} stage/chrome actor(s) properly cleaned up")
1083+
1084+
1085+
def check_message_tray_lifecycle(ext_dir):
1086+
"""R-LIFE-24: MessageTray source added without destroy in disable()/destroy()."""
1087+
js_files = find_js_files(ext_dir, exclude_prefs=True)
1088+
if not js_files:
1089+
return
1090+
1091+
add_re = re.compile(r'Main\.messageTray\.add\s*\(\s*this\.(_\w+)')
1092+
added_sources = [] # (var_name, file_rel, lineno)
1093+
1094+
for filepath in js_files:
1095+
content = strip_comments(read_file(filepath))
1096+
rel = os.path.relpath(filepath, ext_dir)
1097+
for lineno, line in enumerate(content.splitlines(), 1):
1098+
m = add_re.search(line)
1099+
if m:
1100+
added_sources.append((m.group(1), rel, lineno))
1101+
1102+
if not added_sources:
1103+
return
1104+
1105+
# Extract cleanup method bodies across all files
1106+
cleanup_re = re.compile(
1107+
r'\b(?:disable|destroy|_destroy\w*|onDestroy)\s*\(')
1108+
1109+
cleanup_text = ''
1110+
for filepath in js_files:
1111+
content = strip_comments(read_file(filepath))
1112+
lines = content.splitlines()
1113+
i = 0
1114+
while i < len(lines):
1115+
if cleanup_re.search(lines[i]):
1116+
depth = 0
1117+
started = False
1118+
for j in range(i, len(lines)):
1119+
depth += lines[j].count('{') - lines[j].count('}')
1120+
if '{' in lines[j]:
1121+
started = True
1122+
cleanup_text += lines[j] + '\n'
1123+
if started and depth <= 0:
1124+
break
1125+
i = j + 1 if started else i + 1
1126+
else:
1127+
i += 1
1128+
1129+
leaked = []
1130+
for var_name, rel, lineno in added_sources:
1131+
escaped = re.escape(var_name)
1132+
has_destroy = bool(re.search(
1133+
rf'this\.{escaped}\.destroy\s*\(', cleanup_text))
1134+
if not has_destroy:
1135+
leaked.append(f"this.{var_name} ({rel}:{lineno})")
1136+
1137+
if leaked:
1138+
result("WARN", "lifecycle/messagetray-source-leak",
1139+
f"MessageTray source(s) added but not destroyed in "
1140+
f"disable()/destroy(): {', '.join(leaked[:5])}")
1141+
else:
1142+
result("PASS", "lifecycle/messagetray-source-leak",
1143+
f"All {len(added_sources)} messageTray source(s) properly cleaned up")
1144+
1145+
10131146
def check_gsettings_signal_leak(ext_dir):
10141147
"""R-LIFE-21: Bare settings.connect() without stored ID is a guaranteed leak."""
10151148
js_files = find_js_files(ext_dir, exclude_prefs=True)
@@ -1123,6 +1256,8 @@ def main():
11231256
check_soup_session_abort(ext_dir)
11241257
check_destroy_then_null(ext_dir)
11251258
check_widget_lifecycle(ext_dir)
1259+
check_stage_actor_lifecycle(ext_dir)
1260+
check_message_tray_lifecycle(ext_dir)
11261261
check_gsettings_signal_leak(ext_dir)
11271262
check_settings_cleanup(ext_dir)
11281263

tests/assertions/field-test-improvements.sh

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,25 @@ assert_exit_code "exits with 0 (no FAIL)" 0
4343
assert_output_not_contains "no constructor-resources WARN for prefs.js" "\[WARN\].*quality/constructor-resources"
4444
echo ""
4545

46-
# --- stage-add-child (R-LIFE-19) ---
46+
# --- stage-add-child (R-LIFE-22) ---
4747
echo "=== stage-add-child ==="
4848
run_lint "stage-add-child@test"
49-
assert_exit_code "exits with 0 (advisory only)" 0
50-
assert_output_contains "warns on global.stage.add_child" "\[WARN\].*R-LIFE-19"
49+
assert_exit_code "exits with 1 (FAIL — stage actor leak)" 1
50+
assert_output_contains "fails on stage actor leak" "\[FAIL\].*lifecycle/stage-actor-leak"
51+
echo ""
52+
53+
# --- messagetray-source-leak (R-LIFE-24) ---
54+
echo "=== messagetray-source-leak ==="
55+
run_lint "messagetray-source-leak@test"
56+
assert_exit_code "exits with 0 (WARN only)" 0
57+
assert_output_contains "warns on messageTray source leak" "\[WARN\].*lifecycle/messagetray-source-leak"
58+
echo ""
59+
60+
# --- stage-add-child-clean (R-LIFE-22 negative) ---
61+
echo "=== stage-add-child-clean ==="
62+
run_lint "stage-add-child-clean@test"
63+
assert_exit_code "exits with 0 (clean)" 0
64+
assert_output_not_contains "no stage actor leak" "lifecycle/stage-actor-leak.*FAIL"
5165
echo ""
5266

5367
# --- shell-keybinding-mode (R-DEPR-11) ---
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
SPDX-License-Identifier: GPL-2.0-or-later
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import * as Main from 'resource:///org/gnome/shell/ui/main.js';
2+
import {Extension} from 'resource:///org/gnome/shell/extensions/extension.js';
3+
import * as MessageTray from 'resource:///org/gnome/shell/ui/messageTray.js';
4+
5+
export default class MessageTrayLeakExtension extends Extension {
6+
enable() {
7+
this._source = new MessageTray.Source({title: 'Test'});
8+
Main.messageTray.add(this._source);
9+
}
10+
11+
disable() {
12+
this._source = null;
13+
}
14+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"uuid": "messagetray-source-leak@test",
3+
"name": "MessageTray Source Leak Test",
4+
"description": "Test fixture for R-LIFE-24",
5+
"shell-version": ["45"],
6+
"url": ""
7+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
SPDX-License-Identifier: GPL-2.0-or-later
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import St from 'gi://St';
2+
import {Extension} from 'resource:///org/gnome/shell/extensions/extension.js';
3+
4+
export default class StageChildCleanExtension extends Extension {
5+
enable() {
6+
this._label = new St.Label({text: 'Hello'});
7+
global.stage.add_child(this._label);
8+
}
9+
10+
disable() {
11+
global.stage.remove_child(this._label);
12+
this._label = null;
13+
}
14+
}

0 commit comments

Comments
 (0)