DYN-10167: Crash fix. Prevent close on Alt+F4 and hide instead.#16998
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-10167
|
It is weird that Alt+F4 is caught by the node autocomplete window rather than the Dynamo window itself, which usually what I would expect when pressing it. |
johnpierson
left a comment
johnpierson
left a comment
There was a problem hiding this comment.
lgtm, thanks for catching this
I think it is because our window is focusable for the keyboard shortcut interaction. |
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the Node AutoComplete bar window lifecycle by preventing user-initiated window closure (e.g., Alt+F4) from leaving behind a closed/reused window instance and dangling transient nodes.
Changes:
- Intercept
Window.Closingto cancel user-initiated close and hide the window while discarding transient nodes (same behavior as Escape). - Add
Window.Closedhandling to clear the static singleton instance and unsubscribe events to avoid callingShow()on a closed window. - Wrap programmatic
Close()during reset with an_allowCloseguard to bypass the close-cancellation behavior.
| { | ||
| if (_allowClose) | ||
| return; | ||
|
|
There was a problem hiding this comment.
OnNodeAutoCompleteBarClosing cancels all close attempts unless _allowClose is set. During application/host shutdown (e.g., owner window closing, dispatcher shutdown), this may prevent the window from closing and potentially keep the process alive. Consider allowing close when shutdown is in progress (e.g., ViewModel?.dynamoViewModel?.Model?.ShutdownRequested == true and/or Dispatcher.HasShutdownStarted / owner is closing) instead of always cancelling.
| // During application/host shutdown, allow the window to close normally so that | |
| // we do not keep the process alive by cancelling the close. | |
| var isShuttingDown = | |
| (Dispatcher != null && Dispatcher.HasShutdownStarted) || | |
| (ViewModel?.dynamoViewModel?.Model?.ShutdownRequested == true); | |
| if (isShuttingDown) | |
| return; |
| try | ||
| { | ||
| UnsubscribeFromAppEvents(); | ||
| } | ||
| catch | ||
| { | ||
| // ViewModel or model may be invalid during teardown. | ||
| } | ||
|
|
||
| try | ||
| { | ||
| if (ViewModel != null) | ||
| { | ||
| UnsubscribeFromOtherEvents(); | ||
| } | ||
| } | ||
| catch | ||
| { | ||
| // PortViewModel may already be null. | ||
| } |
There was a problem hiding this comment.
OnNodeAutoCompleteBarClosed has broad catch { } blocks around the unsubscribe logic. This can silently hide teardown problems and make diagnosing event-leak/crash issues harder. Consider catching Exception ex and logging (or at least tracing) the exception message, while still keeping shutdown resilient.
I think its because its a |
| /// (see <see cref="ResetNodeAutoCompleteBar"/>). Otherwise <see cref="PrepareAndShowNodeAutoCompleteBar"/> | ||
| /// would call <see cref="Window.Show"/> on a closed window and crash. | ||
| /// </summary> | ||
| private void OnNodeAutoCompleteBarClosed(object sender, EventArgs e) |
There was a problem hiding this comment.
I'm wondering if we really need this. When this hits we are already unsubscribed and the control instance will be set to null - we are doing all this in
ResetNodeAutoCompleteBar which is the only place where the real Close() happens programmatically.
So wouldn't allowClose plus handling on Closing be enough ? - that should cover the ALT+F4 and Closed will not be triggered in this scenario that escaped.
There was a problem hiding this comment.
Ah yes you're right, since we will only arrive here when programatically closing it's not necessary to have it duplicated, fixed.
| DataContext = viewModel; | ||
| InitializeComponent(); | ||
| SubscribeToAppEvents(); | ||
| Closing += OnNodeAutoCompleteBarClosing; |
|
Great find and fix! LGTM! |
|
(cherry picked from commit 45ba46d)
|
Successfully created backport PR for |
Purpose
Several crash reports indicate that Dynamo may crash when trying to
Showthe node autocomplete window. This happened because we did not consider that the user could close the autocomplete window by focusing it and pressing Alt+F4. since there is no UI to really close the window (we hide it for later reuse instead).Reproduction steps of the crash fixed by this ticket:
This PR fixes both bugs.
Declarations
Check these if you believe they are true
Release Notes
Fix crash when closing node autocomplete window with Alt+F4.
Reviewers
@johnpierson
@BogdanZavu
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of