GNOME Bugzilla – Bug 604094
registry: do not remove features when removing a cached plugin that no longer is present
Last modified: 2011-04-24 13:20:31 UTC
Created attachment 149354 [details] [review] patch Basically, the adding of features comes from an existing plugin calling gst_element_register(), so if the plugin is not there, it can't have registered any features, so it does not make sense to remove them when removing the plugin. In the case where (on Windows) a plugin DLL have changed name, the loaded features of the newly loaded plugin will be removed when removing the features of the cached plugin.
If the plugin has disappeared, wouldn't it just rebuild the registry anyway? What happens for you? You rename a plugin library and get a crash?
Hm, Håvard, could you address the questions raised in comment #1? It'd be a pity if the patch bitrots around...
I must admit that I dont quite remember all the details around this patch, as it was a few hours of intense debugging in a part of gstreamer I had never ventured into before (and not since either!). :) However, the patch still stands AFAIAC. If I remember correctly, the first thing that happends is that all plugins are loaded from the registry, then it searches for all plugins on the system, and then any that are in the registry, but not on the system, is to be removed, and as part of the removal is calling gst_registry_remove_features_for_plugin_unlocked, but the thing is that if the plugin did only exist in the registry, and not on the system, it would be impossible for it to have had registered its features, since this happens through the element. (and the element is not there!) We discovered this because we renamed a plugin, so that our registry had the old name, but the feature (the gstreamer element) was still the same, so that something like this would happen: 1) old.plugin with feature A is found in registry 2) new.plugin with feature A is found on the system, old.plugin is not 3) new.plugin feature A is registered 4) old.plugin is removed from the registry, and part of this process is removing feature A 5) Our app ended up not finding feature A (not registered) The patch basically says that removing a feature for a removed plugin like this does not make any sense at all, because it cant have been registered if the plugin did not exist!
Reopening as I think the requested information has been provided.
The problem described here can be reproduced quite easily: all that's needed is to rename a plugin .so file in the plugin path. Afterwards, gst-inspect $feature no longer works. It also seems the patch fixes this particular problem, at least for me. I'm not entirly sure about the reasoning in the commit messsage though. I think the problem is that gst_registry_remove_features_for_plugin_unlocked() removes features based on the plugin *name*, instead of removing them based on the plugin object/pointer. The reason it does that is presumably because GstPluginFeature doesn't contain a pointer to the GstPlugin (to avoid refcounting circles perhaps).
I think it is explained properly in Comment 3. When removing a plugin that did not exist on the system, (but was in the registry) it does not make sense to remove a registered feature of that plugin, because if the plugin was not there in the first place, it could not have registered any features, since the registering of features (elements) happens through the plugin.
Wouldn't it be better to store the GstPlugin in GstPluginFeature and do the removal properly? Could be done with a weak ref to prevent refcounting circles
(In reply to comment #7) > Wouldn't it be better to store the GstPlugin in GstPluginFeature and do the > removal properly? Could be done with a weak ref to prevent refcounting circles I would not know. I am only pointing out a logical error in the code. When you discover that a plugin has been removed, it never makes sense to remove the features of that plugin, since those features can't have been registered in the first place. (since the plugin is gone) It is like discovering that your pregnant dog has ran of, and then go on to advertise its puppies for sale... :)
But we don't if the features have been registered, do we? They've just been loaded from the registry file, just like the plugin marked as CACHED, no?
Håvard: I think your assumptions are wrong here ("if the plugin was not there in the first place, it could not have registered any features, since the registering of features happens through the plugin."): in this case the features have been created when reading the registry cache at startup. Let's try this: commit f9625dbe5531c66be59088f032561ea76e3fc83b Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Sun Apr 24 09:58:53 2011 +0100 registry: when removing a cached-but-no-longer-existing plugin, only remove features that belong to it When a plugin file no longer exists, e.g. because it's been removed or renamed, don't remove all features in the registry based on the *name* of the plugin they belong to, but only remove those who actually belong to that particular plugin (object/pointer). This fixes issues of plugin features disappearing when a plugin .so file is renamed. https://bugzilla.gnome.org/show_bug.cgi?id=604094 commit 4926ce31c7b2fb1b42ddc12d502dbb99d41463b5 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Sun Apr 24 09:53:39 2011 +0100 pluginfeature: store pointer to plugin in addition to the plugin name So we can reliably remove plugin features for a specific plugin later. https://bugzilla.gnome.org/show_bug.cgi?id=604094
(In reply to comment #10) > Håvard: I think your assumptions are wrong here ("if the plugin was not there > in the first place, it could not have registered any features, since the > registering of features happens through the plugin."): in this case the > features have been created when reading the registry cache at startup. > I might be wrong, but I remember that there is a difference between reading the feature-names from the registry, and actually registering them. I found that the actual registering could *only* happen through the plugin itself, and the registry only kept the names as a list of what can be there. Hence any plugin found through the registry would still have the register-function called on them, registering all the features. (elements) So my argument was that if a plugin was not found, there would be no features registered for that plugin, even if they were listed in the registry. If that is the case, the idea of removing features for a plugin that is not found does not make sense.
As discussed on IRC, that line in your patch actually does something useful if the plugin .so file is just removed, not renamed.