1605405 - Windows toast notifications can have a MSCOM <-> XPCOM reference cycle causing a shutdown crash

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Dave Townsend [:mossop] (PTO until July 20)]

While a toast notification is active there is a reference cycle between MSCOM and XPCOM. Specifically ToastNotificationHandler maintains a reference to the IToastNotification via mNotification and the IToastNotification maintains references to the ToastNotificationHandler via the event handlers which have captured RefPtrs to it. The cycle collector cannot break cycles involving MSCOM objects.

Normally this is fine, when the toast notification ends it drops the event handlers and the ToastNotification service drops its reference to the ToastNotificationHandler which then gets destroyed dropping the reference to the IToastNotification.

However if a notification is still active during shutdown this cycle becomes a problem. XPCOM sends out the quit-application which causes ToastNotification to drop its references to all ToastNotificationHandlers. The code comment there suggests that we expect the handler's destructor to unregister from Windows but because the IToastNotification still holds references to the ToastNotificationHandler the destructor doesn't run yet.

After XPCOM (and so the cycle collector) gets shutdown we tear down the MSCOM environment which I think causes the IToastNotification to drop its event handlers at which point the ToastNotificationHandler destructor runs. This causes it to drop its reference to its mAlertListener which is generally a ContentParent. Since ContentParent is a cycle collected class dropping its reference causes us to call into the cycle collector which at this point has already been shutdown. On debug builds this causes an assertion (https://searchfox.org/mozilla-central/rev/b243debf6235b050b42fd2eb615fdc729636ca6b/xpcom/base/nsCycleCollector.cpp#3761) and from inspection this will cause a crash on non-debug.

In order to fix this, during shutdown (and whenever ToastNotification drops its reference to a ToastNotificationHandler) we tell the ToastNotificationHandler to remove the event handlers from the IToastNotification and to drop its reference to the IToastNotification. This breaks the cycle and with no remaining references the ToastNotificationHandler gets destroyed and so drops its reference to the ContentParent before the cycle collector is shut down.

There is an argument that perhaps we should tear down MSCOM before we shutdown XPCOM and that does fix this issue too and possibly others, but that seems a large change likely to cause more fallout compared to the simpler fix here.

Comment 1

[Dave Townsend [:mossop] (PTO until July 20)]

Attachment: Bug 1605405: Break the MSCOM <-> XPCOM cycle in the alerts service during shutdown.

While windows toast notifications are active there is a reference cycle between
MSCOM and XPCOM objects that the cycle collector cannot break. Fix this by
breaking the reference cycle at any point where we expect the toast notification
to be destroyed.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df1974026ae7
Break the MSCOM <-> XPCOM cycle in the alerts service during shutdown. r=jmathies

Comment 3

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/df1974026ae7