On Fri, 11 Jul 2025 18:50:45 GMT, Phil Race <p...@openjdk.org> wrote:
>>> I think we need to have the "whole picture" tested to make sure everything >>> does as we expect. It isn't so easy to tell even from looking at the source >>> of ImageIcon. Then spec'ed so developers know what to expect. When I wrote >>> a little test, I see null args do generate NPEs but invalid args don't >>> result in null images Toolkit images are installed, even if they can't be >>> used because the source isn't valid. They are "effectively" null, but not >>> "actually" null. We should at least specify the null arg behaviours and try >>> to say something about invalid image data. And as a result, I am now >>> flipping to just documenting the setImage(null) NPE The protected method >>> loadImage probably needs to say it too. Invalid image data, maybe we can >>> cover that in a class level statement. Note: I'd like to see every >>> constructor tested with both null and invalid image data. >> >> I tested with null image/imagedata and invalid imagedata and it seems only >> these constructors throw NPE (current JDK) for null image (invalid image >> doesnt throw any exception) >> >> ImageIcon(image, desc) >> ImageIcon(image) >> ImageIcon(byte[], desc) >> ImageIcon(byte[]) >> >> Current PR checks for null in ImageIcon(image) so it will not throw NPE but >> ImageIcon(byte[]) will still throw.. >> Should we handle those in separate PR and let this only be for >> setImage(null)? > >> > I think we need to have the "whole picture" tested to make sure everything >> > does as we expect. It isn't so easy to tell even from looking at the >> > source of ImageIcon. Then spec'ed so developers know what to expect. When >> > I wrote a little test, I see null args do generate NPEs but invalid args >> > don't result in null images Toolkit images are installed, even if they >> > can't be used because the source isn't valid. They are "effectively" null, >> > but not "actually" null. We should at least specify the null arg >> > behaviours and try to say something about invalid image data. And as a >> > result, I am now flipping to just documenting the setImage(null) NPE The >> > protected method loadImage probably needs to say it too. Invalid image >> > data, maybe we can cover that in a class level statement. Note: I'd like >> > to see every constructor tested with both null and invalid image data. >> >> I tested with null image/imagedata and invalid imagedata and it seems only >> these constructors throw NPE (current JDK) for null image (invalid image >> doesnt throw any exception) >> >> ImageIcon(image, desc) ImageIcon(image) ImageIcon(byte[], desc) >> ImageIcon(byte[]) >> >> Current PR checks for null in ImageIcon(image) so it will not throw NPE but >> ImageIcon(byte[]) will still throw.. Should we handle those in separate PR >> and let this only be for setImage(null)? > > You told me (off-line) that null URL also throws NPE and I see that too. > > This class is a bit of a mess of accidental behaviours and scant > documentation. > > Let's document all the NPE behaviours and include a test that verifies it. > > And invalid data like "new byte[0]" or null for a file name, or pointing to > something that isn't an image file, or an invalid URL .. etc .. results in an > Image instance being present, but it will never be completed so can never be > drawn. > > I think we need a class level statement like > "If the image source parameter to a constructor is non-null, but does not > reference valid accessible image data, no exceptions will be thrown but the > image will be 'effectively' null, as it will have no dimensions and never be > drawn, and > getImageLoadStatus() will report ERRORED" - you should verify that last part > but I think it is right. > > We probably should add the similar text on setImage(). > > We can also test the invalid image data cases too. @prrace The scope of this fix has expanded far beyound `ImageIcon.setImage` and its handling of `null` parameter. Should the JBS title as well as the CSR be updated with a more relevant subject? ------------- PR Comment: https://git.openjdk.org/jdk/pull/25767#issuecomment-3128601184
