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