Dispose every dialog in a finally, not on the last line - #33
Merged
Merged
Conversation
All six modal sites constructed a view, ran it, and disposed it on the last
line of the method, with nothing in between protected. If anything there threw
— building the view, adding a button, taking focus, running the modal — the
Dispose was skipped and the whole view subtree leaked for the life of the
session.
Not purely theoretical: dialog layout is known to throw here. A MessageBox
dies with ArgumentOutOfRangeException: width ('-3') when the driver has not
learned the terminal size, recorded in the PTY harness notes after it was
mistaken for an app bug. A terminal that fails to answer a size query is
exactly the situation where fux would then also leak every dialog opened.
Code scanning raised this as cs/dispose-not-called-on-throw (alert 1050)
against StartSnippets. That alert is correct — unlike the eleven false
positives dismissed alongside it during the #17 triage — but it names one site
of six, because the scanner only sees changed lines. All six are fixed in one
pass: fixing only the flagged one would leave the codebase less consistent
than it was, which is why it was left alone at the time rather than patched
piecemeal.
The convention is written down on RunModal, where whoever adds a seventh
dialog will be looking, rather than repeated six times.
Behaviour is unchanged by construction: the results are still read into locals
inside the try, before the view is torn down, and every fixture drills to the
same check count as before (405/409/396/383/388, all green). No new drill
check comes with this — there is nothing observable to assert without
fabricating a throw, and a check that cannot fail would be worse than none.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016X4pgrsMBcsXjqWDo2gBBt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try | ||
| { | ||
| d.Add(new Label { X = 1, Y = 0, Text = "New name:" }, field); | ||
| d.AddButton(new Button { Text = "Cancel" }); // Result 0 |
| { | ||
| d.Add(new Label { X = 1, Y = 0, Text = "New name:" }, field); | ||
| d.AddButton(new Button { Text = "Cancel" }); // Result 0 | ||
| d.AddButton(new Button { Text = "OK" }); // Result 1; last added = default (Enter) |
| { | ||
| d.HotKeySpecifier = NoHotKey; // the title carries a node name — see NoHotKey | ||
| d.Add(new Label { X = 1, Y = 0, Text = "Name (element/attribute/PI):" }, field, kindSel, posSel); | ||
| d.AddButton(new Button { Text = "Cancel" }); // Result 0 |
| d.HotKeySpecifier = NoHotKey; // the title carries a node name — see NoHotKey | ||
| d.Add(new Label { X = 1, Y = 0, Text = "Name (element/attribute/PI):" }, field, kindSel, posSel); | ||
| d.AddButton(new Button { Text = "Cancel" }); // Result 0 | ||
| d.AddButton(new Button { Text = "OK" }); // Result 1; last added = default (Enter) |
|
|
||
| // Enter on the list commits, so the whole gesture is ^B, arrow, Enter — no Tab, | ||
| // no button, no mouse. Bound explicitly: 2.4.17 has no OpenSelectedItem event, and | ||
| // Accept does NOT bubble from a ListView to the dialog's default button (drilled — |
| // Enter on the list commits, so the whole gesture is ^B, arrow, Enter — no Tab, | ||
| // no button, no mouse. Bound explicitly: 2.4.17 has no OpenSelectedItem event, and | ||
| // Accept does NOT bubble from a ListView to the dialog's default button (drilled — | ||
| // the assumption that it did failed the check that pins this). |
| field.MoveEnd(); | ||
| RunModal(ui, d); | ||
|
|
||
| ok = d.Result is 1; |
| RunModal(ui, d); | ||
|
|
||
| ok = d.Result is 1; | ||
| expr = field.Text; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #25.
All six modal sites constructed a view, ran it, and disposed it on the last line of the method,
with nothing in between protected. If anything there threw — building the view, adding a button,
taking focus, running the modal — the
Disposewas skipped and the whole view subtree leaked forthe life of the session.
Not purely theoretical. Dialog layout is known to throw in this codebase: a
MessageBoxdieswith
ArgumentOutOfRangeException: width ('-3')when the driver has not learned the terminalsize, recorded in the PTY harness notes after it was mistaken for an app bug. A terminal that
fails to answer a size query is exactly the situation where fux would then also leak every dialog
the user opens.
StartRenameDialogStartInsertDialogStartSnippetsDialog← the alertStartFindDialogStartOpenOpenDialogStartSaveAsSaveDialogCode scanning raised this as
cs/dispose-not-called-on-throw(alert 1050) againstStartSnippets. That alert is correct — unlike the eleven false positives dismissed alongside itduring the #17 triage — but it names one site of six, because the scanner only sees changed
lines. All six are fixed in one pass, as the issue asked: fixing only the flagged one would leave
the codebase less consistent than it was.
The convention is written down once on
RunModal, where whoever adds a seventh dialog will belooking, rather than repeated six times.
Verification
Behaviour is unchanged by construction — the results are still read into locals inside the
try,before the view is torn down. Every fixture drills to the same check count as before:
405 / 409 / 396 / 383 / 388, all PASS. That the numbers did not move is the point.
No new drill check comes with this. There is nothing observable to assert without fabricating
a throw, and a check that cannot fail would be worse than none. The fix is structural: verified
by inspection, by the build, and by CodeQL on this PR, which is where alert 1050 should clear.
🤖 Generated with Claude Code
https://claude.ai/code/session_016X4pgrsMBcsXjqWDo2gBBt