Hi Pankaj,
On 31/10/2019 12:59, Pankaj Bansal wrote:
Hi Sergey/Alexey
Webrev: http://cr.openjdk.java.net/~pbansal/8230235/webrev01/
<< 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.
<<This requires a comment, doesn't it? Otherwise the condition in if looks
always true. Is it possible that image becomes null before getWidth() is called?
I have added the comment. I don't see a case where image is null before calling
getWidth, so did not add a null check there.
Yes, image is not null before getWidth() call.
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) {
<< I agree with Sergey, try-catch are not necessary in this case: you declared
that main throws Exception. Any exception indicates the test failed.
<<Since JLabel is a Swing component, you should create its instance on EDT.
SwingUtilities.invokeAndWait would do the job.
Done
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.
Regards,
Alexey
Regards,
Pankaj
-----Original Message-----
From: Alexey Ivanov
Sent: Wednesday, October 16, 2019 7:27 PM
To: Pankaj Bansal; swing-dev
Cc: Sergey Bylokhov
Subject: Re: <Swing Dev> [14] RFR JDK-8230235 - Rendering HTML with empty img
attribute and documentBaseKey cause Exception
Hi Pankaj,
I'd like to confirm:
793 int width = image.getWidth(imageObserver);
794 if (image != null) {
the image is not null at line 793, and as the side effect of calling
image.getWidth(imageObserver), image is set to null, right?
This requires a comment, doesn't it? Otherwise the condition in if looks always
true. Is it possible that image becomes null before getWidth() is called?
Can we implement this differently?
I guess we'll also get NPE if src attribute of <image> tag is not an image.
I agree with Sergey, try-catch are not necessary in this case: you declared
that main throws Exception. Any exception indicates the test failed.
Since JLabel is a Swing component, you should create its instance on EDT.
SwingUtilities.invokeAndWait would do the job.
Regards,
Alexey
On 10/10/2019 21:35, Sergey Bylokhov wrote:
Hi, Pankaj.
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".
Also, it is not necessary to have a try/catch block in the test, just
call the problematic method
On 10/10/19 11:01 am, Pankaj Bansal wrote:
Hi All,
Please review the following fix for jdk14.
Bug: https://bugs.openjdk.java.net/browse/JDK-8230235
Webrev: http://cr.openjdk.java.net/~pbansal/8230235/webrev00/
Issue:
Rendering HTML with empty img attribute and documentBaseKey set
causes NPE
Fix:
The issues is introduced after
https://bugs.openjdk.java.net/browse/JDK-8218674.
When the documentBaseKey is set, but img tag is not set, the image
URL may not be complete and may not point to actual image. In
ImageView class, when getWidth(ImageObserver) is called from the
updateImageSize, it checks if image source is null and adds the
ImageObserver (ImageView.ImageHandler) on the Image and sets the
ImageView image as null. When getHeight is called, as image is set to
null, it results in NPE.
We should verify that image is not null.
Testing:
I have tested it on Window, Linux and Mac.
Regards,
Pankaj Bansal
