Comment 1 Szymon Ptaszkiewicz 2015-07-29 10:33:36 EDT

This looks like a dup of bug 473097, see bug 473097 comment 1 and bug 473097 comment 3.

Comment 2 Dani Megert 2015-08-03 04:52:11 EDT

*** Bug 473097 has been marked as a duplicate of this bug. ***

Comment 3 Lars Vogel 2015-08-03 04:55:57 EDT

Simon, looks like Bug 445663 was not sufficient to remove the uninstalled part descriptors from the application model? Can you please have a look?

Comment 4 Szymon Ptaszkiewicz 2015-08-24 04:43:37 EDT

*** Bug 475628 has been marked as a duplicate of this bug. ***

Comment 5 Eclipse Genie 2015-09-03 19:57:00 EDT

New Gerrit change created: https://git.eclipse.org/r/55249

Comment 6 Simon Scholz 2015-09-03 20:06:05 EDT

This patch saves the bundlesymbolicname and class of the 3.x ViewPart as persisted state on its MPartDescriptor in order use this information to check if the plugin for this ViewPart is available at runtime.

Comment 7 Lars Vogel 2015-09-11 12:42:25 EDT

Sorry Simon for the late feedback. I will review early M3.

Comment 8 Eclipse Genie 2015-10-05 08:59:04 EDT

Gerrit change https://git.eclipse.org/r/55249 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=040baab905f3add0f9007d86b27720330f6044d3

Comment 9 Lars Vogel 2015-10-05 09:25:50 EDT

Thanks Simon, looks fine. This fix removes invalid views which are added after this fix, not views which were added before but I also did not find another way to implement this. 

Everyone who has this situation, needs to start Eclipse once with -clearPersistedState, after this patch invalid views should be removed.

Comment 10 Lars Vogel 2015-10-05 09:26:09 EDT

I leave the bug open for a potential backport.

Comment 11 Lars Vogel 2015-10-05 13:10:16 EDT

(In reply to Lars Vogel from comment #10)
> I leave the bug open for a potential backport.

+1 for backport from additional committer and project lead

Comment 12 Lars Vogel 2015-10-07 05:23:33 EDT

Unfortunately the check triggers bundles activation, which is undesired.

Comment 13 Lars Vogel 2015-10-07 05:23:53 EDT

(In reply to Lars Vogel from comment #11)
> (In reply to Lars Vogel from comment #10)
> > I leave the bug open for a potential backport.
> 
> +1 for backport from additional committer and project lead

-1

Comment 14 Lars Vogel 2015-10-07 05:26:34 EDT

ModelCleanupAddon tests of a class is still present by loading it, this triggers activation of the plugins which are configured to be started lazy.

private Class<?> findClass(String className, BundleWiring wiring) {
		if (wiring == null) {
			return null;
		}
		if ((wiring.getRevision().getTypes() & BundleRevision.TYPE_FRAGMENT) != 0) {
			// fragment case; need to get the host wiring
			wiring = wiring.getRequiredWires(HostNamespace.HOST_NAMESPACE).get(0).getProviderWiring();
		}
		try {
			return wiring.getClassLoader().loadClass(className);
		} catch (ClassNotFoundException e) {
			return null;
		}
	}

Simon, can you please provide a revert of this fix, so that we can investigate a better solution to check for the presence of a class?

Comment 15 Eclipse Genie 2015-10-07 05:39:53 EDT

New Gerrit change created: https://git.eclipse.org/r/57603

Comment 16 Eclipse Genie 2015-10-07 06:02:06 EDT

Gerrit change https://git.eclipse.org/r/57603 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=78b70ded41595fe72eb07d61d30ec38d08187f42

Comment 17 Lars Vogel 2015-10-07 06:02:50 EDT

(In reply to Eclipse Genie from comment #16)
> Gerrit change https://git.eclipse.org/r/57603 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=78b70ded41595fe72eb07d61d30ec38d08187f42

Reverted the fix, Simon will check with Equinox if there is a way to check for the existing of a class without triggering activation in OSGi.

Comment 18 Dani Megert 2015-10-07 11:20:51 EDT

(In reply to Lars Vogel from comment #17)
> (In reply to Eclipse Genie from comment #16)
> > Gerrit change https://git.eclipse.org/r/57603 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> > ?id=78b70ded41595fe72eb07d61d30ec38d08187f42
> 
> Reverted the fix, Simon will check with Equinox if there is a way to check
> for the existing of a class without triggering activation in OSGi.

Thank you! I verified it in my workspace that the test now passes again.

Comment 19 Thomas Watson 2015-10-07 17:20:28 EDT

(In reply to Lars Vogel from comment #17)
> (In reply to Eclipse Genie from comment #16)
> > Gerrit change https://git.eclipse.org/r/57603 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> > ?id=78b70ded41595fe72eb07d61d30ec38d08187f42
> 
> Reverted the fix, Simon will check with Equinox if there is a way to check
> for the existing of a class without triggering activation in OSGi.

Sorry I was trying to submit a change to gerrit and I somehow pushed it directly to master:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3351720b8d7805492868333932e7ed90fca0b37d

The scary thing is I did NOT think I was a committer to platform.ui.  I obviously am, otherwise I would have expected my push to master to fail ... or else there is a serious hole in our git committer check on the server.

I probably should revert, but instead could someone have a look at my change?

Comment 20 Dani Megert 2015-10-08 03:24:32 EDT

(In reply to Thomas Watson from comment #19)
> Sorry I was trying to submit a change to gerrit and I somehow pushed it
> directly to master:
> 
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3351720b8d7805492868333932e7ed90fca0b37d
> 
> 
> The scary thing is I did NOT think I was a committer to platform.ui.  I
> obviously am, otherwise I would have expected my push to master to fail ...
> or else there is a serious hole in our git committer check on the server.

You are a committer. I think that happened when we folded some components into Platform UI.


> I probably should revert, but instead could someone have a look at my change?

With this fix most view descriptors are considered invalid and get removed. The IDE is unusable. However, I'm not 100% sure it is your new code or rather a bug in ModelCleanupAddon.isValidPartDescriptor(Bundle, MPartDescriptor).

I've reverted the fix with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e0cf683becc868b01b43aad83d21cb64843307b1

Comment 21 Dani Megert 2015-10-08 05:01:19 EDT

(In reply to Dani Megert from comment #20)
> With this fix most view descriptors are considered invalid and get removed.
> The IDE is unusable. However, I'm not 100% sure it is your new code or
> rather a bug in ModelCleanupAddon.isValidPartDescriptor(Bundle,
> MPartDescriptor).

This was when starting tests or a new workspace from my IDE. It looks like the problem does not happen when using the clean N20151007-2000 build itself.

Comment 22 Eclipse Genie 2015-10-08 08:08:18 EDT

New Gerrit change created: https://git.eclipse.org/r/57709

Comment 23 Simon Scholz 2015-10-08 08:15:09 EDT

I've uploaded a new patch, which combines Thomas Watson's and my latest changes.
Unfortunately the classes are not found, even they should be available.
The checkClassResource method also return false in case the desired class is available.

Therefore I gave -1 for the patch, but at least it may be tested with the latest changes.

Comment 24 Dani Megert 2015-10-08 08:22:19 EDT

(In reply to Dani Megert from comment #21)
> (In reply to Dani Megert from comment #20)
> > With this fix most view descriptors are considered invalid and get removed.
> > The IDE is unusable. However, I'm not 100% sure it is your new code or
> > rather a bug in ModelCleanupAddon.isValidPartDescriptor(Bundle,
> > MPartDescriptor).
> 
> This was when starting tests or a new workspace from my IDE. It looks like
> the problem does not happen when using the clean N20151007-2000 build itself.

Additional hint: in my workspace most of the stuff is in source. That might explain the different results.

Comment 25 Thomas Watson 2015-10-08 10:04:11 EDT

I rebased and modified the gerrit review at:

https://git.eclipse.org/r/#/c/57709/2

I made a very dumb mistake by calling BundleWiring.findEntries instead of BundleWiring.listResources.

The findEntries method only looks in the local bundle file and attached fragments for the resource.  The listResources searches the bundles dependencies (Import-Package, Require-Bundle and its local classpath as specified by Bundle-ClassPath header).  Basically identical to the search done to find and load a Class.

That explains why Dani saw lots of errors when having most of the source loaded in the workspace.  There 'bin' is added to the classpath and the root of the bundle file is the project so searching for .class files from the root of the project with findEntries will mostly not work.  The use findEntries of also was failing when a bundle was contributing a class from some dependency

Comment 26 Simon Scholz 2015-10-08 10:46:22 EDT

(In reply to Markus Keller -- away till Oct 13 from comment #0)
> - open the org.eclipse.ui.tests project and start a runtime workbench
>   => "Show View" dialog shows the "Error handling" category with a few views
> 
> - close org.eclipse.ui.tests and restart the runtime with the same workspace
>   => "Show View" dialog still shows the "Error handling" category, but the
> contained views have a red icon, and they naturally fail to open, since the
> contributing plug-in is gone.
> 

I just tried the latest change https://git.eclipse.org/r/#/c/57709/2 and followed the steps above and it works fine for me.

One view('org.eclipse.ui.tests.api.workbenchpart.ViewWithMissingClass') without class reference from the org.eclipse.ui.tests plugin is removed by the ModelCleanupAddon and logs this message:

!ENTRY org.eclipse.e4.ui.workbench 2 0 2015-10-08 16:34:08.451
!MESSAGE Removing part descriptor with the 'org.eclipse.ui.tests.api.workbenchpart.ViewWithMissingClass' id and the 'ERROR: View pointing to invalid class' description. Points to the invalid 'bundleclass://org.eclipse.ui.workbench/org.eclipse.ui.internal.e4.compatibility.CompatibilityView' class.

I hope this removal does not cause any undesired side effects.

Comment 27 Lars Vogel 2015-10-08 10:49:57 EDT

Simon, did you check if the Plugins from Dani's comment get activated? They should not. You can use the bundle spy to see their state.

Comment 28 Lars Vogel 2015-10-08 14:41:55 EDT

(In reply to Lars Vogel from comment #27)
> Simon, did you check if the Plugins from Dani's comment get activated? They
> should not. You can use the bundle spy to see their state.

Ups, sorry I just noticed that Dani placed that information into the wrong bug. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=478333#c26 for details. This is the list for plug-ins:

junit.framework.AssertionFailedError: Wrong bundles loaded:
- org.eclipse.ant.ui
- org.eclipse.debug.ui
- org.eclipse.help.base
- org.eclipse.help.ui
- org.eclipse.jdt.debug
- org.eclipse.jdt.debug.ui
- org.eclipse.jdt.junit
- org.eclipse.pde.runtime
- org.eclipse.search
- org.eclipse.team.cvs.ui
- org.eclipse.ui.cheatsheets
- org.eclipse.ui.console
- org.eclipse.ui.browser

Comment 29 Dani Megert 2015-10-09 03:54:30 EDT

(In reply to Lars Vogel from comment #28)
> (In reply to Lars Vogel from comment #27)
> > Simon, did you check if the Plugins from Dani's comment get activated? They
> > should not. You can use the bundle spy to see their state.

Or simply run org.eclipse.jdt.text.tests.PluginsNotLoadedTest.testPluginsNotLoaded
from 'org.eclipse.jdt.text.tests'.

The other check is to make sure that things still work (see comment 20).

Comment 30 Simon Scholz 2015-10-13 19:29:12 EDT

The org.eclipse.jdt.text.tests.PluginsNotLoadedTest.testPluginsNotLoaded test
from 'org.eclipse.jdt.text.tests' passes properly now.

I also checked this with the osgi console and the bundle spy.
So it seems to be fine from this side.

Concerning the actual patch I checked if it works for e3 parts from the org.eclipse.ui.tests project, as well as with e4 parts, which are provided by the e4 tools. So on this side everything seems to be fine either.

The IDE now only complains about missing handler implementations, like already did before, which I want to tackle next, once this patch is merged and verified.

Comment 31 Eclipse Genie 2015-10-15 12:42:56 EDT

Gerrit change https://git.eclipse.org/r/57709 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=172fa0ab28e34ea3d35b094198433abf66700b7e

Comment 32 Simon Scholz 2015-10-15 12:44:20 EDT

.

Comment 33 Lars Vogel 2015-10-20 15:22:09 EDT

*** Bug 480206 has been marked as a duplicate of this bug. ***

Comment 34 Kalyan Prasad Tatavarthi 2016-01-28 05:50:32 EST

The fix for this bug has not been backported to 4.5.2 yet.

Comment 35 Dani Megert 2016-01-28 06:45:43 EST

(In reply to Kalyan Prasad Tatavarthi from comment #34)
> The fix for this bug has not been backported to 4.5.2 yet.

==> reopening

Comment 36 Lars Vogel 2016-01-28 07:58:00 EST

(In reply to Kalyan Prasad Tatavarthi from comment #34)
> The fix for this bug has not been backported to 4.5.2 yet.

We need additional committers to give +1 for the backport.

Comment 37 Dani Megert 2016-02-03 08:04:32 EST

Too late for 4.5.2.