Geoff Halliwell wrote On 12/08/06 08:43,:
>[Cross posting for a wider audience...]
>
>
>
> ------------------------------------------------------------------------
>
> Subject:
> FindBugs warnings in glassfish-v2-b26 (was Re: V2 Milestone 3
> Completion Criteria)
> From:
> Bill Pugh <pugh_at_cs.umd.edu>
> Date:
> Fri, 08 Dec 2006 10:00:07 -0500
> To:
> quality_at_glassfish.dev.java.net
>
> To:
> quality_at_glassfish.dev.java.net
>
>
> On Oct 27, 2006, at 5:08 PM, Geoff Halliwell wrote:
>
>> Posting, as I'm not sure everyone has seen the M3 Completion Criteria.
>>
>> http://www.glassfishwiki.org/gfwiki/Wiki.jsp?
>> page=Milestone3ExitCriteria
>>
>> Note: Critical FindBugs issues need to be resolved by 12/11.
>>
>> Geoff
>
>
>
> I haven't really been in the loop, but there are a lot of
> critical FindBugs issues still in glassfish-v2-b26.
>
> FindBugs 1.1.2rc2 generates 84 high priority correctness warnings
> and 367 medium priority correctness warnings (these are just warnings
> on files that occur in the source distribution).
>
> We've done a lot of tuning since FindBugs 1.0, and with the current
> version
> of FindBugs, I've generally found that at least 2/3 of the medium/
> high priority
> correctness warnings translate into things you _really_ want to fix.
>
> I'd be happy to chat or email with anyone interested in addressing
> these issues;
> I've love for Glassfish to be a FindBugs success story and ship
> fairly clean,
> but at the moment it looks fairly nasty.
I've been monitoring the full reports generated by FindBugs against
our nightly builds for webtier related warnings, and neither of the warnings
for
org.apache.jasper.xmlparser.ASCIIReader
or
javax.el.BeanELResolver
described below show up in these reports.
It looks like FindBugs version 1.0.0 (which our nightly runs use)
did not detect them. I agree our exit criteria should be based
on a more recent FindBugs release.
Jan
>
>
> Some samples:
>
> 4 impossible casts:
>
> com.sun.enterprise.admin.monitor.registry.spi.GeneratedMonitoringMBeanIm
> pl
>
> public String[] listAttributes(){
> return (String[]) attributeMap.values().toArray(); // BUG:
> toArray returns an Object[] which
> // can't
> be downcast
> }
>
>
> 3 calls to methods of containers with wrong types.
>
> com.sun.appserv.management.util.jmx.NotificationEmitterSupport:
>
> decrementListenerCountForType( final String type )
> {
> synchronized( mListenerTypeCounts )
> {
> final Integer count = mListenerTypeCounts.get( type );
> if ( count == null )
> {
> throw new IllegalArgumentException( type );
> }
>
> final int oldValue = count.intValue();
> if ( oldValue == 1 )
> {
> mListenerTypeCounts.remove( count ); // BUG: key of
> the Map is a String
> // This
> remove will be a no-op, and count
> // will
> never go to zero/null
> // should be .remove( type)
> }
> else
> {
> mListenerTypeCounts.put( type, new Integer( oldValue
> - 1 ) );
> }
> }
> }
>
>
>
> 1 bad comparison of signed byte:
>
> org.apache.jasper.xmlparser.ASCIIReader:
>
>
> public int read(char ch[], int offset, int length) throws IOException {
> if (length > fBuffer.length) {
> length = fBuffer.length;
> }
> int count = fInputStream.read(fBuffer, 0, length);
> for (int i = 0; i < count; i++) {
> int b0 = fBuffer[i];
> if (b0 > 0x80) { // BUG: b0 is in the range -128 .. 127
> throw new IOException(Localizer.getMessage
> ("jsp.error.xml.invalidASCII",
> Integer.toString(b0)));
> }
> ch[offset + i] = (char)b0;
> }
> return count;
> }
>
>
>
>
> 9 ignored return values:
>
> com.sun.appserv.management.alert.UnitTest
>
> private void unitTestFailed( String propertyName, String expectedValue,
> String realValue )
> {
> System.out.println( "UNIT TEST FAILED FOR ALERTS : " +
> "\nPropertyName -> " + propertyName +
> "\nexpectedValue -> " + expectedValue +
> "\nrealValue -> " + realValue );
> new RuntimeException( "Unit Test Failed For Alerts.." ); //
> BUG, not thrown
> }
>
> 9 dead stores due to switch fall throughs:
>
> com.sun.enterprise.deployment.MethodDescriptor:
>
> // Convert arrays from [[[I form to int[][][] form.
> public static String fixParamClassName(String param)
> {
> if ( param.charAt(0) == '[' ) { // an array
> int dimensions = param.lastIndexOf('[') + 1;
> char code = param.charAt(dimensions);
> String newparam=null;
> switch (code) { // LOTS OF BUGS: no break statements
> case 'B':
> newparam = "byte";
> case 'C':
> newparam = "char";
> case 'D':
> newparam = "double";
> case 'F':
> newparam = "float";
> case 'I':
> newparam = "int";
> case 'J':
> newparam = "long";
> case 'S':
> newparam = "short";
> case 'Z':
> newparam = "boolean";
> case 'L':
> newparam = param.substring(dimensions+1);
> }
> for ( int j=0; j<dimensions; j++ )
> newparam += "[]";
> return newparam;
> }
> ...
>
>
> 11 self assignments of a field in a constructor:
>
> com.sun.appserv.management.j2ee.statistics.BoundedRangeStatisticImpl:
>
> public
> BoundedRangeStatisticImpl(
> final String name,
> final String description,
> final String unit,
> final long startTime,
> final long lastSampleTime,
> final long low,
> final long current,
> final long high,
> final long lowerBound,
> final long upperBound )
> {
> super( name, description, unit, startTime, lastSampleTime,
> low, current, high );
>
> if ( lowerBound > upperBound )
> {
> throw new IllegalArgumentException();
> }
>
> LowerBound = lowerBound;
> UpperBound = UpperBound; // THIS IS A BUG; the parameter is
> upperBound
> }
>
> 3 uncallable method of an anonymous inner class
>
> javax.el.BeanELResolver:
>
> private static final Map<Class, BeanProperties> properties =
> Collections.synchronizedMap(new LinkedHashMap
> (CACHE_INIT_SIZE, 0.75f, true){
> public BeanProperties put(Class key, BeanProperties
> value) { // BUG: should be put(Object, Object)
> SoftReference<BeanProperties> ref = new
> SoftReference(value);
> SoftReference<BeanProperties> prev =
> (SoftReference<BeanProperties>)super.put(key,
> ref);
> if (prev != null) return prev.get();
> return null;
> }
>
> public BeanProperties get(Class key) { // THIS IS A
> BUG: Should be get(Object)
> SoftReference<BeanProperties> ref =
> (SoftReference<BeanProperties>)super.get(key);
> if (ref != null && ref.get() == null) {
> remove(key);
> }
> return ref != null ? ref.get() : null;
> }
>
> protected boolean removeEldestEntry(Map.Entry eldest) {
> return size() > CACHE_MAX_SIZE;
> }
> });
>
> Actually, a better way to fix this is to make the anonymous inner
> class extend
> LinkedHashMap<Class, BeanProperties>, so that the appropriate bridge
> methods
> will be generated.
>
> Plus 190 null pointer warnings.
>
>
> Bill Pugh
>
>
>
>
>------------------------------------------------------------------------
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>
>