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

[Alexey Ivanov]

Hi Pankaj,

On 31/10/2019 12:59, Pankaj Bansal wrote:
> 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.

Yes, image is not null before getWidth() call.

I suggest clarifying the reason why image may become null:
794                     int width = image.getWidth(imageObserver);
795                     // Calling image.getWidth could reset image to 
null if an error during loading occurred
796                     if (image != null) {

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

Thanks!
The <img> tag should not be closed as in XML. I mean
<img src=''/>
should be
<img src=''>

Backlash for closing tag is not standard HTML.

Other than that, the test looks good.


Regards,
Alexey

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