Add scope_exit cleanup to AppNotificationManager::Register#6242
Add scope_exit cleanup to AppNotificationManager::Register#6242guimafelipe wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds cleanup logic to the AppNotificationManager::Register methods to handle failures during registration. When RegisterComServer or event handle creation fails after RegisterUnpackagedApp succeeds, the PR ensures that PushNotifications_UnregisterFullTrustApplication is called to roll back the full-trust application registration.
Changes:
- Adds
wil::scope_exitcleanup guards to bothRegister()overloads - Introduces
isUnpackagedboolean to replace negatedIsPackagedProcess()check for clarity - Implements cleanup via
PushNotifications_UnregisterFullTrustApplicationwhen subsequent registration steps fail - Releases the cleanup guard on successful registration to prevent rollback
| if (isUnpackaged) | ||
| { | ||
| LOG_IF_FAILED(PushNotifications_UnregisterFullTrustApplication(m_appId.c_str())); | ||
| } |
There was a problem hiding this comment.
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).
| } | |
| } | |
| else if (!PushNotificationHelpers::IsPackagedAppScenario() && !WindowsAppRuntime::SelfContained::IsSelfContained()) | |
| { | |
| LOG_IF_FAILED(notificationPlatform->RemoveToastRegistrationMapping(m_appId.c_str())); | |
| } |
| void AppNotificationManager::Register() | ||
| { | ||
| if (!IsSupported()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| auto logTelemetry{ AppNotificationTelemetry::Register::Start(g_telemetryHelper, m_appId) }; | ||
|
|
||
| { | ||
| auto lock{ m_lock.lock_exclusive() }; | ||
| THROW_HR_IF_MSG(HRESULT_FROM_WIN32(ERROR_OPERATION_IN_PROGRESS), m_registering, "Registration is in progress!"); | ||
| m_registering = true; | ||
| } | ||
|
|
||
| auto registeringScopeExit{ wil::scope_exit([&]() | ||
| { | ||
| auto lock { m_lock.lock_exclusive() }; | ||
| m_registering = false; | ||
| }) }; | ||
|
|
||
| winrt::guid registeredClsid{}; | ||
| if (AppModel::Identity::IsPackagedProcess()) | ||
| bool isUnpackaged{ !AppModel::Identity::IsPackagedProcess() }; | ||
|
|
||
| if (!isUnpackaged) | ||
| { | ||
| registeredClsid = RegisterPackagedApp(); | ||
| } | ||
| else | ||
| { | ||
| AppNotificationAssets assets{ GetAssets() }; | ||
| registeredClsid = RegisterUnpackagedApp(assets); | ||
| } | ||
|
|
||
| // Cleanup registration on failure of subsequent steps | ||
| auto registrationCleanup{ wil::scope_exit([&]() | ||
| { | ||
| if (isUnpackaged) | ||
| { | ||
| 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(); | ||
| } |
There was a problem hiding this comment.
Consider adding tests for the registration cleanup logic. Currently, there don't appear to be any tests that verify cleanup happens correctly when Register() fails partway through.
While testing failure scenarios can be challenging, consider adding tests that:
- Verify that if registration fails, subsequent re-registration attempts succeed (which would fail if cleanup didn't work)
- Verify that registry entries are not left behind after a failed registration
- Use mocking or fault injection to simulate failures in RegisterComServer or m_waitHandleForArgs.create()
This would help ensure the cleanup logic works correctly and prevent regressions.
| auto registrationCleanup{ wil::scope_exit([&]() | ||
| { | ||
| if (isUnpackaged) | ||
| { |
There was a problem hiding this comment.
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:
- COM server registry entries via GetOrCreateComActivatorGuid → RegisterComServer (lines 254, 260 in AppNotificationUtility.cpp)
- App identifier registry entries via RegisterAssets (line 256)
- 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.
| { | |
| { | |
| // Best-effort cleanup of all resources created for unpackaged registration. | |
| LOG_IF_FAILED(UnRegisterComServer(registeredClsid)); | |
| LOG_IF_FAILED(UnRegisterNotificationAppIdentifierFromRegistry(m_appId.c_str())); |
|
|
||
| // Cleanup unpackaged registration on failure of subsequent steps | ||
| auto registrationCleanup{ wil::scope_exit([&]() | ||
| { |
There was a problem hiding this comment.
The cleanup is incomplete. If RegisterComServer fails after RegisterUnpackagedApp succeeds, only PushNotifications_UnregisterFullTrustApplication is called. However, RegisterUnpackagedApp also creates other resources that need cleanup:
- COM server registry entries via GetOrCreateComActivatorGuid → RegisterComServer (lines 254, 260 in AppNotificationUtility.cpp)
- App identifier registry entries via RegisterAssets (line 256 in AppNotificationUtility.cpp)
- 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.
| { | |
| { | |
| // 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())); |
1 participant
Summary
wil::scope_exitcleanup guards to bothRegister()overloadsRegisterComServerfails afterRegisterUnpackagedAppsucceeds, the full-trust registration is now rolled back viaPushNotifications_UnregisterFullTrustApplicationFixes #2269