Looks good!
...jim
On 6/2/16 2:51 AM, Ajit Ghaisas wrote:
Thanks Jim and Phil for reviewing this.
Adding a comment is a good suggestion. I have added it and requested a push of
following webrev-
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.04/
(Sent here for a reference)
Regards,
Ajit
-----Original Message-----
From: Jim Graham
Sent: Thursday, June 02, 2016 2:54 AM
To: Ajit Ghaisas; Philip Race; Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return
blank images in Java 8(.45) while working in 7
+1.
If you wanted to add a comment on the new catch, you could say:
} catch (RuntimeException e) {
// We did not previously call this method here and some filters
// were not prepared for it to be called at this time so we
// allow them to have runtime issues for this one call only
// without triggering the ERROR condition below.
... (printstack)
}
Your call, I don't need to approve a webrev for that comment addition...
...jim
On 6/1/16 3:28 AM, Ajit Ghaisas wrote:
Thanks Jim and Phil for valuable feedback and suggestions.
The detailed discussion helped me to understand why catching RuntimeException
is a better option for a call to imageComplete(STATICIMAGEDONE).
I have modified the code on line 192 accordingly.
I have not modified the code catching NullPointerException (on line 196) as
doing so would mask a wider variety of legitimate exceptions. So, line 196
remains unchanged.
I have also modified the test to throw NullPointerException instead of calling
intValue() on a null.
Please review updated webrev :
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.03/
Regards,
Ajit
-----Original Message-----
From: Phil Race
Sent: Wednesday, June 01, 2016 1:41 AM
To: Jim Graham; Ajit Ghaisas
Cc: Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
ImageFilters return blank images in Java 8(.45) while working in 7
I agree with the suggestion of RuntimeException as likely sufficient but not
too much ...
-phil.
On 5/31/2016 12:29 PM, Jim Graham wrote:
I agree with this. It would be nice if we didn't spit out print
statements by default, but I'm not sure what a good flag would be to
use to diagnose this that balances how likely developers will think
to use it.
The point about catching a wider variety of exceptions is not that it
is good practice in general, but we've added a new call to a
long-standing algorithm which has decades (really? plural? pretty
much, I think) of past code that isn't expecting the new call, and
that call is basically "informative" not something that they really
need to successfully handle, so just as someone might accidentally
process a null pointer in a case like that, someone else might run
off the end of an array (AIOOB) or any other "I wasn't expecting that"
sort of issue.
Throwable might be a bit much, though. Phil? I was thinking
Exception because those tend to focus on "your code made a mistake"
whereas Error is something like "you are mix/matching incompatible
code that was compiled to contain conflicting information" (kind of
like calling a method that doesn't exist in this release, etc.).
It's hard to say what the proper level of forgiveness should be here.
Another thought is "RuntimeException", since any other exception
would need to be declared to be thrown and we don't declare it for
that method, so their implementation shouldn't be allowed to declare
it either...
...jim
On 05/31/2016 10:34 AM, Phil Race wrote:
Based on what Ajit wrote in the bug report, he at least found a jar
file that contains this code, but I have so far failed to locate the
source code as all the versions I find on the net use the Java 2D
JDK 1.2 BufferedImageOp APIs so I suspect this bug is in a very old
version and it may not be open source.
Therefore I am not sure how much e.printStackTrace() will help them
if they don't own that source except to encourage them to upgrade.
I'd be inclined to stick it behind a debugging property if we have
one that we could re-use except that in such a case they probably
won't know enough to set the property.
So on balance it is probably best to do as it has been presented
here except that catch (Throwable t) probably makes sense as we
don't want to revisit this for a different exception.
Minor nit about the test: instead of calling intValue() on a null
Integer, why not just "throw new NullPointerException("reason ..") ?
-phil.
On 05/30/2016 01:22 AM, Ajit Ghaisas wrote:
I did contemplate catching 'Exception' instead of
'NullPointerException' while raising the webrev, but decided not to
do it as- 1. Complaint in current image filter was due to NPE only
2. Catching & consuming generic exception does not seem quite
correct here as we are just printing stack trace and not taking any
concrete action.
3. There is no reported issue of any other type of Exception out of
this method.
Regards,
Ajit
-----Original Message-----
From: Jim Graham
Sent: Saturday, May 28, 2016 4:41 AM
To: Ajit Ghaisas; Sergey Bylokhov; 2d-dev; Philip Race
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
ImageFilters return blank images in Java 8(.45) while working in 7
That looks good, but I wonder if it should catch all exceptions
instead of just NPE. Historically we were only catching NPE before
we added the call to STATICIMAGEDONE, and the current filter in
question turns out to throw an NPE, but if a filter that was
commonly used with offscreen images was not expecting the
STATICIMAGEDONE as was this WaterFilter, then they could
potentially throw a wider variety of exceptions.
I'm good with NPE since that is the only case we've seen, but I'd
also be good with changing line 192 to catch all exceptions. Phil?
...jim
On 5/27/16 1:32 AM, Ajit Ghaisas wrote:
Hi Jim,
Thanks for in-depth analysis and detailed review.
I appreciate your concern for filtered image being blank in
Java 8, while it used to work in Java 7.
Yes. I totally agree with your suggestion to do both -
1. Not send IMAGEERROR if there is exception from ImageFilter
while processing STATICIMAGE done
2. At the same time, as we are consuming the exception, show
the exception stacktrace.
With this fix the functionality of
com.jhlabs.image.WaterFilter is restored. It does not return blank images
anymore.
Also, as additional information, exception details are shown
as a stacktrace.
Please review the updated webrev :
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.02/
Regards,
Ajit
-----Original Message-----
From: Jim Graham
Sent: Thursday, May 26, 2016 4:57 AM
To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev; Philip Race
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
ImageFilters return blank images in Java 8(.45) while working in 7
Their complaint is that the resulting image is empty - most likely
because the error gets passed through and clears the image buffer.
It appears that the sequence of events is:
We send SINGLEFRAMEDONE.
- the image is displayable
We send STATICIMAGEDONE.
- filter throws NPE
- image is probably still displayable We send ERROR
- if it gets passed through to the toolkit image consumer then
we'll clear the image buffer
- image is no longer displayable
It's hard to say for sure without having an instance of their
filter in-house for testing, but previously we weren't sending the
STATICDONE notice and the program was working just fine. Since the
NPE comes from their code when we send it, it shouldn't have
affected any down-stream consumers, so we should be OK with just
continuing on and the image should still be displayable just as if
we hadn't sent the STATICDONE notice in the first place.
Now, *during debugging*, they were thwarted by the fact that we
ate the exception, true, but the original issue is that the image
wasn't displayable at all. We might want to do both:
- not send ERROR for STATICDONE when we used to not send
STATICDONE at all
- show the exception so that someone realizes that there is a
problem, even though we've made it not hurt their functionality.
Does that make sense?
...jim
On 5/25/2016 1:36 PM, Sergey Bylokhov wrote:
On 25.05.16 23:33, Jim Graham wrote:
How about option 3 -
NPE before imageComplete sends an ERROR as it does now.
NPE during imageComplete is ignored, both for backwards
compatibility and because it is more of a "hint" than an
operation that requires action from the consumer.
I guess the case is that when we ignore this NPE(w/o any
notification) the users complains that the bug is in jdk.
...jim
On 5/25/2016 1:31 AM, Ajit Ghaisas wrote:
Hi
Bug :
https://bugs.openjdk.java.net/browse/JDK-8139192
Custom ImageFilters return blank images in Java 8(.45) while
working in 7
Root cause :
private method produce() in OffScreenImageSource.java consumes
a NullPointerException that originates from a custom
ImageConsumer (a 3^rd party image library class -
com.jhlabs.image.WaterFilter)
Analysis:
1. How the behavior changed between JDK7 and JK8 :
A call to imageComplete(ImageConsumer.SINGLEFRAMEDONE) was
added in addition to imageComplete(ImageConsumer.
STATICIMAGEDONE) as a fix for JDK-7143612.
2. What debugging revealed:
A NullPointerException is being thrown from the library during
the call to imageComplete(ImageConsumer.STATICIMAGEDONE)
3. It looks like the fix of JDK-7143612 is valid and successive
calls to
imageComplete() are allowed.
4. The 3rd party library behavior appears incorrect (if it can
not handle subsequent calls to imageComplete(), it should
de-register itself).
The 3rd-party library code should be fixed.
Possible modifications in JDK :
Currently, OffScreenImageSource::produce() consumes the
NullPointerException - this is incorrect and results in silent
failure of this particular image filter.
We need to let the image filter library know that exception has
occurred in its code and not in JDK. We have two options -
Option 1 : Rethrow the NullPointerException --- It is the
clearest way
to let 3^rd party library know that their code is erroneous
with latest JDK. This will change the 3^rd party image filter
behavior that generates blank image.
Option 2 : Add a stack trace where the exception is being
consumed
--- Adding stack trace provides more information regarding
failure of 3^rd party image filter with retaining the current
behavior that generates blank image.
I have implemented both the options:
Option 1:
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.00/
Option 2:
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.01/
Request you to review both the webrevs and provide your preference.
I will add a test to the selected webrev.
Regards,
Ajit
