On 12/4/15 1:00 AM, Sergey Bylokhov wrote:
Looks fine to me to.
I guess that the native check still necessary in case we create a native
surface as a cache for a BufferedImage.

Or in case we want to relax the general API restriction later, it keeps us from ending up with a hard-to-diagnose bug on the X11 pipeline. We'll find out right away that we need an alternate solution for X11...

                        ...jim

On 04.12.15 12:39, Jim Graham wrote:
+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





Reply via email to