Fix NPE in ClasspathMultiDirectory#directoryList()

[jdneo]

Signed-off-by: Sheng Chen sheche@microsoft.com

What it does

Fix a potential NPE issue in ClasspathMultiDirectory. the member this.directoryCache could be null if cleanup() is called

How to test

1. Use the sample code https://github.com/micronaut-projects/micronaut-examples/tree/master/hello-world-java
2. Enable Maven JDT APT (Window > Preferences > Maven > Annotation Processing > Automatically configure JDT APT)
3. Import as a Maven project
4. Build the project, and run the test case HelloControllerTest

Behavior without the change

1. Some class files are not generated into target/test-classes/example
2. The test case HelloControllerTest fails

Behavior with the change

1. class files are generated successfully into target/test-classes/example
2. The test case HelloControllerTest passes

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[fbricon]

@jdneo do you know why directoryCache is null in the context of those micronaut projects?

[iloveeclipse]

What is missing is same change in the parent class, and a test.

[jdneo]

@fbricon Not yet, I can investigate more tomorrow. But might need some time since I'm not quite familiar with the code base now.

If anyone has some background knowledge/hints that will be helpful :)

[jdneo]

@iloveeclipse yeah, I could do that after I get some certain understanding of this problem

[stephan-herrmann]

Fix a potential NPE issue in ClasspathMultiDirectory. the member this.directoryCache could be null if cleanup() is called

Scanning the call hierarchy of ClasspathDirectory.cleanup() I see many clients who systematically discard the NameEnvironment after cleanup, where the name environment should hold the only reference to the classpath directory. Hence, nobody should be able to see the null in directoryCache.

Seams like one of those clients didn't observe that discipline?

[jdneo]

I found that the same NPE issues can be found in bugzilla as well, but for different use cases.

For this PR, looks like it is caused by multiple instances reference the same NameEnvironment. See following screenshots:

When triggering update maven project, the code goes to:

eclipse.jdt.core/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/JavaBuilder.java

Lines 271 to 272 in efccc21

	BatchImageBuilder imageBuilder = new BatchImageBuilder(this, true, CompilationGroup.MAIN);
	BatchImageBuilder testImageBuilder = new BatchImageBuilder(imageBuilder, true, CompilationGroup.TEST);

And then, during the build, some typebindings are generated with the same nameEnvironment

After imageBuilder.build();, it starts to clean up

At this time, the directoryCache is set to null.

Then, the testImageBuilder starts to build and the code goes to:

And NPE happens.

I'm thinking that, maybe reset directoryCache is better than set it to null when cleaning up?

---

Update:

Looks like the root cause is that, the annotation processor dependency (In this case, it's micronaut) caches the AnnotationMemberValue in a static map, whose nameEnvironment has been cleaned up after building the main source set.
When building the test source set, the 'outdated' AnnotationMemberValue is picked from the cache and causes the NPE when it's trying to get the binding.

Though we can make sure we can discard the NameEnvironment after cleanup in JDT's implementation, but for those annotation processors there is no guarantee that the implementation is aware of this. That makes setting it to null kind of dangerous, I think.

To simply fix this issue, maybe we can just add null checks and return null directly if the directoryCache is null.

[stephan-herrmann]

To simply fix this issue, maybe we can just add null checks and return null directly if the directoryCache is null.

That sounds fair.

Still I'm a bit worried that perhaps we are leaking internal structures, inviting all kinds of unexpected usage.

@jarthana can you say a word on the life cycle of IdeBuildProcessingEnvImpl? It seems to hold on to stuff like a LookupEnvironment which may not be suitable for infinite usage by clients. Reminds me of similar problems where clean-up of scopes breaks clients of DOM bindings triggering on-demand resolving. How much out-off-band resolving can methods on TypeElementImpl and friends trigger, that might find the LE beyond its best-before date?

[jarthana]

@jarthana can you say a word on the life cycle of IdeBuildProcessingEnvImpl? It seems to hold on to stuff like a LookupEnvironment which may not be suitable for infinite usage by clients. Reminds me of similar problems where clean-up of scopes breaks clients of DOM bindings triggering on-demand resolving. How much out-off-band resolving can methods on TypeElementImpl and friends trigger, that might find the LE beyond its best-before date?

I will have to check, but a quick search for BaseProcessingEnvImpl.getLookupEnvironment() brings up a lot of results :)