Skip to content

Add scope_exit cleanup to AppNotificationManager::Register #6242

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.

Sign up for GitHub

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

Closed
guimafelipe wants to merge 1 commit into from
Closed

Add scope_exit cleanup to AppNotificationManager::Register #6242

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 22 additions & 1 deletion dev/AppNotifications/AppNotificationManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation
}) };

winrt::guid registeredClsid{};
if (AppModel::Identity::IsPackagedProcess())
bool isUnpackaged{ !AppModel::Identity::IsPackagedProcess() };

if (!isUnpackaged)
{
registeredClsid = RegisterPackagedApp();
}
Expand All @@ -140,12 +142,23 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation
registeredClsid = RegisterUnpackagedApp(assets);
}

// Cleanup registration on failure of subsequent steps
auto registrationCleanup{ wil::scope_exit([&]()
{
if (isUnpackaged)
{
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

The cleanup is incomplete. If RegisterComServer or m_waitHandleForArgs.create() fails after RegisterUnpackagedApp succeeds, only PushNotifications_UnregisterFullTrustApplication is called. However, RegisterUnpackagedApp also creates other resources that need cleanup:

  1. COM server registry entries via GetOrCreateComActivatorGuid → RegisterComServer (lines 254, 260 in AppNotificationUtility.cpp)
  2. App identifier registry entries via RegisterAssets (line 256)
  3. Toast registration mapping via RegisterAppNotificationSinkWithLongRunningPlatform (line 260)

Compare with UnregisterAll() (lines 333-338) which cleans up all these resources for unpackaged apps:

  • Gets the COM activator GUID and calls UnRegisterComServer
  • Calls UnRegisterNotificationAppIdentifierFromRegistry
  • Calls PushNotifications_UnregisterFullTrustApplication

The cleanup lambda should perform all these cleanup steps, wrapped in LOG_IF_FAILED to handle cases where resources weren't created yet.

Suggested change
{
{
// Best-effort cleanup of all resources created for unpackaged registration.
LOG_IF_FAILED(UnRegisterComServer(registeredClsid));
LOG_IF_FAILED(UnRegisterNotificationAppIdentifierFromRegistry(m_appId.c_str()));

Copilot uses AI. Check for mistakes.
LOG_IF_FAILED(PushNotifications_UnregisterFullTrustApplication(m_appId.c_str()));
}
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

For packaged apps (when !isUnpackaged is true), the cleanup should also handle the toast registration mapping created by RegisterAppNotificationSinkWithLongRunningPlatform.

RegisterPackagedApp (line 237) conditionally calls RegisterAppNotificationSinkWithLongRunningPlatform (line 243) which calls AddToastRegistrationMapping. If RegisterComServer or m_waitHandleForArgs.create() fails after this, the mapping is not cleaned up.

The cleanup lambda should include: if (!PushNotificationHelpers::IsPackagedAppScenario() && !WindowsAppRuntime::SelfContained::IsSelfContained()) { LOG_IF_FAILED(notificationPlatform->RemoveToastRegistrationMapping(...)); }

This matches the cleanup in UnregisterAll() (lines 319-323).

Suggested change
}
}
else if (!PushNotificationHelpers::IsPackagedAppScenario() && !WindowsAppRuntime::SelfContained::IsSelfContained())
{
LOG_IF_FAILED(notificationPlatform->RemoveToastRegistrationMapping(m_appId.c_str()));
}

Copilot uses AI. Check for mistakes.
}) };

// Create event handle before COM Registration otherwise if a notification arrives will lead to race condition
m_waitHandleForArgs.create();

// Register the AppNotificationManager as a COM server for Shell to Activate and Invoke
RegisterComServer(registeredClsid);

registrationCleanup.release();

logTelemetry.Stop();
}

Expand Down Expand Up @@ -178,12 +191,20 @@ namespace winrt::Microsoft::Windows::AppNotifications::implementation

winrt::guid registeredClsid{ RegisterUnpackagedApp(assets) };

// Cleanup unpackaged registration on failure of subsequent steps
auto registrationCleanup{ wil::scope_exit([&]()
{
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

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

The cleanup is incomplete. If RegisterComServer fails after RegisterUnpackagedApp succeeds, only PushNotifications_UnregisterFullTrustApplication is called. However, RegisterUnpackagedApp also creates other resources that need cleanup:

  1. COM server registry entries via GetOrCreateComActivatorGuid → RegisterComServer (lines 254, 260 in AppNotificationUtility.cpp)
  2. App identifier registry entries via RegisterAssets (line 256 in AppNotificationUtility.cpp)
  3. Toast registration mapping via RegisterAppNotificationSinkWithLongRunningPlatform (line 260)

Compare with UnregisterAll() (lines 333-338) which cleans up all these resources for unpackaged apps:

  • Gets the COM activator GUID and calls UnRegisterComServer
  • Calls UnRegisterNotificationAppIdentifierFromRegistry
  • Calls PushNotifications_UnregisterFullTrustApplication

The cleanup lambda should perform all these cleanup steps, wrapped in LOG_IF_FAILED to handle cases where resources weren't created yet.

Suggested change
{
{
// Mirror UnregisterAll() behavior for unpackaged apps: clean up COM server,
// app identifier registry entries, and push notification registration.
LOG_IF_FAILED(UnRegisterComServer(registeredClsid));
LOG_IF_FAILED(UnRegisterNotificationAppIdentifierFromRegistry(m_appId.c_str()));

Copilot uses AI. Check for mistakes.
LOG_IF_FAILED(PushNotifications_UnregisterFullTrustApplication(m_appId.c_str()));
}) };

// Create event handle before COM Registration otherwise if a notification arrives will lead to race condition
m_waitHandleForArgs.create();

// Register the AppNotificationManager as a COM server for Shell to Activate and Invoke
RegisterComServer(registeredClsid);

registrationCleanup.release();

logTelemetry.Stop();
}

Expand Down