Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 327003 - EMF Registries who use PluginClassDescriptor are potentially broken when you handle your Bundle dynamically
Summary: EMF Registries who use PluginClassDescriptor are potentially broken when you ...
Status: CLOSED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: Core (show other bugs)
Version: 2.6.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Ed Merks CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-05 09:11 EDT by Xavier Maysonnave CLA
Modified: 2010-12-14 23:29 EST (History)
0 users

See Also:


Attachments
Registry Sample from org.eclipse.egf (3.00 KB, application/octet-stream)
2010-10-05 14:05 EDT, Xavier Maysonnave CLA
no flags Details
Patches to address the issue. (1.46 KB, patch)
2010-10-05 16:05 EDT, Ed Merks CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xavier Maysonnave CLA 2010-10-05 09:11:37 EDT
The PluginClassDescriptor holds an IConfigurationElement of a matching extension point. Such element could become invalid.

As an example the class PluginChildCreationExtenderDescriptor holds information of its RegistryReader analysed extension point 'childCreationExtenders'. 
As such it stores the according readed IConfigurationElement. 
When a bundle who uses this extension point is uninstalled and the osgi package admin is refreshed the relevant RegistryReader is notified and the PluginChildCreationExtenderDescriptor try to clean itself. However the stored IConfigurationElement is no longer valid and as such the match method crash:

public boolean matches(IConfigurationElement element)
{
return element.getContributor().equals(this.element.getContributor());
}

This report could be reproduced with an OSGI console.
1 - Start emf edit bundle to have an active ChildCreationExtender registry
2 - Start a bundle who exposes a ChildCreationExtender in its plugin.xml
3 - In the osgi console 'ss' your configuration and checked that both plugins are started (note the bundle id).
4 - use the 'uninstall id' command in the osgi console to uninstall the bundle who exposes the ChildCreationExtender
5 - refresh (at this point the PluginChildCreationExtenderDescriptor should crash as its stored IConfigurationElement is no longer valid)

Thanks
Comment 1 Ed Merks CLA 2010-10-05 10:56:18 EDT
This logic to remove it should kick in, doesn't it?

                 Collection<IChildCreationExtender.Descriptor> collection = childCreationExtenderDescriptorRegistry.get(packageURI);
                 if (add)
                 {
                   if (collection == null)
                   {
                     childCreationExtenderDescriptorRegistry.put(packageURI, collection = new ArrayList<IChildCreationExtender.Descriptor>());
                   }

                   collection.add(new PluginChildCreationExtenderDescriptor(element, "class"));
                 }
                 else if (collection != null)
                 {
                   for (IChildCreationExtender.Descriptor descriptor : collection)
                   {
                     if (descriptor instanceof PluginChildCreationExtenderDescriptor && ((PluginChildCreationExtenderDescriptor)descriptor).matches(element))
                     {
                       collection.remove(descriptor);
                       break;
                     }
                   }
                 }

I.e., did you really test that the descriptor isn't removed?
Comment 2 Xavier Maysonnave CLA 2010-10-05 11:03:14 EDT
Yes, I tested it. 
The descriptor isn't removed because the matches test is against an invalid IConfigurationElement and crashed... 
Here is the stack trace I got :

rg.eclipse.core.runtime.InvalidRegistryObjectException: Invalid registry object
	at org.eclipse.core.internal.registry.RegistryObjectManager.basicGetObject(RegistryObjectManager.java:272)
	at org.eclipse.core.internal.registry.RegistryObjectManager.getObject(RegistryObjectManager.java:262)
	at org.eclipse.core.internal.registry.ConfigurationElementHandle.getConfigurationElement(ConfigurationElementHandle.java:26)
	at org.eclipse.core.internal.registry.ConfigurationElementHandle.getContributor(ConfigurationElementHandle.java:134)
	at org.eclipse.emf.edit.EMFEditPlugin$4$1PluginChildCreationExtenderDescriptor.matches(EMFEditPlugin.java:254)
	at org.eclipse.emf.edit.EMFEditPlugin$4.readElement(EMFEditPlugin.java:272)
	at org.eclipse.emf.ecore.plugin.RegistryReader.internalReadElement(RegistryReader.java:123)
	at org.eclipse.emf.ecore.plugin.RegistryReader.access$0(RegistryReader.java:121)
	at org.eclipse.emf.ecore.plugin.RegistryReader$1.registryChanged(RegistryReader.java:113)
	at org.eclipse.core.internal.registry.ExtensionRegistry$2.run(ExtensionRegistry.java:921)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.core.internal.registry.ExtensionRegistry.processChangeEvent(ExtensionRegistry.java:919)
	at org.eclipse.core.runtime.spi.RegistryStrategy.processChangeEvent(RegistryStrategy.java:260)
	at org.eclipse.core.internal.registry.osgi.ExtensionEventDispatcherJob.run(ExtensionEventDispatcherJob.java:50)
Comment 3 Ed Merks CLA 2010-10-05 12:10:07 EDT
I see!

So we could either cache the contributor, which probably also becomes invalid or the contribute name, which is a string, and then compare to the String instead.  See what I mean?  Could you try that?
Comment 4 Xavier Maysonnave CLA 2010-10-05 14:05:25 EDT
Created attachment 180267 [details]
Registry Sample from org.eclipse.egf
Comment 5 Xavier Maysonnave CLA 2010-10-05 14:06:33 EDT
Ed,

I think a more robust implementation is possible, however it relies on internal code. 

Each extension point has a unique identifier and this information is not yet a public API.
 
In the attached registry sample you could see how EGF is managing its registries.
1 - Each registry listen for a particular extension-point
2 - If suitable each of them have a public or a private unique identifier org.eclipse.core.internal.Handle (I know it's bad...)

I found this code in eclipse itself and it sounds to me a good approach.

Cheers
Comment 6 Ed Merks CLA 2010-10-05 16:05:42 EDT
Created attachment 180280 [details]
Patches to address the issue.

Given that all we want is to implement matches

                   public boolean matches(IConfigurationElement element)
                   {
                     return element.getContributor().equals(this.element.getContributor());
                   }

which compare contributors, for which the only method is getName(), I'm not sure that something needs to be more robust than comparing the name more directly by caching the contributor name as in this patch...

What needs to be more robust?
Comment 7 Xavier Maysonnave CLA 2010-10-06 06:01:11 EDT
Ed,

Thanks for the patch. 
It works as expected.
By more robust I mean other registries who rely on PluginClassDescriptor. I feared other side effects on other registries.
Comment 8 Ed Merks CLA 2010-11-05 06:24:02 EDT
The fix has been committed to CVS for 2.7.
Comment 9 Ed Merks CLA 2010-12-13 16:42:43 EST
The fixes are available in a published build.
Comment 10 Ed Merks CLA 2010-12-14 23:28:05 EST
Try again to resolve as fixed (and available in a build).
Comment 11 Ed Merks CLA 2010-12-14 23:29:44 EST
Oh, I guess I have to close them!