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

[Alexey Ivanov]

Hi Pankaj,

I believe this piece of code:
793                 synchronized (this) {
794                     Image img = image;
795                     Dimension d = adjustWidthHeight(
796                             img.getWidth(imageObserver),
797                             img.getHeight(imageObserver));

should look like this:
                 Image img
                 synchronized (this) {
                     img = image;
                 }
                 Dimension d = adjustWidthHeight(
                         img.getWidth(imageObserver),
                         img.getHeight(imageObserver));

Otherwise, a deadlock is possible when you call img.getWidth or 
getHeight as I explained in my previous email.

> I have removed the "headful" tag from the test as it is headless test. I added it by mistake earlier.

I wonder whether the test runs correctly in headless environment. You're 
creating a Swing component. No, you don't show it on the screen, but still…


Regards,
Alexey

On 18/11/2019 11:50, Pankaj Bansal wrote:
> Hi Sergey/Alexey
>
> << You need to read the image to the local var under synchronization and then get W/H from the local image variable. I guess in this case both W/H will be 0 since error is occured?
> Done. As Alexey pointed, the values are set to -1 when the error occurs.
>
> << 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) {
> This will not be needed now as we are using the local variable.
>
> <<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.
> Done. Along with this, I have removed the "headful" tag from the test as it is headless test. I added it by mistake earlier.
>
> I have run a mach5 job and it is green on all platforms.
>
> Webrev: http://cr.openjdk.java.net/~pbansal/8230235/webrev02/
>
> Regards,
> Pankaj
>
> -----Original Message-----
> From: Alexey Ivanov
> Sent: Wednesday, November 13, 2019 1:28 AM
> To: Sergey Bylokhov; Pankaj Bansal; swing-dev
> Subject: Re: <Swing Dev> [14] RFR JDK-8230235 - Rendering HTML with empty img attribute and documentBaseKey cause Exception
>
> On 31/10/2019 19:53, Sergey Bylokhov wrote:
>> On 10/31/19 5:59 am, Pankaj Bansal wrote:
>>> << 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.
>> You need to read the image to the local var under synchronization and
>> then get W/H from the local image variable.
>> I guess in this case both W/H will be 0 since error is occured?
> You're right! This approach seems to be easier to understand.
>
> I think the current code can deadlock: we call image.getWidth() holding the monitor of ImageView.this; ImageObserver (line 958) needs to lock the monitor of ImageView.this to reset 'image' field to null if an error occurred.
>
> If an error occurs, width and height are set to -1.
>
> --
> Regards,
> Alexey