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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>
>
