FIX(plugins): Unset active positional plugin on unloading it #6602
Conversation
|
I happened to notice that #6549 sounds related, and this might fix that issue. |
|
I have to double check that this doesn't cause unintended side effects in the semantics for plugins. 👀 |
Krzmbrzl
force-pushed
the
unload-pa-plugin
branch
2 times, most recently
from
3213792 to
14d323b
Compare
If a plugin is providing positional audio data and it becomes unloaded, such as by disabling the plugin in the settings GUI, the plugin manager may still interact with the plugin through m_activePositionalDataPlugin. This looks unintentional and, in debug mode, this fails an assertion in the plugin fairly quickly: `assertPluginLoaded(this)`. Additionally, in case Mumble shuts down while there exists an active positional data plugin, Mumble will (now) try to log that the plugin has lost link by calling the respective function on the global Log instance. However, at the point where the plugin manager object is destroyed, the log has already been deleted, making this produce a segmentation fault. To prevent this from happening, an explicit check for whether the Log instance still exists has been added to the logging logic. Fixes mumble-voip#6549
Krzmbrzl
force-pushed
the
unload-pa-plugin
branch
from
14d323b to
e9c53b1
Compare
There was a problem hiding this comment.
I have replaced the earlier deletion of the plugin manager object with an explicit check for the existence of the Log instance.
This way, we don't change any semantics on when plugins are unloaded when Mumble shuts down. This is a rather complicated topic as there are many implicit relationships (mainly due to plugins having the opportunity to use an API to make calls into the Mumble code base and some of that code expects certain things to be around (or to be in a given state if they are no longer around)....
I don't feel like digging all that up if the fix can be seemingly as simple as adding a condition to an already existing if clause :)
|
Thanks for fixing this 👍 |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
…positional plugin on unloading it"
If a plugin is providing positional audio data and it becomes unloaded, such as by disabling the plugin in the settings GUI, the plugin manager may still interact with the plugin through m_activePositionalDataPlugin.
This looks unintentional and, in debug mode, this fails an assertion in the plugin fairly quickly:
assertPluginLoaded(this).Additionally, in case Mumble shuts down while there exists an active
positional data plugin, Mumble will (now) try to log that the plugin has
lost link by calling the respective function on the global Log instance.
However, at the point where the plugin manager object is destroyed, the
log has already been deleted, making this produce a segmentation fault.
To prevent this from happening, an explicit check for whether the Log
instance still exists has been added to the logging logic.
Fixes #6549
Checks
I haven't checked what this does in a release build or on any stable branches. I only noticed this assertion tripped on master if a plugin is un-enabled while it is linked.
And I'm not quite sure if moving where the plugin manager is deconstructed is quite right or if you have a different idea in mind. But if the log is destroyed before the plugin manager, it will try to log about the link lost when unloading an active plugin and crash. If the main window isn't around while logging it can crash trying to display the log in the main window.