[OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D V jayathirth.d.v at oracle.com
Thu Feb 9 07:56:42 UTC 2017 

Hello All,

There was a closed test which was failing because of identity-as-equals approach for ColorModel equals() method.
I have modified it and added in the webrev. Along with this we are now using colorspace.hashCode() in hashCode() functions instead of Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in IndexColorModel equals() method because Arrays.copyOf() takes lot of time.

Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.18/ 

Ran jtreg test and JCK there are no additional test case failures because of the above change. Only 4 JCK tests are failing as it was happening previously.

Just copy pasted my observation regarding JCK failures so that we can trace it easily:

1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test cases: 4; passed: 3; failed: 1; first test case failure: ColorModel2001

	This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In the test case they have bits per component array 	of length 4 like {8, 8, 8, 8}. But in the test case wherever they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check 	that we have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is false.

2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases: 3; passed: 2; failed: 1; first test case failure: ColorModel0004

	Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are using identity-as-equals check in ColorModel.

3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test cases: 2; passed: 1; failed: 1; first test case failure: ColorModel2006

	Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we don't have hashCode check in ColorModel and it 	will be different between 2 Objects.

4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1: Failed. test cases: 2; passed: 1; failed: 1; first test case failure: testCase1

	Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the first JCK test is failing. We omit alpha bit if 	hasAlpha is false but JCK test tries to call getComponentSize() with index 3 which throws ArrayIndexOutOfBoundsException.

Thanks,
Jay
-----Original Message-----
From: Jayathirth D V 
Sent: Wednesday, February 08, 2017 3:41 PM
To: Jim Graham; Philip Race; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Hello All,

I have updated the webrev to include the following changes.

	1) Have identity as equals check in equals() method of ColorModel but elaborate the specification of equals() and hashCode() in ColorModel on what properties to 	     check in subclasses of ColorModel.
	2) Made changes to test case to have single helper method wherever we have same equals/hashCode() check.
	3) Updated IndexColorModel equals() method to use Arrays.equals() for rgb[] data.
	4) Add comment on why we are not using validBits to calculate hashCode() in IndexColorModel hashCode() method.

Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.17/ 

Thanks,
Jay

-----Original Message-----
From: Jim Graham
Sent: Thursday, February 02, 2017 2:51 AM
To: Phil Race; Jayathirth D V; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix. 
I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...

			...jim

On 1/31/17 2:34 PM, Jim Graham wrote:
> For an application to run into this same issue they'd have to expect
> getCompSizes() to return data for components that don't exist.  It's 
> unlikely they would use that data if they really understand the 
> objects.  While that would be odd, I guess I can see someone might be 
> constructing all of their CM's from an array of 4 components 
> regardless of the number of actual components and we'd be happily 
> remembering the useless extra components and returning an array of 4 
> from getCompSizes().  As I said, they shouldn't really be reading and 
> interpreting those extra components for any image processing, but I can imagine that they might do something like create a variant CM by calling the CompSizes() and copying them into a new array to construct a new CM with modifications.  They might just robotically always copy 4 values without really checking how many are valid.  That's a stretch, and their code is weak.  I can conceive of how this might happen, but I really have no idea how likely it is...
>
>             ...jim
>
> On 1/30/17 3:56 PM, Phil Race wrote:
>> Sounds like we should at least try to get the tests updated so they only test what the spec. says.
>> Although it does indicate that there is at least a chance that 
>> application code might also fail due to similar assumptions.
>> Does #1 not fail with the previous iteration of this change too ?
>>
>> -phil.
>>
>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>> Hmmm.  Sounds like the test cases were written based on bugs in the 
>>> implementation.  I'm not sure what the best tactic is here for the 
>>> short term for getting this in, but many of these changes should eventually be considered bugs in the tests.  Is it acceptable to break API tests like this at the last minute even if the tests are at fault?  Phil?
>>>
>>> Notes on specific instances below...
>>>
>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>> Hi Phil,
>>>>
>>>>     1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. 
>>>> test cases: 4; passed: 3; failed: 1; first test case failure: 
>>>> ColorModel2001
>>>>
>>>>     This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In
>>>> the test case they have bits per component array     of length 4 like {8, 8, 8, 8}. But in the test case wherever
>>>> they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check     that we
>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits, 
>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>
>>> This is a bug in the test then, especially if the size of our array matches the return value of getNumComponents.
>>>
>>>>     2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>>> cases: 3; passed: 2; failed: 1; first test case
>>>> failure: ColorModel0004
>>>>
>>>>     Here they check for equality between 2 ColorModel objects 
>>>> having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>>
>>> How do they accomplish this when the CM class is abstract?  Do they 
>>> create a relatively empty subclass and instantiate that?
>>>
>>> The documentation for the equals() method does not document the 
>>> conditions under which it returns true, it uses a vague concept of "equals this ColorModel".  I don't see how they could test anything given that documentation.
>>>
>>>>     3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. 
>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>> failure: ColorModel2006
>>>>
>>>>     Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we
>>>> don't have hashCode check in ColorModel and it     will be different between 2 Objects.
>>>
>>> Same as above, there are no promises documented.
>>>
>>>>     
>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTest
>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>> failed: 1; first test case failure: testCase1
>>>>
>>>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the
>>>> first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK test tries to call getComponentSize()
>>>> with index 3 which throws ArrayIndexOutOfBoundsException.
>>>
>>> Same assessment as #1 above...
>>>
>>> Again, while these are my recommendations about the correctness of 
>>> these tests, the question remains whether we want to introduce a 
>>> change at this point in the release cycle that will essentially invalidate a number of tests that have been working for several releases already.  I'll leave that tactic issue to Phil...
>>>
>>>                 ...jim
>>>
>>

More information about the 2d-dev mailing list