[OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7
Phil Race
philip.race at oracle.com
Tue May 31 20:10:45 UTC 2016
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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>
More information about the 2d-dev
mailing list