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

[Alexey Ivanov]

On 18/11/2019 12:31, Pankaj Bansal wrote:
> Hi Alexey,
>
> <<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.
>
> << 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 think this should not cause an issue even if the error occurred as this all is happening on same thread. As far as I understand, synchronized block use "reentrant locks", which mean same thread can acquire a lock which it already owns. So if a function func() is called from a synchronized block and that func() also has a synchronized block on same object, that should work fine as long as it is called on same thread.

This is correct!

> In fact in the present test case, the error occurs and code goes to reset the "image" to null. This is working fine as all this is happening on same thread. Please correct me if I am missing something here.

That's exactly the problem: the code which resets image field to null 
could be called from another thread.

I guess the image loader thread already detected an error that's why it 
works. Yet if it takes a while to load the image, for example slow 
network connection, the call img.getWidth will block. Then 
ImageObserver, i.e. ImageHandler.imageUpdate(), will be called from 
another thread to set the width or to reset image field to null if an 
error occurs. In this case, it would deadlock waiting to enter the 
synchronized section, right?

> << 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…
> Yes, this works fine as long as we don’t try to display something on screen. I have done similar things in past without any issue. Also, I have a mach5 job on all platforms with this test. It is green.

Perfect! Thank you for confirming it.

Regards,
Alexey

>
>
> Regards,
> Pankaj Bansal
>
> -----Original Message-----
> From: Alexey Ivanov
> Sent: Monday, November 18, 2019 5:38 PM
> To: Pankaj Bansal; Sergey Bylokhov; swing-dev
> Subject: Re: <Swing Dev> [14] RFR JDK-8230235 - Rendering HTML with empty img attribute and documentBaseKey cause Exception
>
> 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
>>
>> <SNIP>