-
Notifications
You must be signed in to change notification settings - Fork 8.9k
Fix crash when canceling file picker in profile settings #19504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix crash when canceling file picker in profile settings #19504
Conversation
Wraps SetClientGuid, SetFileTypes, and related dialog setup calls in try-catch blocks to prevent unhandled exceptions when the user cancels the file selection dialog or when dialog configuration fails. Previously, if these operations threw an exception (which could happen during cancellation or in certain system states), it would crash the terminal. Now these operations are treated as non-fatal, allowing the dialog to still open even if some configuration steps fail. Fixes microsoft#19501
|
@microsoft-github-policy-service agree
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! A few notes:
- I've made some suggestions replacing simple
try-catchblocks withLOG_IF_FAILED. it's cleaner and makes them one line. - For the bigger
try-catchblocks, we probably want to block them together, but I think we could pull out the one forSetDefaultExtension(). Might be worth testing out to confirm though. - The PRs changes are all in the lambda for
OpenFilePicker()which callsFilePicker(). There's a bunch ofTHROW_IF_FAILEDin there, but looking through them, they all seem pretty essential. I'm thinking it needs to be updated with a methodtry-catchblock, and if we catch an error, return an empty hstring. That way, inProfiles_Base, the returned path would be empty and we just skip over setting the setting (andterminal/src/cascadia/TerminalSettingsEditor/Profiles_Base.cpp
Lines 131 to 134 in a8beb4c
if (!path.empty()) { _Profile.Commandline(path); } )terminal/src/cascadia/TerminalSettingsEditor/Profiles_Base.cpp
Lines 168 to 171 in a8beb4c
if (!folder.empty()) { _Profile.StartingDirectory(folder); } - We probably want to also update
Profiles_Base::Icon_Click()andAppearances::BackgroundImage_Click. Idk if there's any other file/folder/image pickers in the settings UI, but these are likely prone to the same issues you mentioned.
Hope this helps! Excited to see this land 😊
| try | ||
| { | ||
| THROW_IF_FAILED(dialog->SetClientGuid(clientGuidExecutables)); | ||
| } | ||
| CATCH_LOG(); // non-fatal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| try | |
| { | |
| THROW_IF_FAILED(dialog->SetClientGuid(clientGuidExecutables)); | |
| } | |
| CATCH_LOG(); // non-fatal | |
| LOG_IF_FAILED(dialog->SetClientGuid(clientGuidExecutables)); |
| try | ||
| { | ||
| THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedFileTypes), supportedFileTypes)); | ||
| THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed | ||
| THROW_IF_FAILED(dialog->SetDefaultExtension(L"exe;cmd;bat")); | ||
| } | ||
| CATCH_LOG(); // non-fatal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| try | |
| { | |
| THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedFileTypes), supportedFileTypes)); | |
| THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed | |
| THROW_IF_FAILED(dialog->SetDefaultExtension(L"exe;cmd;bat")); | |
| } | |
| CATCH_LOG(); // non-fatal | |
| try | |
| { | |
| THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedFileTypes), supportedFileTypes)); | |
| THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed | |
| } | |
| CATCH_LOG(); // non-fatal | |
| LOG_IF_FAILED(dialog->SetDefaultExtension(L"exe;cmd;bat")); |
I'm thinking we want to treat SetFileTypes() and SetFileTypeIndex() as a block, but we can probably pull out SetDefaultExtension() so that it can succeed independently. Also adding in the LOG_IF_FAILED so that it doesn't throw if it fails.
| try | ||
| { | ||
| THROW_IF_FAILED(dialog->SetClientGuid(clientGuidFolderPicker)); | ||
| } | ||
| CATCH_LOG(); // non-fatal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| try | |
| { | |
| THROW_IF_FAILED(dialog->SetClientGuid(clientGuidFolderPicker)); | |
| } | |
| CATCH_LOG(); // non-fatal | |
| LOG_IF_FAILED(dialog->SetClientGuid(clientGuidFolderPicker)); |
Wraps SetClientGuid, SetFileTypes, and related dialog setup calls in try-catch blocks to prevent unhandled exceptions when the user cancels the file selection dialog or when dialog configuration fails.
Previously, if these operations threw an exception (which could happen during cancellation or in certain system states), it would crash the terminal. Now these operations are treated as non-fatal, allowing the dialog to still open even if some configuration steps fail.
Fixes #19501
Fix crash when canceling file picker in profile settings
Summary
Fixes a crash that occurs when users cancel the file selection dialog while setting the Command Line executable or Starting Directory for a profile.
References
Closes #19501
Detailed Description
When adding a new profile and clicking the browse button to select a Command Line executable or Starting Directory, canceling the file dialog would cause the terminal to crash. This happened because some of the file dialog configuration calls could throw exceptions that weren't being caught.
Changes Made
Added proper exception handling to the
Commandline_ClickandStartingDirectory_Clickfunctions:SetClientGuid,SetFileTypes, and related calls in try-catch blocksSetDefaultFolderelsewhere in the codebaseValidation
PR Checklist