Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 320129

Summary: NPE in J2EEComponentClasspathContainer.getBaseEARLibRefs
Product: [WebTools] WTP Java EE Tools Reporter: Jason Sholl <jsholl>
Component: jst.j2eeAssignee: Jason Sholl <jsholl>
Status: RESOLVED DUPLICATE QA Contact: Chuck Bridgham <cbridgha>
Severity: normal    
Priority: P3 CC: ccc, david_williams
Version: 3.2Flags: deboer: pmc_approved+
cbridgha: review+
Target Milestone: 3.2.2   
Hardware: PC   
OS: Windows Server 2003   
Whiteboard:
Attachments:
Description Flags
patch for 3.2.1 none

Description Jason Sholl CLA 2010-07-16 13:05:22 EDT 
It is possible to NPE in J2EEComponentClasspathContainer.getBaseEARLibRefs if a very specific thread racing condition occurs.  Towards the top of the method a component's references are fetched.  By the time these references are used later in the method, it is possible they are stale and if so, the following NPE results.  The fix is simple and safe.  If we do not fix this, then the NPE will surface to the user in the form of an error dialog with the exception.

The specific use case involves rapidly adding and removing a Web Module from an EAR.

This stack trace is from last week's declared build.

!ENTRY org.eclipse.jst.j2ee 4 2 2010-07-15 18:00:40.451
!MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.jst.j2ee".
!STACK 0
java.lang.NullPointerException
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathContainer.getBaseEARLibRefs(J2EEComponentClasspathContainer.java:364)
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathContainer.requiresUpdate(J2EEComponentClasspathContainer.java:131)
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathContainer.refresh(J2EEComponentClasspathContainer.java:427)
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathUpdater$ModuleUpdateJob.processModules(J2EEComponentClasspathUpdater.java:302)
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathUpdater$ModuleUpdateJob.access$4(J2EEComponentClasspathUpdater.java:286)
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathUpdater$ModuleUpdateJob$1.run(J2EEComponentClasspathUpdater.java:327)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathUpdater$ModuleUpdateJob.run(J2EEComponentClasspathUpdater.java:312)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Comment 1 Jason Sholl CLA 2010-07-16 13:07:00 EDT 
Created attachment 174517 [details]
patch for 3.2.1
Comment 2 Chuck Bridgham CLA 2010-07-19 10:02:04 EDT 
approved
Comment 3 Jason Sholl CLA 2010-07-19 15:56:19 EDT 
    * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. 

This NPE prevents the classpath container from properly resolving.  This is timing related and happens rarely, but one user was able to consistently reproduce.

    * Is there a work-around? If so, why do you believe the work-around is insufficient? 

No

    * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 

Tested with UI.  No JUnit has been added.

    * Give a brief technical overview. Who has reviewed this fix? 

Simple null check.  Chuck Bridgham has reviewed

    * What is the risk associated with this fix? 

none.
Comment 4 David Williams CLA 2010-07-20 11:44:46 EDT 
I don't mean to say we should not fix this ... but, we did say "blocking or major regressions only" this week. From the description, it doesn't sound like "blocking or major regression". And, it is classified as "normal". And you say it "happens rarely". So does it really belong in final re-build for 3.2.1? Or would 3.2.2 suffice? 

Also ... the patch itself contains some formatting changes, so it is hard to see exactly what's changed and what hasn't. Is it possible the attach a more "changes only" patch for review? Or is the current one just the say CVS deals with indentation, and no improvements possible. 

For example, if I'm seeing it right, it look like one line changed from 

if (!libDir.equals(earComp.getReference(component.getName()).getRuntimePath().toString()))

to

if (!libDir.equals(componentRef.getRuntimePath().toString())) {

which seems like more then a "null check". 

But ... I might be comparing the wrong lines? 


So, improve the patch formatting if you can, but in either case, I think more justification needs to be be provided.  Is this a regression? Does it prevent (block) some particular use-cases from being tested? (If so, seems seems severity could be changed?) 

Thanks,
Comment 5 Jason Sholl CLA 2010-07-20 21:16:11 EDT 
This is a simple null check.  A block of code was wrapped with the null check which is why it is so ugly.

But, yes, you are right, this is not a critical problem; moving it out to 3.2.2 and removing the PMC flags.
Comment 6 Jason Sholl CLA 2010-08-04 14:43:12 EDT 
defect 320729 fixed this

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