WinToastHandler objects leak

[kambala-decapitator]

When a toast is created, one simply throws in new WinToastHandler (or a subclass), and later the object is never deleted inside the library. IMO WinToast should be responsible for their lifecycle. (smart pointer might also be an option, but should be handled accordingly)

[dscho]

@kambala-decapitator good idea. The library already remembers the notification object and uses that to hide the toast later.

You could simply change the type of the map that remembers the notification objects to remember a pair of a notification object and the toast handler instead: std::map<INT64, std::pair<ComPtr<IToastNotification>, IWinToastHandler *> _buffer;

While at it, you could also change the signature of showToast() to accept a std::shared_ptr<IWinToastHandler> instead. Then you would simply need to teach the hideToast() method to delete the mapping and the toast handler would be destroyed, as it should be.

How about taking a crack at it yourself?

[malja]

@dscho Since I am using this library in my project, I would like to solve this issue.

Changes:

_buffer in WinToastLib::WinToast is now defined as:

std::map<INT64, std::pair<ComPtr<IToastNotification>, std::shared_ptr<IWinToastHandler>>> _buffer;

And the corresponding method:

showToast(_In_ const WinToastTemplate& toast, _In_ std::shared_ptr<IWinToastHandler> handler)

Problem:

I did some tests to make sure ToastHandler instance is properly destructed after calling WinToast::hide. I found out there are four instances in total - one in _buffer and one instance for each callback. Only one of those instances is cleared - the one in _buffer after calling WinToast::hide. The rest of them live. Do they get destructed somewhere in Windows? Or do they still leak?

I may pass ToastHandler instance by reference to each callback. It does not increase the shared_ptr counter. On the other hand, calling hide method clears handler before OnDismiss callback and throws exception.

Do I miss something?

[dscho]

I found out there are four instances in total - one in _buffer and one instance for each callback.

Oy, I did not realize this. As the callbacks do not need to do anything after the toast is hidden (and destroyed), maybe the best idea would be to unregister the callbacks in the destructor (if that is possible, that is)?

[malja]

@dscho I found remove_Activated/Failed/Dismissed methods. It will be possible to remove event handlers if registration tokens are saved during registration. But what happens if you close app (this removes event handler) and user clicks the notification? Shouldn't handler keep existing?

[dscho]

@malja TBH I am not quite sure... I guess the handler will not keep existing, because AFAIU WinToast only sets up foreground activation (meaning: the activation is handled only as long as the process is around). See https://docs.microsoft.com/en-us/windows/uwp/design/shell/tiles-and-notifications/send-local-toast#handling-activation-1 for more details.

[mohabouje]

@dscho I think you are right: the activation is handled only as long as the process is around