DYN-9903: Avoid using closed Window as EditWindow owner#17014
DYN-9903: Avoid using closed Window as EditWindow owner#17014zeusongit merged 4 commits intoDynamoDS:masterfrom
Conversation
Guard against assigning a closed Window as the owner for EditWindow and ensure DynamoViewModel.Owner is restored when modal child windows close. Add a null/IsLoaded check in EditWindow constructor before setting Owner; reset dynamoViewModel.Owner back to the main DynamoView when Package Manager or Preferences close. Include regression tests that reproduce the stale-owner condition and verify EditWindow construction does not throw and the owner is restored. Files changed: EditWindow.xaml.cs, DynamoView.xaml.cs, PreferencesView.xaml.cs, and NodeViewTests.cs (tests added/updated).
![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-9903
There was a problem hiding this comment.
Pull request overview
Fixes a WPF crash caused by DynamoViewModel.Owner referencing a closed child window (Package Manager / Preferences), which could throw when constructing EditWindow (e.g., during node rename / note edit).
Changes:
- Restore
DynamoViewModel.Ownerback to the mainDynamoViewwhen the Package Manager window closes. - Restore
DynamoViewModel.Ownerwhen Preferences closes, and hardenEditWindowto avoid assigning a closed window asOwner. - Add regression tests intended to reproduce the stale-owner crash and validate the owner-restoration contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/DynamoCoreWpf3Tests/NodeViewTests.cs | Adds regression tests around stale/closed window owner behavior. |
| src/DynamoCoreWpf/Views/Menu/PreferencesView.xaml.cs | Restores dynViewModel.Owner when Preferences closes via the close button handler. |
| src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs | Restores dynamoViewModel.Owner when Package Manager closes. |
| src/DynamoCoreWpf/UI/Prompts/EditWindow.xaml.cs | Guards Owner assignment to avoid InvalidOperationException when owner window is closed. |
EditWindow: avoid setting Owner to a closed window and choose an appropriate WindowStartupLocation. If dynamoViewModel.Owner is not visible, fall back to Application.Current.MainWindow when available, otherwise CenterScreen. PreferencesView: subscribe to Closed to restore dynViewModel.Owner back to the main window regardless of how the window was closed; ensure proper cleanup and Close behavior. Tests: update NodeViewTests to simulate the stale-owner scenario more robustly, assert EditWindow construction does not throw, and restore the original owner in a finally block to avoid leaking state. Adjust owner-restoration regression test to exercise the Closed handler and ensure Dispatcher events are processed. These changes prevent InvalidOperationException when assigning a closed Window as owner and make owner-restoration reliable across UI flows.
| } | ||
| else | ||
| { | ||
| this.WindowStartupLocation = WindowStartupLocation.CenterScreen; |
There was a problem hiding this comment.
When will this condition be true?
There was a problem hiding this comment.
it was added from this Copilot comment: #17014 (comment)
but seems like the else block is pretty much dead code, will remove.
| private void EditWindow_Closing(object sender, System.ComponentModel.CancelEventArgs e) | ||
| { | ||
| try { this.DialogResult = false; } | ||
| catch (InvalidOperationException) { } |
There was a problem hiding this comment.
I thought your fix was to avoid/fix the exception. If so, why is this catch needed?
There was a problem hiding this comment.
yes, the original fix avoids the owner exception, the try/catch here handles a separate pre-existing issue in the Closing handler that the test exposed as a side effect, when working on this
|
Purpose
Crash Fix
dynamoViewModel.Owneris a mutable property that gets temporarily redirected to child windows (Package Manager, Preferences) so dialogs opened from within those windows are centered correctly. Neither window restored the owner back to the main DynamoView on close, leaving a stale reference to a closed window. Any subsequentEditWindowconstruction (node rename, note edit) would crash withInvalidOperationException: Cannot set owner to a Window that has been closed.Added 2 tests
WhenDynamoViewModelOwnerIsClosedWindowThenEditWindowDoesNotThrowWhenChildWindowClosesOwnerIsRestoredToDynamoViewDeclarations
Check these if you believe they are true
Release Notes
Fixes a crash (InvalidOperationException) when renaming a node after the Package Manager or Preferences window had been opened and closed.