Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 127 additions & 1 deletion src/cascadia/WindowsTerminal/WindowEmperor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <wil/token_helpers.h>
#include <winrt/TerminalApp.h>
#include <sddl.h>
#include <propkey.h>
#include <propvarutil.h>

#include "AppHost.h"
#include "resource.h"
Expand Down Expand Up @@ -313,6 +315,107 @@ AppHost* WindowEmperor::_mostRecentWindow() const noexcept
return mostRecent;
}

// GH#20053: The shell resolves taskbar grouping identity as: per-window AUMID >
// per-process AUMID > auto-derived from exe path. Before we started setting a
// process AUMID, both the pinned .lnk and the process used auto-derived
// identity, so they matched. Now that we set an explicit AUMID, a pinned .lnk
// that predates the AUMID change has no AUMID and still uses auto-derived
// identity, causing a mismatch and a duplicate taskbar button.
Comment on lines +318 to +323
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So LNK files essentially... cache? the AUMID that an app sets? Is that right?
So if my app ever changes the ID all LNKs are invalid?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! The LNK can either have an AUMID explicitly set or it's derived by the OS. I've definitely had a few apps update and not glom to the same taskbar entry and I'm guessing it's because they set a new AUMID.

//
// To fix this, we check if a pinned taskbar shortcut (.lnk) points to our exe.
// If it already carries our AUMID (or no pin exists), we set the process AUMID
// normally. If a pin exists WITHOUT our AUMID, we skip setting the process
// AUMID for THIS launch (both sides use auto-derived identity, so they match)
// and defer stamping the shortcut to process exit. On the next launch, the pin
// has our AUMID, so we set the process AUMID to match, and both agree.
Comment on lines +328 to +330
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not update the LNK immediately?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

// The pin exists but doesn't have our AUMID yet. Don't set the process
// AUMID or stamp the shortcut now. Writing the shortcut causes the
// shell to re-read it immediately, changing the pin's cached identity
// mid-launch and creating a mismatch in the opposite direction. Instead,
// stamp it at shutdown when the taskbar association no longer matters.

If we stamp now, we get two choices:

  • set process AUMID this launch: process stays auto-derived (old AUMID) + pin's identity switches to new AUMID = mismatch
  • set process AUMID: while the window is being created, the shell is auto-deriving the AUMID with the LNK. Since the two operations aren't atomic, we get a mismatch.

Deferring means that we always have a match:

  1. first launch:
    • LNK AUMID: auto-derived
    • Process AUMID: auto-derived
  2. at shutdown:
    • LNK AUMID: stamped with new AUMID
    • Process AUMID: N/A (we've shut down)
  3. next launch:
    • LNK AUMID: new AUMID
    • Process AUMID: new AUMID

These were battle scars I earned while testing/implementing this change haha.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this explanation more than the one in the code to be honest. 🙈 May be worth adjusting the comment IMO.

//
// NOTE: On the first launch after pinning, the process AUMID is not set. If
// toast notifications are needed in the future, use
// ToastNotificationManager::CreateToastNotifier(aumid) with the AUMID string
// directly. That API does not depend on SetCurrentProcessExplicitAppUserModelID.
// A Start Menu shortcut with the AUMID (separate from the taskbar pin) is also
// required for toast routing; see
// https://learn.microsoft.com/windows/apps/develop/notifications/app-notifications/send-local-toast-other-apps
void WindowEmperor::_setupAumid(const std::wstring& aumid)
{
const auto ourExePath = wil::GetModuleFileNameW<std::wstring>(nullptr);

bool needsDeferredStamping = false;
std::wstring pinnedLnkPath;
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed

const auto taskbarGlob = wil::ExpandEnvironmentStringsW<std::wstring>(
LR"(%APPDATA%\Microsoft\Internet Explorer\Quick Launch\User Pinned\TaskBar\*.lnk)");
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed

WIN32_FIND_DATAW findData{};
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
const wil::unique_hfind findHandle{ FindFirstFileExW(taskbarGlob.c_str(), FindExInfoBasic, &findData, FindExSearchNameMatch, nullptr, FIND_FIRST_EX_LARGE_FETCH) };
if (findHandle)
{
const auto lastSlash = taskbarGlob.rfind(L'\\');
const auto taskbarDir = taskbarGlob.substr(0, lastSlash + 1);

do
{
const auto lnkPath = taskbarDir + findData.cFileName;

wil::com_ptr<IShellLinkW> shellLink;
if (FAILED(CoCreateInstance(CLSID_ShellLink, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&shellLink))))
{
continue;
}

const auto persistFile = shellLink.try_query<IPersistFile>();
if (!persistFile || FAILED(persistFile->Load(lnkPath.c_str(), STGM_READ)))
{
continue;
}

wchar_t targetPath[MAX_PATH]{};
if (FAILED(shellLink->GetPath(targetPath, MAX_PATH, nullptr, SLGP_RAWPATH)))
{
continue;
}

if (til::compare_ordinal_insensitive(targetPath, ourExePath) != 0)
{
continue;
}

// Found a pin pointing to us. Assume it needs stamping unless
// we confirm it already has our AUMID.
pinnedLnkPath = lnkPath;
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
needsDeferredStamping = true;

if (const auto propertyStore = shellLink.try_query<IPropertyStore>())
{
wil::unique_prop_variant pv;
if (SUCCEEDED(propertyStore->GetValue(PKEY_AppUserModel_ID, &pv)) &&
pv.vt == VT_LPWSTR && pv.pwszVal &&
aumid == pv.pwszVal)
{
needsDeferredStamping = false;
}
}

break;
} while (FindNextFileW(findHandle.get(), &findData));
}

if (needsDeferredStamping)
{
// The pin exists but doesn't have our AUMID yet. Don't set the process
// AUMID or stamp the shortcut now. Writing the shortcut causes the
// shell to re-read it immediately, changing the pin's cached identity
// mid-launch and creating a mismatch in the opposite direction. Instead,
// stamp it at shutdown when the taskbar association no longer matters.
_pendingAumidLnkPath = std::move(pinnedLnkPath);
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
_pendingAumid = aumid;
}
else
{
LOG_IF_FAILED(SetCurrentProcessExplicitAppUserModelID(aumid.c_str()));
}
}

void WindowEmperor::HandleCommandlineArgs(int nCmdShow)
{
// When running without package identity, set an explicit AppUserModelID so
Expand Down Expand Up @@ -373,7 +476,7 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow)
#else
fmt::format_to(std::back_inserter(unpackagedAumid), FMT_COMPILE(L".{:08x}"), hash);
#endif
LOG_IF_FAILED(SetCurrentProcessExplicitAppUserModelID(unpackagedAumid.c_str()));
_setupAumid(unpackagedAumid);
}
}

Expand Down Expand Up @@ -553,6 +656,29 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow)
Shell_NotifyIconW(NIM_DELETE, &_notificationIcon);
}

// GH#20053: Deferred shortcut stamping. See _setupAumid() for context.
if (!_pendingAumidLnkPath.empty())
{
wil::com_ptr<IShellLinkW> shellLink;
if (SUCCEEDED(CoCreateInstance(CLSID_ShellLink, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&shellLink))))
{
if (const auto persistFile = shellLink.try_query<IPersistFile>();
persistFile && SUCCEEDED(persistFile->Load(_pendingAumidLnkPath.c_str(), STGM_READWRITE)))
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
{
if (const auto propertyStore = shellLink.try_query<IPropertyStore>())
{
wil::unique_prop_variant pv;
if (SUCCEEDED(InitPropVariantFromString(_pendingAumid.c_str(), &pv)) &&
SUCCEEDED(propertyStore->SetValue(PKEY_AppUserModel_ID, pv)) &&
SUCCEEDED(propertyStore->Commit()))
{
persistFile->Save(_pendingAumidLnkPath.c_str(), TRUE);
}
}
}
}
}

// There's a mysterious crash in XAML on Windows 10 if you just let _app get destroyed (GH#15410).
// We also need to ensure that all UI threads exit before WindowEmperor leaves the scope on the main thread (MSFT:46744208).
// Both problems can be solved and the shutdown accelerated by using TerminateProcess.
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/WindowsTerminal/WindowEmperor.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class WindowEmperor
void _persistState(const winrt::Microsoft::Terminal::Settings::Model::ApplicationState& state) const;
void _finalizeSessionPersistence() const;
void _checkWindowsForNotificationIcon();
void _setupAumid(const std::wstring& aumid);

wil::unique_hwnd _window;
winrt::TerminalApp::App _app{ nullptr };
Expand All @@ -84,6 +85,8 @@ class WindowEmperor
std::optional<bool> _currentSystemThemeIsDark;
int32_t _windowCount = 0;
int32_t _messageBoxCount = 0;
std::wstring _pendingAumidLnkPath;
std::wstring _pendingAumid;

#if 0 // #ifdef NDEBUG
static constexpr void _assertIsMainThread() noexcept
Expand Down
Loading