<Swing Dev> [14] RFR JDK-8230235 - Rendering HTML with empty img attribute and documentBaseKey cause Exception

[Pankaj Bansal]

Hi Sergey/Alexey

Webrev: http://cr.openjdk.java.net/~pbansal/8230235/webrev01/

<< I guess that the problem is that the "image" is accessed w/o synchronization on "this" like in other places, but it will be necessary to add one null check before usage of "image".
I have updated the code to access image with synchronization. But that does not solve the issue. So I have kept the null check also. I tried to add the null check before calling getWidth, calling getWidth still makes the image null. So the check before calling getHeight is still required.

<<This requires a comment, doesn't it? Otherwise the condition in if looks always true. Is it possible that image becomes null before getWidth() is called?
I have added the comment. I don't see a case where image is null before calling getWidth, so did not add a null check there.

<< I agree with Sergey, try-catch are not necessary in this case: you declared that main throws Exception. Any exception indicates the test failed.
<<Since JLabel is a Swing component, you should create its instance on EDT. SwingUtilities.invokeAndWait would do the job.
Done

Regards,
Pankaj

-----Original Message-----
From: Alexey Ivanov 
Sent: Wednesday, October 16, 2019 7:27 PM
To: Pankaj Bansal; swing-dev
Cc: Sergey Bylokhov
Subject: Re: <Swing Dev> [14] RFR JDK-8230235 - Rendering HTML with empty img attribute and documentBaseKey cause Exception

Hi Pankaj,

I'd like to confirm:

793                 int width = image.getWidth(imageObserver);
794                 if (image != null) {

the image is not null at line 793, and as the side effect of calling image.getWidth(imageObserver), image is set to null, right?
This requires a comment, doesn't it? Otherwise the condition in if looks always true. Is it possible that image becomes null before getWidth() is called?

Can we implement this differently?
I guess we'll also get NPE if src attribute of <image> tag is not an image.


I agree with Sergey, try-catch are not necessary in this case: you declared that main throws Exception. Any exception indicates the test failed.

Since JLabel is a Swing component, you should create its instance on EDT. SwingUtilities.invokeAndWait would do the job.

Regards,
Alexey

On 10/10/2019 21:35, Sergey Bylokhov wrote:
> Hi, Pankaj.
> I guess that the problem is that the "image" is accessed w/o 
> synchronization on "this"
> like in other places, but it will be necessary to add one null check 
> before usage of "image".
> Also, it is not necessary to have a try/catch block in the test, just 
> call the problematic method
>
> On 10/10/19 11:01 am, Pankaj Bansal wrote:
>> Hi All,
>>
>> Please review the following fix for jdk14.
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8230235
>>
>> Webrev: http://cr.openjdk.java.net/~pbansal/8230235/webrev00/
>>
>> Issue:
>>
>> Rendering HTML with empty img attribute and documentBaseKey set 
>> causes NPE
>>
>> Fix:
>>
>> The issues is introduced after
>> https://bugs.openjdk.java.net/browse/JDK-8218674.
>>
>> When the documentBaseKey is set, but img tag is not set, the image 
>> URL may not be complete and may not point to actual image. In 
>> ImageView class, when getWidth(ImageObserver) is called from the 
>> updateImageSize, it checks if image source is null and adds the 
>> ImageObserver (ImageView.ImageHandler) on the Image and sets the 
>> ImageView image as null. When getHeight is called, as image is set to 
>> null, it results in NPE.
>>
>> We should verify that image is not null.
>>
>> Testing:
>>
>> I have tested it on Window, Linux and Mac.
>>
>>
>> Regards,
>> Pankaj Bansal
>>
>
>