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>
