Fix a crash on exit with the command palette open #13778
Conversation
Fixes MSFT:38775539 Might also fix MSFT:38614563 Looking at this code should be pretty clear what's going on. On exit, the XAML root is already nulled out. But here, we're just yolo'ing and assuming it exists (why wouldn't it). So yea. This is like weirdly high percent of crashes internally, but that's not from real users. Real users, I suspect hit this as like .3% of our crashes. Not zero, but _low_. * [x] tested manually
| @@ -474,7 +474,13 @@ namespace winrt::TerminalApp::implementation | |||
| return; | |||
| } | |||
|
|
|||
| auto focusedElementOrAncestor = Input::FocusManager::GetFocusedElement(this->XamlRoot()).try_as<DependencyObject>(); | |||
| auto root = this->XamlRoot(); | |||
| if (root == nullptr) | |||
There was a problem hiding this comment.
Why not just write if (!root) ("If no root")? I personally find it more readable.
BTW I believe this kind of expression also constructs an entire XamlRoot object (albeit empty/nullptr) for the comparison.
There was a problem hiding this comment.
The compiler almost certainly sees right through that, right?
There was a problem hiding this comment.
Yeah you're right about that. The only advantage is in debug builds then (avoiding the construction of an IUnknown).
|
@msftbot merge this in 5 minutes |
ghost
added
the
AutoMerge
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
DHowett
changed the title
ghost
merged commit 
Fixes MSFT:38775539 Might also fix MSFT:38614563 Looking at this code should be pretty clear what's going on. On exit, the XAML root is already nulled out. But here, we're just yolo'ing and assuming it exists (why wouldn't it). So yea. This is like weirdly high percent of crashes internally, but that's not from real users. Real users, I suspect hit this as like .3% of our crashes. Not zero, but _low_. * [x] tested manually <hr> May also be related to... * MSFT:40602905 * MSFT:40602904 * MSFT:40412800 * MSFT:35213459 <---has links (cherry picked from commit 64bcc0b) Service-Card-Id: 85486788 Service-Version: 1.15
|
🎉 Handy links: |
|
🎉 Handy links: |
Fixes MSFT:38775539
Might also fix MSFT:38614563
Looking at this code should be pretty clear what's going on. On exit, the XAML root is already nulled out. But here, we're just yolo'ing and assuming it exists (why wouldn't it). So yea. This is like weirdly high percent of crashes internally, but that's not from real users. Real users, I suspect hit this as like .3% of our crashes. Not zero, but low.
May also be related to...