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