[OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7
Jim Graham
james.graham at oracle.com
Wed Jun 1 21:23:50 UTC 2016
+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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>
More information about the 2d-dev
mailing list