[OpenJDK 2D-Dev] [9] RFR: JDK-8140530, , Creating a VolatileImage with size 0, 0 results in no longer working g2d.drawStri

[Jim Graham]

+1

		...jim

On 12/3/15 10:17 PM, prasanta sadhukhan wrote:
> ok. Thanks Jim .
> Please review the modified webrev
> http://cr.openjdk.java.net/~psadhukhan/8140530/webrev.02/
>
> Regards
> Prasanta
> On 12/4/2015 6:45 AM, Jim Graham wrote:
>> I think it makes sense to catch it at a higher level, but also to
>> throw some type of exception from the X11 code as you do now because
>> regardless of our higher level policy, the X11 implementation function
>> can never succeed there...
>>
>> So, my preference would be to keep the existing pieces of this fix
>> that you already have and to add an explicit check and throw IAE to
>> SunVolatileImage for completeness and guaranteed consistency...
>>
>>             ...jim
>>
>> On 12/2/15 11:18 PM, prasanta sadhukhan wrote:
>>> Hi Jim,
>>>
>>> On 12/3/2015 12:38 AM, Jim Graham wrote:
>>>> Hi Prasanta,
>>>>
>>>> (On a practical note, in the HTML version of your message, the text
>>>> said "webrev.01", but the link in the href pointed to "webrev.00" so I
>>>> sat there wondering why the changes you noted weren't there until I
>>>> realized that I was still looking at webrev.00 and had to manually
>>>> enter webrev.01 in the browser to see the new code...)
>>>>
>>>> Have you run your new test on all platforms to make sure that it
>>>> succeeds (by throwing IAE) on all currently supported/tested platorms?
>>>>
>>> IAE was already been thrown for windows and mac. It was not working for
>>> linux only.
>>>> It seems, from the comment, that one issue is that X11 has a special
>>>> need in that if we make it through to the native code with 0,0
>>>> arguments and attempt to create a pixmap of 0,0 then we get an X11
>>>> error so I'm OK with the native code having its own check for
>>>> protection against the X11 error. But, for consistency, shouldn't the
>>>> 0,0 be detected and and IAE thrown at a much higher level shared by
>>>> all platforms so that no platform can accidentally allow 0,0?
>>>> Otherwise we have to make sure that each and every current platform
>>>> and each and every future platform port contains these checks to
>>>> satisfy the new behavior expectation.
>>>>
>>>> Apparently, somewhere above the native method there is a check that
>>>> converts OOME to IAE - is that in shared code or in the X11-specific
>>>> code?
>>> It is not a direct conversion from OOME to IAE. Basically, the Java will
>>> try to create a accelerated surface and if it cannot, it creates a
>>> backup BufferedImage via createCompatibleImage() which calls
>>> createCompatibleWritableRaster() and that function has a check for 0
>>> width,height which throws IAE.
>>>
>>> Code wise flow:
>>> VolatileSurfaceManager.initialize() will check for accelerated surface
>>> via initAcceleratedSurface() which goes to different pipeline for
>>> different platforms.
>>>
>>> In windows, D3DVolatileSurfaceManager.java#initAcceleratedSurface() will
>>> call D3DSurfaceData.createData() which calls initSurface() which
>>> initializes surface by calling "native" initSurfaceNow() which returns
>>> false for 0 width.height. D3DSurfaceData throws InvalidPipeException to
>>> D3DVolatileSurfaceManager#initAcceleratedSurface()  which marks
>>> accelerated surface as null in that case so Java will fall back to
>>> BufferedImage as explained above.
>>>
>>> In mac, CGLVolatileSurfaceManager.jaav#initAcceleratedSurface() will get
>>> OOM from CGlSurfaceData.createData() which calls native initFBObject()
>>> which returns false
>>>
>>> In linux, native was not throwing any issue even though it does not
>>> utilise 0 width,height so Java still assumes it is working with
>>> accelerated surface so will not try to create backup BufferedImage
>>> surface so cannot throw IAE.
>>> My fix will let native throw OOM to
>>> XRVolatileSurfaceManager.java#initAcceleratedSurface() so that it knows
>>> it fails to create accelerated surface and has to fall back to
>>> BufferedImage surface and then the IAE will be thrown from
>>> createCompatibleWritableRaster()
>>>
>>>
>>> If you think it's ok, we can catch 0 width,height in SunVolatileImage
>>> constructor before it calls VolatileSurfacemaanger. initialize() so that
>>> it gets trapped  at higher level? Please advise.
>>>
>>> Regards
>>> Prasanta
>>>
>>>>
>>>>             ...jim
>>>>
>>>> On 11/30/15 9:58 PM, prasanta sadhukhan wrote:
>>>>> Modified the testcase to "fail" if IAE is not thrown
>>>>> http://cr.openjdk.java.net/~psadhukhan/8140530/webrev.01
>>>>>
>>>>> Regards
>>>>> Prasanta
>>>>> On 11/30/2015 2:13 PM, prasanta sadhukhan wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Please review a fix for jdk9
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8140530
>>>>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8140530/webrev.00/
>>>>>>
>>>>>> The issue was creating a volatileImage with 0 width, height does not
>>>>>> result in IllegalArgumentException.
>>>>>> But, when we try to create a non-volatile Image via
>>>>>> GraphicsConfiguration.createCompatibleImage(0,0) or a
>>>>>> BufferedImage(0,0,imagetype) it results in IAE.
>>>>>> So, to maintain consistency across all image w.r.t 0 width,height,
>>>>>> createVolatileImage() should also throw IAE.
>>>>>>
>>>>>> In windows, creating a volatileImage with 0 width,height resulted in
>>>>>> IAE but in linux it does not.
>>>>>>
>>>>>> This is because XCreatePixmap() generate BadValue unless width,height
>>>>>> is nonzero but the error handler does not catch it.
>>>>>> https://tronche.com/gui/x/xlib/pixmap-and-cursor/XCreatePixmap.html
>>>>>> [The width and height arguments must be nonzero, or a *BadValue*
>>>>>> error
>>>>>> results.]
>>>>>>
>>>>>> I have added a check to prevent zero width,height to be used for
>>>>>> XCreatePixmap() and also throw OOME so to ask Java to throw IAE.
>>>>>>
>>>>>> Regards
>>>>>> Prasanta
>>>>>
>>>
>