[OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception

[Jim Graham]

Hi Ajit,


On 3/30/2016 5:12 AM, Ajit Ghaisas wrote:
> Raster.java, line 986: Something to file as a potential bug for future work since the fix would have to be verified that it doesn't disrupt the other parts of this method, but... The set of if statements in this method never checked for a BandedSampleModel to try to create a BandedRaster as in createBandedRaster.  On the other hand, no developer has complained so far (that I know of, but maybe we have an old bug filed on it that I'm not aware of).
>
>>>> [Ajit] :  Without my fix, it was easier to get RasterFormatException as explained in Bug Description. This fix enhances the code by making type specific Raster class constructors type-safe.
> Now, the bigger question is: Whether methods in Raster.java support all possible combinations of Raster creations?
> What I can say is - this fix does not break anything than what is already present in Raster.java.
> If there is specific user complaint or any existing bug - we can look into it separately.

I agree that this creates no new failure modes, which is why I suggested 
it as something to file for future work. (an enhancement)

> The following 2 comments are identical for all of the raster types {
>       ByteBandedRaster.java, line 76: This code will throw a ClassCastException
>           now instead of the RasterFormatException that used to be thrown.
>           Do we care?  It would be easy enough to add a test to just this
>           method (and the similar versions in other files).
>>>> [Ajit] :  Since we have decided to make the type specific Raster class constructors type safe, yes, it is possible to get ClassCastException. But, as the usage of these classes is guarded now with "instanceof" checks, it would be rare.

In other cases, where the caller supplies the data buffer, we have 
protected them all with instance checks, but in this case the method is 
relying on the sample model to create the correct type of data buffer.

If this method were called from a random place then we'd have no 
guarantees, but as far as I can see, it is only called from within 
ByteBandedRaster itself and it is called on the same sample model that 
it was originally created with.

The typical case will probably not be an issue, but it's not immediately 
clear if there is any way that someone can end up with a custom sample 
model in a ByteBandedRaster that produces some other kind of data 
buffer?  Possibly this would be more likely if there were any other 
callers to that method, but a search of java.desktop didn't turn up any.

It would be difficult at best to engineer a test case that would 
discover the difference here and that test case would have previously 
thrown an exception anyway so I suppose I'm not going to be concerned if 
the type of the exception has changed here...

>       ByteBandedRaster.java, line 668: Extra credit: This cast could be avoided
>           if we make SunWritableRaster employ generics for a strongly typed DataBuffer.
>
>>>> [Ajit] : the casted member ' dataBuffer' on this line is a member of Raster class.
> The class hierarchy of ByteBandedRaster is ( added on single line for convenience)
>   --------   ByteBandedRaster extends SunWritableRaster extends WritableRaster extends Raster.
>
> I am not quite clear on how we can avoid cast on this line. Can you please elaborate?
>
> }

The object being cast here is the contents of the dataBuffer field of 
the raster, which is known to be a DataBufferByte for the case of 
ByteBandedRaster because all of its constructors enforce that type, 
especially so now that the constructors are all type-specific.  If all 
of the super-classes were generified for the kind of DataBuffer that 
they held then we wouldn't need a cast here.

But, that would be a public API change since the field is stored in the 
public java.awt.image.Raster class, so the generification would have to 
extend all the way up to that class.

Something to consider for the future, but definitely outside the scope 
of this fix.  And if we do that, then we should also consider other use 
of generics within all of the java.awt.image classes and that would be a 
much larger effort.

> Here is the updated webrev:
> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.05/

If you only changed those things you mentioned above then it looks good 
to go. If there is something else that was changed, let me know and I'll 
do a more thorough review...

			...jim