Hi Alexey, << 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 agree, it is possible. Also, your suggested code has less code in synchronized block, which is better anyway. I have updated the code. webrev: http://cr.openjdk.java.net/~pbansal/8230235/webrev03/ Regards, Pankaj -----Original Message----- From: Alexey Ivanov Sent: Monday, November 18, 2019 6:22 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 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>
