Community
Participate
Working Groups
I20070925-1406 In History view for a .java file from CVS, select an old revision and choose context menu > Show Annotation. Close the editor. => A CompilationUnitEditor is leaked (via JavaEditorErrorTickUpdater and via AbstractTextEditor$PropertyChangeListener). The reason is that org.eclipse.team.internal.ui.Utils.isTextEditor(IEditorDescriptor) uses internal ui.workbench code to create an editor but does not dispose it afterwards. I don't think you should wait for bug 99568. A better request would be a query on IEditorDescriptor that returns the Class object that would be used to create an editor instance. The check can then use Class.isAssignableFrom(Class) without creating the editor at all.
Created attachment 79201 [details] Temporary fix Fixes the leak until a better solution is available.
Created attachment 79299 [details] Fix without creating any editor This fix also removes the usage access restricted code. If you want to keep on using access restricted code you can make the fix simpler as the EditorDescriptor already has the class and bundle name. I decided to make the util method easier to use by directly handling the exception case inside it. Also, this code could be sent over to Platform UI should they decide to provide such a helper in the future.
(In reply to comment #2) Caveat: the "Fix without creating any editor" probably fails for clients that supply an IExecutableExtensionFactory for creating the instance, see Javadoc of org.eclipse.core.runtime.IConfigurationElement.createExecutableExtension(String). > Also, this code could be sent over to Platform UI should they decide to provide > such a helper in the future. I think the right final home for the registry reading and class loading would be org.eclipse.core.runtime.IConfigurationElement, e.g.: public Class createExtensionClass(String propertyName) throws CoreException; This method would have to specify what happens in the IExecutableExtensionFactory case (whether it just always fails, or whether we need new API in IExecutableExtensionFactory to create just the Class but not an instance).
I have released the patch from comment 1 since it does fix the problem and is guaranteed not to cause a regression. However, I like the idea of not needing to create the editor to determine if it is a text editor. In practice, do we know if editors are often registered vi an IExecutableExtensionFactory?
Created attachment 79622 [details] Fix which only creates editor if needed > In practice, do we know if editors are often registered vi an >IExecutableExtensionFactory? We don't know how often but I can check whether the given 'class' does so. Here's a patch that fixes this and has less changes as it takes the necessary attributes from the internal EditorDescriptor class.
(In reply to comment #5) I have not tried it, but from looking at the patch, I think it will fail too if the class attribute uses IExecutableExtensionFactory as suggested in the Javadoc, e.g. class="p.MyExecutableExtensionFactory:pinkTextEditor". See the code around prop.indexOf(':') in ConfigurationElement.createExecutableExtension(String). I think you should also try to create (and dispose) the editor in case of a ClassNotFoundException.
>I think you should also try to create (and dispose) the editor in case of a >ClassNotFoundException. I wouldn't do that. Simply check the class string for a ':'.
Note that ConfigurationElement.createExecutableExtension(String) contains even more special stuff (e.g. trimming of the strings, a contributorName, or a definition in a separate child element). I agree with the shortcut for the simplest case (which the SDK uses exclusively), but I would defer everything else to createExecutableExtension until Core has better support for this. Even if you decide to add the ":" case, I'd argue that it is not normal that a class cannot be found, and chances are much higher that our hand-made shortcut is incomplete than that there's really something wrong with the contribution.
Created attachment 79629 [details] Fix which only creates editor if needed OK. I give up ;-) Here's a more pessimistic patch that also handles the 1 in a million case where such a factory is used (I'd even say there's none out there).
The patch is not that big, maybe still for M4? ;-)
Dani, I'm busy with some other items, so I can't promise you I will take a look at it before M4... but I will try.
Thanks.
Created attachment 88867 [details] Dani's latest patch with an additional null check This is Daniel's patch with an additional null check, despite the fact the null shouldn't be returned as the editor descriptor gives us the plug-in name. I will release the patch as the first one for 3.4 M6. Sorry for the delay Daniel.
The latest patch released to HEAD. Thanks Dani.
Verified by code inspection.
