-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Misc. elevation crash fixes #12205
Merged
Merged
Misc. elevation crash fixes #12205
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
dd213a5
not an actual fix for 12169, but a crash nonetheless
zadjii-msft ba49121
This was my prototyped fix for the crash, but uuhhhhggggggg
zadjii-msft 4487aa7
Probably should have been in the previous commit
zadjii-msft 0b18ae4
Don't create the window at all when autoelevating
zadjii-msft 4275c50
fix a crash on startup where we might not have yet loaded the settings
zadjii-msft ff72599
auto promote the first split to a tab in this scenario
zadjii-msft 9ddcdbc
dead code
zadjii-msft ab217a7
spel
zadjii-msft 581600e
Merge remote-tracking branch 'origin/main' into dev/migrie/b/12169-el…
zadjii-msft 062aff5
comments are good
zadjii-msft a50c3c4
not sure why this was missed in an earlier PR but I'm sneaking this i…
zadjii-msft File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -302,6 +302,114 @@ namespace winrt::TerminalApp::implementation | |
settings.GlobalSettings().FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout; | ||
} | ||
|
||
// Method Description: | ||
// - This is a bit of trickiness: If we're running unelevated, and the user | ||
// passed in only --elevate actions, the we don't _actually_ want to | ||
// restore the layouts here. We're not _actually_ about to create the | ||
// window. We're simply going to toss the commandlines | ||
// Arguments: | ||
// - <none> | ||
// Return Value: | ||
// - <none> | ||
bool TerminalPage::ShouldImmediatelyHandoffToElevated(CascadiaSettings& settings) const | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to need a mutable reference to |
||
{ | ||
if (!_startupActions || IsElevated()) | ||
{ | ||
// there arent startup actions, or we're elevated. In that case, go for it. | ||
return false; | ||
} | ||
|
||
// Check that there's at least one action that's not just an elevated newTab action. | ||
for (const auto& action : _startupActions) | ||
{ | ||
NewTerminalArgs newTerminalArgs{ nullptr }; | ||
|
||
if (action.Action() == ShortcutAction::NewTab) | ||
{ | ||
const auto& args{ action.Args().try_as<NewTabArgs>() }; | ||
if (args) | ||
{ | ||
newTerminalArgs = args.TerminalArgs(); | ||
} | ||
else | ||
{ | ||
// This was a nt action that didn't have any args. The default | ||
// profile may want to be elevated, so don't just early return. | ||
} | ||
} | ||
else if (action.Action() == ShortcutAction::SplitPane) | ||
{ | ||
const auto& args{ action.Args().try_as<SplitPaneArgs>() }; | ||
if (args) | ||
{ | ||
newTerminalArgs = args.TerminalArgs(); | ||
} | ||
else | ||
{ | ||
// This was a nt action that didn't have any args. The default | ||
// profile may want to be elevated, so don't just early return. | ||
} | ||
} | ||
else | ||
{ | ||
// This was not a new tab or split pane action. | ||
// This doesn't affect the outcome | ||
continue; | ||
} | ||
|
||
// It's possible that newTerminalArgs is null here. | ||
// GetProfileForArgs should be resilient to that. | ||
const auto profile{ settings.GetProfileForArgs(newTerminalArgs) }; | ||
if (profile.Elevate()) | ||
{ | ||
continue; | ||
} | ||
|
||
// The profile didn't want to be elevated, and we aren't elevated. | ||
// We're going to open at least one tab, so return false. | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
// Method Description: | ||
// - Escape hatch for immediately dispatching requests to elevated windows | ||
// when first launched. At this point in startup, the window doesn't exist | ||
// yet, XAML hasn't been started, but we need to dispatch these actions. | ||
// We can't just go through ProcessStartupActions, because that processes | ||
// the actions async using the XAML dispatcher (which doesn't exist yet) | ||
// - DON'T CALL THIS if you haven't already checked | ||
// ShouldImmediatelyHandoffToElevated. If you're thinking about calling | ||
// this outside of the one place it's used, that's probably the wrong | ||
// solution. | ||
// Arguments: | ||
// - settings: the settings we should use for dispatching these actions. At | ||
// this point in startup, we hadn't otherwise been initialized with these, | ||
// so use them now. | ||
// Return Value: | ||
// - <none> | ||
void TerminalPage::HandoffToElevated(CascadiaSettings& settings) | ||
{ | ||
if (!_startupActions) | ||
{ | ||
return; | ||
} | ||
|
||
// Hookup our event handlers to the ShortcutActionDispatch | ||
_settings = settings; | ||
_HookupKeyBindings(_settings.ActionMap()); | ||
_RegisterActionCallbacks(); | ||
|
||
for (const auto& action : _startupActions) | ||
{ | ||
// only process new tabs and split panes. They're all going to the elevated window anyways. | ||
if (action.Action() == ShortcutAction::NewTab || action.Action() == ShortcutAction::SplitPane) | ||
{ | ||
_actionDispatch->DoAction(action); | ||
} | ||
} | ||
} | ||
|
||
// Method Description; | ||
// - Checks if the current window is configured to load a particular layout | ||
// Arguments: | ||
|
@@ -1638,7 +1746,30 @@ namespace winrt::TerminalApp::implementation | |
std::shared_ptr<Pane> newPane) | ||
{ | ||
const auto focusedTab{ _GetFocusedTabImpl() }; | ||
_SplitPane(*focusedTab, splitDirection, splitSize, newPane); | ||
|
||
// Clever hack for a crash in startup, with multiple sub-commands. Say | ||
// you have the following commandline: | ||
// | ||
// wtd nt -p "elevated cmd" ; sp -p "elevated cmd" ; sp -p "Command Prompt" | ||
// | ||
// Where "elevated cmd" is an elevated profile. | ||
// | ||
// In that scenario, we won't dump off the commandline immediately to an | ||
// elevated window, because it's got the final unelevated split in it. | ||
// However, when we get to that command, there won't be a tab yet. So | ||
// we'd crash right about here. | ||
// | ||
// Instead, let's just promote this first split to be a tab instead. | ||
// Crash avoided, and we don't need to worry about inserting a new-tab | ||
// command in at the start. | ||
if (!focusedTab && _tabs.Size() == 0) | ||
{ | ||
_CreateNewTabFromPane(newPane); | ||
} | ||
else | ||
{ | ||
_SplitPane(*focusedTab, splitDirection, splitSize, newPane); | ||
} | ||
} | ||
|
||
// Method Description: | ||
|
This file contains 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
This file contains 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
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.
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.
Just so I get it: This returns true if we're not elevated but all relevant pane-spawning actions are elevated. Is that correct? Would such a comment be helpful for others in the future?