Skip to content

Commit 43a275c

Browse files
authored
Misc. elevation crash fixes (#12205)
This is a collection of fixes: * dd213a5: This was a crash I discovered while investigating. Probably not the root cause crash, but a crash nonetheless. * ba49121...0b18ae4: A collection of fixes to _not_ create the window when we're about to handoff each of the new tabs, panes, to an elevated window. That should prevent us from starting up XAML at all, which should take care of #12169. Additionally, it'll prevent us from restoring the unelevated windows, which should resolve #12190 * The remainder of the commits where fixes for other weird edge cases as a part of this. Notably: * ff72599: Autopromote the first `split-pane` to a new tab, in the case that it's preceded with only `new-tab` actions that opened elevated windows. #### checklist * [x] I work here * [x] Docs are fine * [x] Closes #12190 * [x] Closes #12169
1 parent 5258fea commit 43a275c

File tree

8 files changed

+179
-4
lines changed

8 files changed

+179
-4
lines changed

.github/actions/spelling/allow/allow.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ libfuzzer
4242
liga
4343
lje
4444
Llast
45+
llvm
4546
Lmid
4647
locl
4748
lorem

src/cascadia/Remoting/WindowManager.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
252252

253253
if (_peasant)
254254
{
255-
// Inform the monarch of the time we were last activated
256-
_monarch.HandleActivatePeasant(_peasant.GetLastActivatedArgs());
255+
if (const auto& lastActivated{ _peasant.GetLastActivatedArgs() })
256+
{
257+
// Inform the monarch of the time we were last activated
258+
_monarch.HandleActivatePeasant(lastActivated);
259+
}
257260
}
258261

259262
if (!_isKing)

src/cascadia/TerminalApp/AppLogic.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1472,6 +1472,24 @@ namespace winrt::TerminalApp::implementation
14721472
return _root != nullptr ? _root->ShouldUsePersistedLayout(_settings) : false;
14731473
}
14741474

1475+
bool AppLogic::ShouldImmediatelyHandoffToElevated()
1476+
{
1477+
if (!_loadedInitialSettings)
1478+
{
1479+
// Load settings if we haven't already
1480+
LoadSettings();
1481+
}
1482+
1483+
return _root != nullptr ? _root->ShouldImmediatelyHandoffToElevated(_settings) : false;
1484+
}
1485+
void AppLogic::HandoffToElevated()
1486+
{
1487+
if (_root)
1488+
{
1489+
_root->HandoffToElevated(_settings);
1490+
}
1491+
}
1492+
14751493
void AppLogic::SaveWindowLayoutJsons(const Windows::Foundation::Collections::IVector<hstring>& layouts)
14761494
{
14771495
std::vector<WindowLayout> converted;

src/cascadia/TerminalApp/AppLogic.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ namespace winrt::TerminalApp::implementation
8181
bool AlwaysOnTop() const;
8282

8383
bool ShouldUsePersistedLayout();
84+
bool ShouldImmediatelyHandoffToElevated();
85+
void HandoffToElevated();
8486
hstring GetWindowLayoutJson(Microsoft::Terminal::Settings::Model::LaunchPosition position);
8587
void SaveWindowLayoutJsons(const Windows::Foundation::Collections::IVector<hstring>& layouts);
8688
void IdentifyWindow();

src/cascadia/TerminalApp/AppLogic.idl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ namespace TerminalApp
9393
TaskbarState TaskbarState{ get; };
9494

9595
Boolean ShouldUsePersistedLayout();
96+
Boolean ShouldImmediatelyHandoffToElevated();
97+
void HandoffToElevated();
9698
String GetWindowLayoutJson(Microsoft.Terminal.Settings.Model.LaunchPosition position);
9799
void SaveWindowLayoutJsons(Windows.Foundation.Collections.IVector<String> layouts);
98100

src/cascadia/TerminalApp/TerminalPage.cpp

Lines changed: 132 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,114 @@ namespace winrt::TerminalApp::implementation
302302
settings.GlobalSettings().FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout;
303303
}
304304

305+
// Method Description:
306+
// - This is a bit of trickiness: If we're running unelevated, and the user
307+
// passed in only --elevate actions, the we don't _actually_ want to
308+
// restore the layouts here. We're not _actually_ about to create the
309+
// window. We're simply going to toss the commandlines
310+
// Arguments:
311+
// - <none>
312+
// Return Value:
313+
// - true if we're not elevated but all relevant pane-spawning actions are elevated
314+
bool TerminalPage::ShouldImmediatelyHandoffToElevated(const CascadiaSettings& settings) const
315+
{
316+
if (!_startupActions || IsElevated())
317+
{
318+
// there arent startup actions, or we're elevated. In that case, go for it.
319+
return false;
320+
}
321+
322+
// Check that there's at least one action that's not just an elevated newTab action.
323+
for (const auto& action : _startupActions)
324+
{
325+
NewTerminalArgs newTerminalArgs{ nullptr };
326+
327+
if (action.Action() == ShortcutAction::NewTab)
328+
{
329+
const auto& args{ action.Args().try_as<NewTabArgs>() };
330+
if (args)
331+
{
332+
newTerminalArgs = args.TerminalArgs();
333+
}
334+
else
335+
{
336+
// This was a nt action that didn't have any args. The default
337+
// profile may want to be elevated, so don't just early return.
338+
}
339+
}
340+
else if (action.Action() == ShortcutAction::SplitPane)
341+
{
342+
const auto& args{ action.Args().try_as<SplitPaneArgs>() };
343+
if (args)
344+
{
345+
newTerminalArgs = args.TerminalArgs();
346+
}
347+
else
348+
{
349+
// This was a nt action that didn't have any args. The default
350+
// profile may want to be elevated, so don't just early return.
351+
}
352+
}
353+
else
354+
{
355+
// This was not a new tab or split pane action.
356+
// This doesn't affect the outcome
357+
continue;
358+
}
359+
360+
// It's possible that newTerminalArgs is null here.
361+
// GetProfileForArgs should be resilient to that.
362+
const auto profile{ settings.GetProfileForArgs(newTerminalArgs) };
363+
if (profile.Elevate())
364+
{
365+
continue;
366+
}
367+
368+
// The profile didn't want to be elevated, and we aren't elevated.
369+
// We're going to open at least one tab, so return false.
370+
return false;
371+
}
372+
return true;
373+
}
374+
375+
// Method Description:
376+
// - Escape hatch for immediately dispatching requests to elevated windows
377+
// when first launched. At this point in startup, the window doesn't exist
378+
// yet, XAML hasn't been started, but we need to dispatch these actions.
379+
// We can't just go through ProcessStartupActions, because that processes
380+
// the actions async using the XAML dispatcher (which doesn't exist yet)
381+
// - DON'T CALL THIS if you haven't already checked
382+
// ShouldImmediatelyHandoffToElevated. If you're thinking about calling
383+
// this outside of the one place it's used, that's probably the wrong
384+
// solution.
385+
// Arguments:
386+
// - settings: the settings we should use for dispatching these actions. At
387+
// this point in startup, we hadn't otherwise been initialized with these,
388+
// so use them now.
389+
// Return Value:
390+
// - <none>
391+
void TerminalPage::HandoffToElevated(const CascadiaSettings& settings)
392+
{
393+
if (!_startupActions)
394+
{
395+
return;
396+
}
397+
398+
// Hookup our event handlers to the ShortcutActionDispatch
399+
_settings = settings;
400+
_HookupKeyBindings(_settings.ActionMap());
401+
_RegisterActionCallbacks();
402+
403+
for (const auto& action : _startupActions)
404+
{
405+
// only process new tabs and split panes. They're all going to the elevated window anyways.
406+
if (action.Action() == ShortcutAction::NewTab || action.Action() == ShortcutAction::SplitPane)
407+
{
408+
_actionDispatch->DoAction(action);
409+
}
410+
}
411+
}
412+
305413
// Method Description;
306414
// - Checks if the current window is configured to load a particular layout
307415
// Arguments:
@@ -1638,7 +1746,30 @@ namespace winrt::TerminalApp::implementation
16381746
std::shared_ptr<Pane> newPane)
16391747
{
16401748
const auto focusedTab{ _GetFocusedTabImpl() };
1641-
_SplitPane(*focusedTab, splitDirection, splitSize, newPane);
1749+
1750+
// Clever hack for a crash in startup, with multiple sub-commands. Say
1751+
// you have the following commandline:
1752+
//
1753+
// wtd nt -p "elevated cmd" ; sp -p "elevated cmd" ; sp -p "Command Prompt"
1754+
//
1755+
// Where "elevated cmd" is an elevated profile.
1756+
//
1757+
// In that scenario, we won't dump off the commandline immediately to an
1758+
// elevated window, because it's got the final unelevated split in it.
1759+
// However, when we get to that command, there won't be a tab yet. So
1760+
// we'd crash right about here.
1761+
//
1762+
// Instead, let's just promote this first split to be a tab instead.
1763+
// Crash avoided, and we don't need to worry about inserting a new-tab
1764+
// command in at the start.
1765+
if (!focusedTab && _tabs.Size() == 0)
1766+
{
1767+
_CreateNewTabFromPane(newPane);
1768+
}
1769+
else
1770+
{
1771+
_SplitPane(*focusedTab, splitDirection, splitSize, newPane);
1772+
}
16421773
}
16431774

16441775
// Method Description:

src/cascadia/TerminalApp/TerminalPage.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ namespace winrt::TerminalApp::implementation
6464
void Create();
6565

6666
bool ShouldUsePersistedLayout(Microsoft::Terminal::Settings::Model::CascadiaSettings& settings) const;
67+
bool ShouldImmediatelyHandoffToElevated(const Microsoft::Terminal::Settings::Model::CascadiaSettings& settings) const;
68+
void HandoffToElevated(const Microsoft::Terminal::Settings::Model::CascadiaSettings& settings);
6769
std::optional<uint32_t> LoadPersistedLayoutIdx(Microsoft::Terminal::Settings::Model::CascadiaSettings& settings) const;
6870
winrt::Microsoft::Terminal::Settings::Model::WindowLayout LoadPersistedLayout(Microsoft::Terminal::Settings::Model::CascadiaSettings& settings) const;
6971
Microsoft::Terminal::Settings::Model::WindowLayout GetWindowLayout();

src/cascadia/WindowsTerminal/AppHost.cpp

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,20 @@ void AppHost::_HandleCommandlineArgs()
214214
}
215215
}
216216

217+
// This is a fix for GH#12190 and hopefully GH#12169.
218+
//
219+
// If the commandline we were provided is going to result in us only
220+
// opening elevated terminal instances, then we need to not even create
221+
// the window at all here. In that case, we're going through this
222+
// special escape hatch to dispatch all the calls to elevate-shim, and
223+
// then we're going to exit immediately.
224+
if (_logic.ShouldImmediatelyHandoffToElevated())
225+
{
226+
_shouldCreateWindow = false;
227+
_logic.HandoffToElevated();
228+
return;
229+
}
230+
217231
// After handling the initial args, hookup the callback for handling
218232
// future commandline invocations. When our peasant is told to execute a
219233
// commandline (in the future), it'll trigger this callback, that we'll
@@ -231,7 +245,9 @@ void AppHost::_HandleCommandlineArgs()
231245
{
232246
const auto numPeasants = _windowManager.GetNumberOfPeasants();
233247
const auto layouts = ApplicationState::SharedInstance().PersistedWindowLayouts();
234-
if (_logic.ShouldUsePersistedLayout() && layouts && layouts.Size() > 0)
248+
if (_logic.ShouldUsePersistedLayout() &&
249+
layouts &&
250+
layouts.Size() > 0)
235251
{
236252
uint32_t startIdx = 0;
237253
// We want to create a window for every saved layout.

0 commit comments

Comments
 (0)