Community
Participate
Working Groups
Found in the Workspace, by setting a Java Exception Breakpoint on NullPointerException and creating a new connection of type "Linux" with the RSE Examples and Test Subsystem installed. In SubSystemConfigurationAdapter.getNewConnectionWizardPages(), when the subsystem is not of type ServiceSubsystem, the method getSubSystemPropertyPages() is called in order to automatically contribute wizard pages from contributed property pages. This method iterates over all extensions of type org.eclipse.ui.propertyPages and tries to load the class specified by the "objectClass" attribute. This is problematic in several ways: 1.) As of Eclipse 3.3, objectClass is deprecated. It would be better to ask some Eclipse PropertyPageManager rather than parsing the extensions ourselves. 2.) objectClass is not checked for "null" although it is an optional attribute 3.) By loading the class, lots of plugins may be activated even though they do not actually contribute property pages. It would be better to require a special tag for those property pages that should contribute to the Wizard. I think that the best solution would be to get rid of this functionality completely and not allow to automatically re-use contributed property pages in the Wizard. Subsystem contributors should rather be forced to specify their wizard pages explicitly by overriding the getWizardPages() method in their own registered SubsystemConfigurationAdapter. -----------Enter bugs above this line----------- TM 2.0M5 Testing installation : eclipse-SDK-3.3M5 (I20070209-1006), cdt-4.0M5, emf-2.3M5 RSE install : RSE-SDK I20070219-1645 + discovery + efs java.runtime : Sun 1.4.2_13 os.name: : Windows XP 5.1, Service Pack 1 ------------------------------------------------ systemtype : Unix-ssh / Linux-dstore-processes targetos : Red Hat Enterprise Linux WS release 4 (Nahant Update 3) targetuname : Linux parser.takefive.co.at 2.6.9-34.EL #1 Fri Feb 24 16:44:51 EST 2006 i686 athlon i386 GNU/Linux targetvm : Sun Java HotSpot(TM) Client VM (build 1.4.2_13-b06, mixed mode) ------------------------------------------------
Kushal, since you're more involved in the wizard stuff as of late, I'm going to assign this to you for now. Feel free to give it back if you don't have time.
#2 is fixed (the NPE), but if we can get rid of the whole code, it better would be.
This is a bigger non-critical thing that we'll need to look at in M7. It is related to bug 176490 (wizard improvements).
Hmm... this is an API change in the sense that we'd change whether extenders can expect property pages being added to the wizard, or not. If we are to make this change for 2.0, I'd like to clearly understand 1. Why the code is there 2. Who would potentially use the code 3. Whether it currently works as expected or not I'd love to get rid of instantiating property pages for the wizard, because it is another reason for activating plugins too early and unnecessarily (bug #181939). But we need to clearly know what we are doing. Can we chat about this at the committer meeting? Please be prepared with answers.
In a large product based on Eclipse, it can be really problematic that each and every plugin contributing a Property Page may be activated here. We should investigate what the intention was of having this code, and whether it actually works or is needed. If it doesn't work as expected anyways, we should take it out for 2.0.1 -- it would not be an API change in that case because the API was never described and never actually worked.
(In reply to comment #5) > > We should investigate what the intention was of having this code, and whether > it actually works or is needed. If it doesn't work as expected anyways, we > should take it out for 2.0.1 -- it would not be an API change in that case > because the API was never described and never actually worked. > The original intention of this code was to allow existing property pages for subsystems to be configured in the new connection wizard. A particular interface needed to be implemented in order for it to be acceptable. As noted, there's a nasty side-effect of checking the type of the object for which the property page is enabled for - that it can load plugins that aren't needed/wanted. In the past, IBM products have used this for configurating dstore connections, however, we now have the services property pages to do the same thing and developers can override getWizardPages() if custom pages are desired. So I think we can safely remove this code.
I made some more investigations. 1. The code in question is only executed when looking at an ISubSystemConfiguration that is not an IServiceSubSystemConfiguration. In stock RSE, these are DeveloperSubSystemConfiguration and TestSubSystemConfiguration -- without these, nothing is reproducable. But note that commercial adopters are likely to have subsystems which are not service (Wind River does). 2. When any systemType has a non-servicesubsystem registered against it, the code is executed. In my workspace, it checks for 73 propertyPages, and loads 10 different interface classes specified in the objectClass attribute. Among these interface classes are: org.eclipse.cdt.debug.core.model.ICDebugTarget org.eclipse.core.resources.IProject org.eclipse.cdt.core.model.ITranslationUnit org.eclipse.jdt.core.IJavaProject org.eclipse.core.resources.IFile org.eclipse.core.resources.IFolder org.eclipse.team.internal.ccvs.ui.repo.RepositoryRoot org.eclipse.update.internal.ui.model.IFeatureAdapter org.eclipse.update.internal.core.SiteLocal This means that on wizard startup, the following plugins are activated: cdt.core, cdt.debug.core, core.resources, jdt.core, team.cvs.ui, update.ui The first 3 are no problem because they are in earlyStartup anyways. The other 3 are not activated by default -- and in a large product, there are potentially many more activated because of our wizard. 3. In order for the code to actually succeed eventually, the propertyPage must be registered against an ISubSystem that is created from the SubSystemConfiguration being investigated. Plus, the propertyPage must implement ISystemConnectionWizardPropertyPage in stock RSE, there are two implementations of this interface, both of which are registered against Dstore, which is an IServiceSubSystem, so nothing actually happens. 4. Moreover, for RSE-internal the mechanism got "deactivated" when we replaced the objectClass attribute of our property pages by the new not-deprecated <enabledWhen> / <instanceof> mechanism. I think we should really remove the code in question for 2.0.1 since I'm scared about the performance impact of activating at least 3 unnecessary plugins in a "small" installation -- so what about a really large installation? I do think that this is a change in functionality, but I do not consider it an API change since that functionality has never been documented (Javadoc of ISystemConnectionWizardPropertyPage and SystemSubSystemsPropertiesWizardPage is very very vague, and nobody says that propery pages can automatically be contributed as wizard pages -- it is just part of the default implementation in the SubSystemConfigurationAdapter. If anybody really really has a problem with this change, he can write his own SubSystemConfigurationAdapter, deriving from our default impl, and override the getNewConnectionWizardPages() method to restore the old functionality. That way, somebody could restore the 2.0 functionality in a 2.0.1 based product and it would work the same in both versions. So, I removed the [api] tag from this bug and give my go for the change in 2.0.1 -- I filed bug #197129 for a future API change to complete the task in the 3.0 timeframe.
DaveD -- do you agree that this change is OK for 2.0.1?
Created attachment 74145 [details] Patch to fix the performance issue and deprecate obsolete API Attached patch removes the offending code, and deprecates the offending methods for future removal. I think this is the best we can do for 2.0.1, and it is the right thing to do.
Based on the discussion, this sounds like exactly the right thing to do.
Makes sense to me. Martin, were you going to commit this?
Patch committed and released for I20070719-1300.
