Windows toast notifications can have a MSCOM <-> XPCOM reference cycle causing a shutdown crash
Categories
(Toolkit Graveyard :: Notifications and Alerts, defect)
Tracking
(firefox73 fixed)
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(1 file)
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 ToastNotificationHandler
s. 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.
Assignee | ||
Comment 1•4 years ago
|
||
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.
Pushed by dtownsend@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df1974026ae7 Break the MSCOM <-> XPCOM cycle in the alerts service during shutdown. r=jmathies
Comment 3•4 years ago
|
||
bugherder |
Updated•10 months ago
|
Description
•