FIX(client): Issues when updating installed plugins

[Krzmbrzl]

The first issue was that the old plugin would simply get overwritten
without at least warning the user that such a thing will take place.
With this commit this is still not guaranteed as plugins can be
installed in different directories in which case no warning is emitted
(the user will end up with two versions of the plugin installed in
parallel).

The second issue was that if a plugin was loaded while it is being
updated, on some OS (e.g. Windows) the corresponding library file is
locked and can thus not be overwritten. Therefore plugins are now
explicitly cleared before an overwrite is attempted.

Checks

• My commits follow the commit guidelines

[hbeni]

Is there some nightly Installer I can download for testing?

[davidebeatrici]

Yes, you can download it from Azure Pipelines.

[hbeni]

Yes but which one? The latest i see is from friday - is the code already there (i assume no, since its from master branch?)

[davidebeatrici]

https://dev.azure.com/Mumble-VoIP/Mumble/_build/results?buildId=4987

[hbeni]

Ok did a test.
In the minute reported as 20:32 i tried three times to install the plugin.
it looks like the shutdown is not initiated, at least nothing in that direction is logged.
fgcom-mumble.log

The line FGCom [2021-06-27 20:33:19.441]: Shutdown plugin was when I finally shutdown the client.

---

edit: When i umload the plugin manually before reinstalling, the shutdown is called.
Still, however, the same error occurs.

[Krzmbrzl]

That's weird. The shutdown message of your plugin was always printed for me but the file still kept being locked 👀

[hbeni]

Really, weird.
Did you really see the shutdown msg from the unload, or was it from mumble-closing like i observe?

I really have no clue what could cause dll locking, but i will try to search the interwebs for it.

[davidebeatrici]

mumble/src/mumble/PluginManager.cpp

Lines 549 to 562 in cc03756

	bool PluginManager::clearPlugin(plugin_id_t pluginID) {
	// We have to unload the plugin before we take the write lock. The reasoning being that if
	// the plugin makes an API call in its shutdown callback, that'll lead to this manager being
	// asked for whether a plugin with such an ID exists. The function performing this check will
	// take a read lock which is not possible if we hold a write lock here already (deadlock).
	unloadPlugin(pluginID);
	QWriteLocker lock(&m_pluginCollectionLock);
	// Remove the plugin from the list of known plugins
	plugin_ptr_t plugin = m_pluginHashMap.take(pluginID);
	return plugin != nullptr;
	}

mumble/src/mumble/Plugin.h

Line 108 in baedddd

typedef std::shared_ptr< Plugin > plugin_ptr_t;

@Krzmbrzl Is perhaps the shared pointer being used somewhere?

[Krzmbrzl]

Did you really see the shutdown msg from the unload, or was it from mumble-closing like i observe?

I did observe it when installing the plugin because I saw it before closing Mumble.

Is perhaps the shared pointer being used somewhere?

That was what I thought at first as well but

1. It worked as expected with a different plugin
2. I verified the plugin's destructor to be called and that Qt claims to have successfully unloaded the underlying library. On that note though, I did not explicitly verify that it was the fgcom plugin (but then again: it would be odd if it was a different one) 🤔

[hbeni]

Whats different at your test plugin?

[Krzmbrzl]

Whats different at your test plugin?

I don't know. It was the ACRE2 plugin that I tested this with and with that it just worked as expected 🤷

[hbeni]

At least this tells its plugin sided.
Is there source i can look for?

[Krzmbrzl]

Yes there is: https://github.com/IDI-Systems/acre2/tree/mumble-plugin/extensions/src/ACRE2Mumble

[hbeni]

Ok, i opened hbeni/fgcom-mumble#131 for further tracking.
It would be cool if someone could help with this, i feel really alone with this dll locking stuff.

[Krzmbrzl]

Okay after a fair amount of troubleshooting it turned out that the issue was indeed on the plugin's side (more specifically an issue with OpenSSL).
Now that it has been fixed the issue seems to have been resolved.

Thus I am merging this now.

[davidebeatrici]

What kind of issue with OpenSSL?

[hbeni]

See here: hbeni/fgcom-mumble#131 (comment)

TLDR; the windows plugin dll got locked and not released when unloading, preventing the updater from replacing the plugin with the new version.
Cause was a change in OpenSSL which could be mitigated by adding the compile time configure option no-pinshared.

[davidebeatrici]

Oh, I see. For reference: microsoft/vcpkg#12056