Bug 209961 - [compiler][apt] NPE in apt processing
Summary: [compiler][apt] NPE in apt processing
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 critical (vote)
Target Milestone: 3.3.2   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 207944 236255 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-11-15 10:29 EST by Olivier Thomann CLA
Modified: 2008-06-09 13:28 EDT (History)
7 users (show)

See Also:
philippe_mulet: pmc_approved+


Attachments
Proposed fix (876 bytes, patch)
2007-11-15 10:30 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix (876 bytes, patch)
2007-11-15 12:42 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed fix (6.92 KB, patch)
2007-11-15 12:48 EST, Olivier Thomann CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Thomann CLA 2007-11-15 10:29:43 EST
When compiling in 1.6 mode, if several units have been retrieved through accept(ICompilationUnit) in the compiler, the processAnnotations loop is boggus.
It is using this.unitsToProcess.length for the top value instead of this.totalUnits.
The consequence is:
java.lang.NullPointerException
	at org.eclipse.jdt.internal.compiler.Compiler.processAnnotations(Compiler.java:659)
	at org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:374)
	at org.eclipse.jdt.internal.compiler.batch.Main.performCompilation(Main.java:3411)
	at org.eclipse.jdt.internal.compiler.batch.Main.compile(Main.java:1565)
	at org.eclipse.jdt.core.tools.compiler.Compile.main(Compile.java:94)

This should be backported to 3.3.2.
Comment 1 Olivier Thomann CLA 2007-11-15 10:30:32 EST
Created attachment 82968 [details]
Proposed fix
Comment 2 Olivier Thomann CLA 2007-11-15 10:32:00 EST
Moving to JDT/Core as the loop in located in the Compiler itself.
Comment 3 Olivier Thomann CLA 2007-11-15 12:42:08 EST
Created attachment 82983 [details]
Proposed fix
Comment 4 Olivier Thomann CLA 2007-11-15 12:45:36 EST
Released for 3.4M4.
Walter, please verify. Thank you.

The last patch also includes the regression test.

Philippe, I think this is worth being backported to 3.3.2 since it is causing a bad NPE.
Comment 5 Olivier Thomann CLA 2007-11-15 12:48:32 EST
Created attachment 82985 [details]
Proposed fix

I forgot to include the corresponding change in the EclipseFileManager located in compiler.apt in the last patch.
Comment 6 Walter Harley CLA 2007-11-15 13:37:42 EST
With the patch, all tests pass and manual testing of the product, including file generation with both Java 5 and Java 6 processors active, appears to work correctly.  I am +1 for putting this into 3.3.2.

One very small comment: It would be nice to rename the "iterable" variable in EclipseFileManager to something more descriptive, like "oldClasspath".  

Further discussion:

This bug is very closely related to bug 203454; the fix is in the same algorithm.  Perhaps this code is tricky because it is trying to maintain a complex data structure, without a layer of abstraction?  I think the data structure here is functionally equivalent to a linked list.  Even if the existing array implementation is necessary for speed and/or size, perhaps the code should be refactored to abstract the data structure into a separate class, that would be easier to understand, test, and maintain.
Comment 7 Olivier Thomann CLA 2007-11-15 13:50:23 EST
This is a complement for bug 203454. I missed it when fixing bug 203454.
The steps to get it are not exactly the same.
Comment 8 Olivier Thomann CLA 2007-12-05 15:09:21 EST
*** Bug 207944 has been marked as a duplicate of this bug. ***
Comment 9 Olivier Thomann CLA 2007-12-05 15:11:18 EST
Philippe,

+1 for 3.3.2?
The export test case from bug 207944 could be used to verify it.

I would not backport the regression test from HEAD since it implies many changes in the apt tests.
The fix itself is a one line change.
Comment 10 Philipe Mulet CLA 2007-12-06 05:13:28 EST
+1 for 3.3.2
Comment 11 David Audel CLA 2007-12-11 10:21:33 EST
Verified for 3.4M4.
Comment 12 Olivier Thomann CLA 2007-12-11 10:29:12 EST
The request for 3.3.2 is due to the following reasons:
- NPE is thrown
- fix is trivial and implies no risk
- this can occur as soon as compiling the first compilation unit requests more units through the accept method. If the number of requested units is lower than the unitsToProcess size, the NPE will occur.
- it is not easy for the user to find out what is going on.
Comment 13 Olivier Thomann CLA 2007-12-12 09:45:12 EST
Reopen for 3.3.2 release.
Comment 14 Olivier Thomann CLA 2007-12-12 09:46:51 EST
Released for 3.3.2.
The regression tests is not backported as it implies too many changes for 3.3.2 in the test suite.
The test case in bug 207944 should be used to verify it.
Comment 15 Maxime Daniel CLA 2008-01-24 07:07:57 EST
Verified for 3.3.2 using build M20080123-0800.

Note: the bug reproduction using the test case of bug 207944 seems to need that a fresh workspace be used (at least, I consistently failed to reproduce it when starting with a non-empty workspace and eclipse-SDK-M20070905-1045, while I reproduced it without problem when using the same build on a fresh workspace).
Comment 16 Olivier Thomann CLA 2008-06-09 13:28:21 EDT
*** Bug 236255 has been marked as a duplicate of this bug. ***