Community
Participate
Working Groups
Hi, I get a NPE in org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.StylingPreferencesHandler.getPreferences(): protected Set<IEclipsePreferences> getPreferences() { if (prefs == null) { prefs = new HashSet<IEclipsePreferences>(); PlatformAdmin admin = WorkbenchSWTActivator.getDefault() .getPlatformAdmin(); State state = admin.getState(false); //<-- NPE In our application getPlatformAdmin() returns null and hence admin.getState(false) throws a NPE.
You need the org.eclipse.osgi.compatibility.state fragment in your product. PW
Pushed change to gerrit: https://git.eclipse.org/r/#/c/27352/
Can we however apply my patch? I think it still makes sense
(In reply to Matthias Nick from comment #3) > Can we however apply my patch? I think it still makes sense You can't return null from that method, it'll blow up. Plus, you'll have rendered some part of the system completely inoperable. PW
(In reply to Paul Webster from comment #4) > > You can't return null from that method, it'll blow up. Plus, you'll have > rendered some part of the system completely inoperable. That might be acceptable in your case (I believe the CSS will no longer work correctly). What if you just return the empty Set instead? PW
In my patch I don't return null, I return the empty Set: prefs = new HashSet<IEclipsePreferences>(); if (admin == null) { return prefs; }
Sorry, I overlooked that. Comments on the review. Maybe it should log something about CSS preferences (as used inthe dark theme) being disabled. PW
Hi Paul, thanks for the review. I uploaded a new pach set
Can we ship the patch with the Luna Release? This would be important to our project.
*** Bug 436016 has been marked as a duplicate of this bug. ***
I'm -0.5 to this fix as it only masks the issue, and may possibly lead to unexplained behaviour (preference is set, but not "found"). Pure RCP apps should be using the org.eclipse.e4.rcp or org.eclipse.rcp features, which includes the org.eclipse.osgi.compatibility.state bundle. Products that create custom feature definitions need to review those definitions after every new release as new bundles may have been created. The right solution here is to rewrite the code not to use PlatformAdmin. And probably shouldn't be caching preference nodes for all bundles (not great when there are hundreds and hundreds of bundles) and instead pull them in on-demand.
I believe this is the root cause of Tycho bug 436159 and will affect all Tycho users that run UI-based tests against Luna. Although it is possible to workaround the problem by making PlatformAdmin available during the test, the workaround is rather tedious and will almost certainly result in many user questions. Given the impact, I think this bug should be addressed for Luna.
The RCP features need to include compatibility.state because PlatformAdmin is used in various places throughout the SDK. So it might still go in the migration guide. We might be able to fix this, although at this point I'm not sure if it will be Luna or Luna SR1. PW
Possible fix: https://git.eclipse.org/r/27544 PW
(In reply to Brian de Alwis from comment #11) > And probably shouldn't be caching preference nodes for all bundles (not > great when there are hundreds and hundreds of bundles) and instead pull them > in on-demand. I've opened Bug 436183 to look at getting them on demand. PW
(In reply to Paul Webster from comment #14) > Possible fix: https://git.eclipse.org/r/27544 > > PW Those changes are quite big for RC4. Would Matthias's proposed change be enough to avoid the Tycho bug 436159? If so, I'd take that fix and then get rid of PackageAdming for SR1.
(In reply to Dani Megert from comment #16) > (In reply to Paul Webster from comment #14) > > Possible fix: https://git.eclipse.org/r/27544 > > > > PW > > Those changes are quite big for RC4. Would Matthias's proposed change be > enough to avoid the Tycho bug 436159? If so, I'd take that fix and then get > rid of PackageAdming for SR1. I believe https://git.eclipse.org/r/#/c/27352/2 is sufficient to address test failures in Tycho.
(In reply to Dani Megert from comment #16) > (In reply to Paul Webster from comment #14) > > Possible fix: https://git.eclipse.org/r/27544 > > > > PW > > Those changes are quite big for RC4. Would Matthias's proposed change be > enough to avoid the Tycho bug 436159? If so, I'd take that fix and then get > rid of PackageAdming for SR1. The changes are pretty straightforward. I'm not sure I like the alternative of returning an empty collection if PlatformAdmin is not available. Seems like that would cause other hard to debug issues later if we really are expecting the collection of preferences to be returned with all the installed bundle preferences.
(In reply to Dani Megert from comment #16) > > Those changes are quite big for RC4. Would Matthias's proposed change be > enough to avoid the Tycho bug 436159? If so, I'd take that fix and then get > rid of PackageAdming for SR1. Bug 436016 provided a test project that fails without the compat.state fragment. Both of the fixes provided remove the problems associated with that test app, so it's probably fair to say that Matthias' proposal would avoid the tycho bug as well. I'd split the risk between the patches at 50/50. https://git.eclipse.org/r/#/c/27352/ will simply prevent the NPE and will disable the CSS->styling preference bridge. Any RCP app that needs the CSS/preferences to work would have to add the compat.state fragment to their build (churn) and that fragment would then become unnecessary in SR1. https://git.eclipse.org/r/27544 is more code (so by definition more risk). But it keeps the CSS->styling preference bridge functioning without any change to an RCP build (no extra fragment). And the pattern: BundleContext context = WorkbenchSWTActivator.getDefault() .getContext(); for (Bundle bundle : context.getBundles()) { if (bundle.getSymbolicName() != null) { prefs.add(InstanceScope.INSTANCE.getNode(bundle .getSymbolicName())); } } of walking through bundles from the BundleContext is safe and used in other areas of the platform/workbench and I feel that helps mitigate the risk. I'd still prefer to go with the BundleContext fix, but am willing to discuss further. PW
I'm +1 for Paul's fix for 4.4.
I was asked to review this again to make sure I am 100% confident in the fix. While it is RC4 I do think Paul's fix is the right fix now. The use of a read-only PlatformAdmin State has several drawbacks. 1) it requires the compatibility state which is the source of this bug and results in the NPE when not present 2) the call to the read-only State.getBundles() is very expensive operation the first time it is called. This is because the BundleDescriptions in the read-only state are created lazily. They are basically a complete copy of all the meta-data already present in the BundleRevision objects used natively in the framework for Bundle meta-data. Ideally we don't have code that calls this method in the Eclipse SDK so we can avoid the overhead of these rather expensive objects to create and keep in memory. 3) it is true that the fix involves more code changes, but the end result is less code overall, which is a good thing.
(In reply to Thomas Watson from comment #21) > I was asked to review this again to make sure I am 100% confident in the > fix. While it is RC4 I do think Paul's fix is the right fix now. Right, we had a chat where you saw a 5% risk. Thanks for ruling that out. +1 for that fix.
Brian, could you please update the flag? PW
Thanks everybody. Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=39847f806ee52cc6e665b2002fbe0595f11e2b4a PW
Thanks from my side as well :)
Tested successfully with org.eclipse.e4.ui.workbench.swt 0.12.100.v20140530-1436. Our problem is gone now!
*** Bug 436858 has been marked as a duplicate of this bug. ***
