[OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProduction

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Wed Nov 29 13:56:22 UTC 2017


Looks fine, thank you.

On 29/11/2017 04:44, Prahalad Kumar Narayanan wrote:
> Hello Everyone
> 
> First, Thanks to Sergey for his detailed suggestion on test-case approach.
> 
> I've modified the test-case keeping Sergey's suggestion as a reference.
> Few subtle changes are-
>      . Created explicit classes rather than using anonymous class definitions
>      . Modified minimum test duration from 10 seconds to 5 seconds.
> The test-case now reproduces this issue without the proposed fix. (Tested on win7 and Ubuntu VM)
> 
> Besides this, I 've updated Javadoc comments for FilteredImageSource as requested by Joe Darcy in CSR review.
>      . Since every method is synchronized, I decided to update javadoc comments for the class.
>      . The approach is similar to how java.util.HashTable captures the synchronized contract of its methods.
> 
> The javadoc modification is as follows-
> @@ -35,7 +35,8 @@
>   /**
>    * This class is an implementation of the ImageProducer interface which
>    * takes an existing image and a filter object and uses them to produce
> - * image data for a new filtered version of the original image.
> + * image data for a new filtered version of the original image. Furthermore,
> + * {@code FilteredImageSource} is synchronized for thread-safety.
>    * Here is an example which filters an image by swapping the red and
> 
> Kindly review the above changes at your convenience and share your views
>      Webrev link: http://cr.openjdk.java.net/~pnarayanan/8188083/webrev.02/
> 
> Thank you for your time
> Have a good day
> 
> Prahalad
> 
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Tuesday, November 28, 2017 11:14 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Cc: Philip Race
> Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProduction
> 
> The test can use any part of our API. It is a small part of the code which can confirm/verify the fix. It should not be considered as a user's code and it could emulate behavior of classes inside jdk.
> If you do not like to call these methods directly you can do this like this:
> 
> import java.awt.Graphics;
> import java.awt.Image;
> import java.awt.image.ColorModel;
> import java.awt.image.FilteredImageSource;
> import java.awt.image.ImageConsumer;
> import java.awt.image.ImageFilter;
> import java.awt.image.ImageObserver;
> import java.awt.image.ImageProducer;
> import java.util.Hashtable;
> 
> public final class Test {
> 
>       private volatile static Throwable fail;
> 
>       public static void main(final String[] args) throws
>                                                    InterruptedException {
> 
>           final ImageConsumer ic = new ImageConsumer() {
>               @Override
>               public void setDimensions(int i, int i1) {
>               }
> 
>               @Override
>               public void setProperties(Hashtable<?, ?> hashtable) {
>               }
> 
>               @Override
>               public void setColorModel(ColorModel colorModel) {
>               }
> 
>               @Override
>               public void setHints(int i) {
>               }
> 
>               @Override
>               public void setPixels(int i, int i1, int i2, int i3,
>                                     ColorModel colorModel, byte[] bytes,
>                                     int i4,
>                                     int i5) {
>               }
> 
>               @Override
>               public void setPixels(int i, int i1, int i2, int i3,
>                                     ColorModel colorModel, int[] ints,
>                                     int i4,
>                                     int i5) {
>               }
> 
>               @Override
>               public void imageComplete(int i) {
>               }
>           };
>           final ImageProducer ip = new ImageProducer() {
>               @Override
>               public void addConsumer(ImageConsumer imageConsumer) {
>               }
> 
>               @Override
>               public boolean isConsumer(ImageConsumer imageConsumer) {
>                   return false;
>               }
> 
>               @Override
>               public void removeConsumer(ImageConsumer imageConsumer) {
>               }
> 
>               @Override
>               public void startProduction(ImageConsumer imageConsumer) {
>               }
> 
>               @Override
>               public void requestTopDownLeftRightResend(
>                       ImageConsumer imageConsumer) {
>               }
>           };
> 
>           final Image image = new Image() {
>               ImageFilter filter = new ImageFilter();
>               ImageProducer producer =  new FilteredImageSource(ip, filter);
> 
>               @Override
>               public int getWidth(ImageObserver observer) {
>                   return 100;
>               }
> 
>               @Override
>               public int getHeight(ImageObserver observer) {
>                   return 100;
>               }
> 
>               @Override
>               public ImageProducer getSource() {
>                   return producer;
>               }
> 
>               @Override
>               public Graphics getGraphics() {
>                   throw new UnsupportedOperationException();
>               }
> 
>               @Override
>               public Object getProperty(String name, ImageObserver
> observer) {
>                   return null;
>               }
>           };
> 
>           Thread t1 = new Thread(() -> {
>               try {
>                   while (true) {
>                       image.getSource().addConsumer(ic);
>                   }
>               } catch (Throwable t) {
>                   fail = t;
>               }
>           });
>           Thread t2 = new Thread(() -> {
>               try {
>                   while (true) {
>                       image.getSource().removeConsumer(ic);
>                   }
>               } catch (Throwable t) {
>                   fail = t;
>               }
>           });
>           Thread t3 = new Thread(() -> {
>               try {
>                   while (true) {
>                       image.getSource().startProduction(ic);
>                   }
>               } catch (Throwable t) {
>                   fail = t;
>               }
>           });
>           t1.setDaemon(true);
>           t2.setDaemon(true);
>           t3.setDaemon(true);
> 
>           t1.start();
>           t2.start();
>           t3.start();
> 
>           t1.join(10000);
>           if (fail != null) {
>               throw new RuntimeException("Test fail", fail);
>           }
>       }
> }
> 
> 
> On 27/11/2017 23:31, Prahalad Kumar Narayanan wrote:
>> Hello Sergey
>>
>> Thank you for your review response.
>>
>> I had one version of the test that resembled your test code.
>> I uploaded the result (screenshot) of that test case with/ without fix on JBS.
>>
>> The only point of concern - API documentation mentions that the methods of FilteredImageSource are not supposed to be invoked directly from user code. If the methods are invoked directly from user code, the result is un-defined. Since our JTreg test cases cannot violate the documented guide, I refrained from attaching the test case in the first webrev.
>>
>> Kindly let me know your views to this.
>>
>> Thank you once again
>> Have a good day
>>
>> Prahalad
>>
>> -----Original Message-----
>> From: Sergey Bylokhov
>> Sent: Tuesday, November 28, 2017 12:46 PM
>> To: Prahalad Kumar Narayanan; 2d-dev
>> Cc: Philip Race
>> Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in
>> java.awt.image.FilteredImageSource.startProduction
>>
>> On 27/11/2017 22:12, Prahalad Kumar Narayanan wrote:
>>> Yes..! As you rightly pointed, the test passes before the fix as well.
>>
>> Then I suggest to additionally reuse a test which I sent previously,
>> it is fail almost always before the fix. The test which fails one time
>> in
>> 10000 runs will be never fixed, or such bugs will be closed as not reproducible.
>>
>>>
>>> Reason is that, the occurrence of this bug is rare.
>>>        . Submitter has observed only 15 occurrences of the exception with over 10,000 instances per month.
>>>        . Though the occurrence is rare, submitter is expecting a fix for this issue.
>>>
>>> So what does this test-case resolve ?
>>>        . Our change adds "synchronized" contract to the method to fix the Null pointer exception.
>>>        . To test the changes, we should check for both- Null Pointer Exception and any deadlock that could arise from "synchronized" contract.
>>>        . The test case helps to address both but in a passive way. Why do I say passive ?
>>>              . First, the test provides a hope to detect & report NPE though its occurrence is rare.
>>>              . Second, any deadlock arising from the concerned method, will cause the test to fail upon time-out.
>>>     
>>> Thank you once again for your time
>>> Have a good day
>>>
>>> Prahalad
>>>
>>> -----Original Message-----
>>> From: Sergey Bylokhov
>>> Sent: Monday, November 27, 2017 9:11 PM
>>> To: Prahalad Kumar Narayanan; 2d-dev
>>> Cc: Philip Race
>>> Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in
>>> java.awt.image.FilteredImageSource.startProduction
>>>
>>> Hi,Prahalad.
>>> On 24/11/2017 21:40, Prahalad Kumar Narayanan wrote:
>>>> Based on discussions with Sergey, I 've now updated the fix with a test case.
>>>> The changes are available for review under:
>>>> http://cr.openjdk.java.net/~pnarayanan/8188083/webrev.01/
>>>
>>> This version of the test is always passed before the fix(I have checked w/ and w/o jtreg).
>>>
>>> --
>>> Best regards, Sergey.
>>>
>>
>>
> 
> 
> --
> Best regards, Sergey.
> 


-- 
Best regards, Sergey.


More information about the 2d-dev mailing list