Comment 1 Stephan Herrmann 2012-12-18 11:27:32 EST

That is: ExternalPackageFragmentRoot#externalPath is null.

Interestingly, this field is only ever assigned in constructors of that class, so it's not that an object of the class got corrupted by cleanup but it must have been created in the corrupt state.

I see a slight risk of concurrency spoiling this case:
Assume the thing was instantiated along this path:

JavaProject.getPackageFragmentRoot(IResource)
-> getPackageFragmentRoot(IResource, null)
-> if resource is link to external then we call
    new ExternalPackageFragmentRoot(resource, entryPath, this);
now:
 entryPath == null
thus we assign:
 this.externalPath = linkedFolder.getLocation()

If in this very moment s.t. in the linked resource structure got deleted we're hosed.


@Michael, do you think this is reproduceable?

Comment 2 Michael Rennie 2012-12-18 11:47:32 EST

(In reply to comment #1)
> That is: ExternalPackageFragmentRoot#externalPath is null.
> 
> Interestingly, this field is only ever assigned in constructors of that
> class, so it's not that an object of the class got corrupted by cleanup but
> it must have been created in the corrupt state.
> 
> I see a slight risk of concurrency spoiling this case:
> Assume the thing was instantiated along this path:
> 
> JavaProject.getPackageFragmentRoot(IResource)
> -> getPackageFragmentRoot(IResource, null)
> -> if resource is link to external then we call
>     new ExternalPackageFragmentRoot(resource, entryPath, this);
> now:
>  entryPath == null
> thus we assign:
>  this.externalPath = linkedFolder.getLocation()
> 
> If in this very moment s.t. in the linked resource structure got deleted
> we're hosed.
> 
> 
> @Michael, do you think this is reproduceable?

Curtis and I have both tried to reproduce, only the one time I managed to get the NPE, so I would say it is not very reproducable

Comment 3 Tobias Bertelsen 2013-05-10 07:29:08 EDT

Created attachment 230770 [details]
Stacktraces.

Comment 4 Tobias Bertelsen 2013-05-10 07:29:32 EDT

Hi there.

I have a setup that happens to provoke this error consistently. I have done a some debugging that might help.


In my case it happens in the following situation. Let A and B be plugin-projects. A depends on B. B is also present in the running platform as an 'unpacked' plugin.
I import A, B, and a lot of other stuff in a single WorkspaceModifyOperation in my own tool.

 - A is imported first.
 - During the WorkspaceModifyOperation an IResourceChangeEvent is sent to the DeltaProcessor.
    - During this, ExternalPackageFragmentRoots gets initialized with externalPath = null when DeltaProcessor$RootInfo is referring to an 'unpacked' plugin in the running platform.
      This is e.g. B
 - B is imported after that event.
 - When the WorkspaceModifyOperation terminates the ExternalPackageFragmentRoot pointing from A to B with externalPath = null is accessed and throws the NPE.

So essentially what happens is, that within a WorkspaceModifyOperation,
 - The ExternalPackageFragmentRoot A->B is created pointing to the running platform
 - It is later used when B is imported as a project. I suspect the code is assuming it "normal" PackageFragmentRoot, since B exists as a project.


Looking a little more into it, reveals that for a path external P (that points to a 'unpacked' plugin in the running platform) it holds:
 - externalPath = null for an ExternalPackageFragmentRoots created with P in DeltaProcessor$RootInfo.getPackageFragmentRoot()
 - externalPath = P for an ExternalPackageFragmentRoots created with P in JavaProject.computePackageFragmentRoots(...)
Further I have not experienced problems for dependencies in the target platform, that is not imported, i.e., externalPath *can* be null when ExternalPackageFragmentRoots is pointing to the target platform.

I'll attach stacktrace of where the externalPath = null contruction takes place. 
I have only provoked this problem with a run-configuration. More details below.



***My setup***
I'm creating a eclipse setup tool, that automatically adds projects to eclipse, and doing a little dog-food testing.
I have our repository checked out twice "dev" and "test". To test my tool, I develop/build in "dev" and start an eclipse application run configuration with these plugins from "dev". I then try to setup a workspace in "test" importing my own tool, i.e., import the duplicates of the plugins I am testing.
My target platform is based on an eclipse installation.


***Construction with "externalPath = null"***
I've detected when the ExternalPackageFragmentRoot got initialized with the null path. It seems to be related to external folders and the DeltaProcessor.
What happens (in my case) is the following.
 
- In "DeltaProcessor.RootInfo.getPackageFragmentRoot()" the field "rootPath" is an absolute path.
- Based on this "JavaModel.getTarget(IPath, boolean)" returns an IFolder with a path like this "/.org.eclipse.jdt.core.external.folders/.link0"
- This is passed to "JavaProject.getPackageFragmentRoot(IResource)" which creates the ExternalPackageFragmentRoot with "externalPath = null"
- In ExternalPackageFragmentRoot:49 "externalPath == null" is true and "linkedFolder.getLocation()" returns null, since there are no project called ".org.eclipse.jdt.core.external.folders"

When the same paths are handled in "computePackageFragmentRoots" the ExternalPackageFragmentRoot is created fine, since the non-null "entryPath" is passed along to "getPackageFragmentRoot(IResource, IPath)".

The rootPath was in all cases locations of plugins that the imported projects depended on. In my case, it was references to two categories of folders:
 1. The "bin" folder of some of my projects in "dev"!
 2. The location of 'unpacked' plugins in the target platform
I.e. folders (not jars) of plugins in the running platform. These plugins are all dependencies of some of the imported projects. 


***NPE***
I got multiple unhandled NPE's, but they all refered to the same external folder. This folder was the only one that satisfied these characteristics:
 1. It pointing to a plugin that was in the running platform and unpacked.
 2. The plugin contianed .class files
 3. It (or its duplicate) was a dependenciy of plugins imported *before* the IResourceChangeEvent.
 4. A dubplicate of the plugin was imported *after* the IResourceChangeEvent

I think the first tree characteristics is the reason why the ExternalPackageFragmentRoot with "externalPath = null" is created.
I think the last two characteristics is the reason why the ExternalPackageFragmentRoot is used with the

Comment 5 Tobias Bertelsen 2013-08-09 06:06:48 EDT

I found a workaround for this bug (in my case).

To sum up my previous comment, this bug happens when change notifications are fired *during* a long WorkspaceJob or WorkspaceModifyOperation. In this case the workspace can be inconsistent in some way that the JDT can't handle.

I came across this[1] 8+ year old post that dealt with these "halfway" change notifications by simple disabling them. I tried it out and it works for me.

This is however a workaround and not a fix. In my opinion the JDT should be robust towards the "halfway" change notifications.

-Tobias

PS: Perhaps this bug should be renamed to something like "ExternalPackageFragmentRoot constructed with null as externalPath"


[1] http://www.eclipse.org/forums/index.php/t/94630/

Comment 6 Tobias Bertelsen 2013-08-12 05:46:34 EDT

The workaround previously posted came with some other issues, so dug into this again to find a better solution and came closer to the core cause of the problem.

Essentially this happens when an ExternalPackageFragmentRoot is created before the  project ".org.eclipse.jdt.core.external.folders" is created. In my case this happens when "DeltaProcessingState.resourceChanged(...)" is called *during* a WorkspaceJob that imports a lot of projects.

A solution to this could be to create ".org.eclipse.jdt.core.external.folders" somewhere in "resourceChanged(...)" or deeper in the call stack. In my case this is however not trivial, since "resourceChanged(...)" is called while another process has workspace lock.

To work around this I manually call "JavaModelManager.getExternalManager().createExternalFoldersProject(new NullProgressMonitor());" as the first thing in my job importing the projects. In this way the external folders project is created within the workspace lock, before any resource change events are fired.

I do not have the knowledge of the JDT to figure out where would be the right place to call "ExternalFoldersManager.createExternalFoldersProject(...)" to handle these halfway resource changed events.

Comment 7 Ed Merks 2014-11-22 08:39:44 EST

I've committed a fix for this bug for Gerrit review:

https://git.eclipse.org/r/36879

It's clear that the current code is just wrong.  I.e., in org.eclipse.jdt.internal.core.DeltaProcessor.RootInfo.getPackageFragmentRoot() it's clearly expecting to create a package fragment root for the case of an IResource that might not yet exist.

		public IPackageFragmentRoot getPackageFragmentRoot() {
			IPackageFragmentRoot tRoot = null;
			Object target = JavaModel.getTarget(this.rootPath, false/*don't check existence*/);
			if (target instanceof IResource) {
				tRoot = this.project.getPackageFragmentRoot((IResource)target, this.rootPath);


An in fact, for external folders, the links are created in a delayed fashion by design so this method is definitely called in cases where IResource.exists if false.  

The current logic calls this in JavaProject

	public IPackageFragmentRoot getPackageFragmentRoot(IResource resource) {
		return getPackageFragmentRoot(resource, null/*no entry path*/);
	}

I.e., it forwards the null entry path to this method, which uses that null path, for the case of an external folder, to the ExternalPackageFragmentRoot constructor:

	IPackageFragmentRoot getPackageFragmentRoot(IResource resource, IPath entryPath) {
		switch (resource.getType()) {
			case IResource.FILE:
				return new JarPackageFragmentRoot(resource, this);
			case IResource.FOLDER:
				if (ExternalFoldersManager.isInternalPathForExternalFolder(resource.getFullPath()))
					return new ExternalPackageFragmentRoot(resource, entryPath, this);
				return new PackageFragmentRoot(resource, this);
			case IResource.PROJECT:
				return new PackageFragmentRoot(resource, this);
			default:
				return null;
		}
	}

That doesn't always break because the constructor does this:

		this.externalPath = externalPath == null ? linkedFolder.getLocation() : externalPath;

But in the case that linkedFolder.exists() is false, a case that definitely happens by design because of this:

   Object target = JavaModel.getTarget(this.rootPath, false/*don't check existence*/);

The externalPath field will be null and the equals, getElementName and hashCode methods don't expect this and do not cope with it gracefully producing the stack trace reported in this Bugzilla.

As a workaround I had to write code like this in Oomph:

        // JDT lazily creates the external folder links,
        // but if many projects are imported, the resource deltas are processed early,
        // and then JDT creates ExternalPackageFragmentRoot instances for which linkedFolder.getLocation() is null,
        // which causes null pointer exceptions in that class' hashCode method, this.externalPath.hashCode(),
        // which completely screws up PDEs own notification handling
        // causing it to handle the same deltas more the once, making a mess of the PDE's model.
        // JDT doesn't create the links until the AutoBuildJob runs, which happens very late in the overall processing cycle.
        org.eclipse.jdt.internal.core.ExternalFoldersManager externalFoldersManager = org.eclipse.jdt.internal.core.ExternalFoldersManager.getExternalFoldersManager();
        for (IPluginModelBase pluginModelBase : PluginRegistry.getExternalModels())
        {
          String installLocation = pluginModelBase.getInstallLocation();
          if (installLocation != null)
          {
            if (new File(installLocation).isDirectory())
            {
              externalFoldersManager.createLinkFolder(new Path(installLocation), false, null);
            }
          }
        }

Comment 8 Stephan Herrmann 2014-11-22 09:45:23 EST

Hi Ed,

thanks for the analysis and the patch!

Do you think it would be possible to create a reproducing test from your analysis? Could the Oomph scenario help to invent such a test?

Comment 9 Ed Merks 2014-11-22 10:55:11 EST

It would be hard to write a test case for something like this.  One needs to import so many projects at once that resource deltas are processed before they're all done.  Note the TP has to have classpaths that are folders.

If you provision an Oomph setup environment for Oomph with Oomph, remove the "workaround" code from

org.eclipse.oomph.targlets.internal.core.WorkspaceIUImporter.updateWorkspace(Set<WorkspaceIUInfo>, IProgressMonitor)

Then launch the "IDE" launcher, and in that IDE using the Oomph Project Importer to import all the JDT.setup's project, it will reproduce this problem consistently.

Comment 10 Jay Arthanareeswaran 2014-11-28 02:24:51 EST

I agree with Ed's findings and analysis. Although it would be nice to have a test case, the patch looks safe to me. 

Stephan, if you are fine with the patch, we can go ahead without the test.

Comment 11 Dani Megert 2014-11-28 04:16:39 EST

(In reply to Jayaprakash Arthanareeswaran from comment #10)
> I agree with Ed's findings and analysis. Although it would be nice to have a
> test case, the patch looks safe to me. 

Jay, can you please mark this on the Gerrit change? Thanks.

Comment 12 Jay Arthanareeswaran 2014-12-09 23:20:28 EST

(In reply to Dani Megert from comment #11)
> Jay, can you please mark this on the Gerrit change? Thanks.

Done. We will release this early M5. Sorry about it.

Comment 13 Jay Arthanareeswaran 2015-01-06 09:36:12 EST

Resolved via bug 449780.

*** This bug has been marked as a duplicate of bug 449780 ***

Comment 14 Sasikanth Bharadwaj 2015-01-28 04:58:16 EST

Verified for 4.5 M5 using I20150127-0900 build

Comment 15 Stephan Herrmann 2015-09-15 15:50:33 EDT

Granting a long overdue review. Patch looks good! I just wonder if we may need more of that, filed bug 477504 to follow up.