RFR: 8319925: CSS.BackgroundImage incorrectly uses double-checked locking

[Alexey Ivanov]

On Mon, 4 Dec 2023 06:30:01 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

> > That code does not look like double-checked lock, it is something different. It checks/init/sets one field and then returns another one. Even if both will be marked as volatile the method may return null, since the loadedImage is set to true before init of image.
> 
> I think the field `loadedImage` still needs to be volatile as it is accessed multiple time even though it can be argued to be non-DCL in classical way

Yep.

It is a DCL, essentially, but it uses a different field to protect access to another field whereas the classic DCL uses the same field that it protects.

> Even if both will be marked as volatile the method may return null, since the loadedImage is set to true before init of image.

This is correct. *Good catch!* I missed it.

*For correctness*, the assignment

https://github.com/openjdk/jdk/blob/96d69bcf8cbf1ab20b1577196120c60e072bbe78/src/java.desktop/share/classes/javax/swing/text/html/CSS.java#L2955

has to be moved below the if-statement:


                    if (!loadedImage) {
                        URL url = CSS.getURL(base, svalue);
                        if (url != null) {
                            image = new ImageIcon();
                            Image tmpImg = Toolkit.getDefaultToolkit().createImage(url);
                            if (tmpImg != null) {
                                image.setImage(tmpImg);
                            }
                        }
                        loadedImage = true;
                    }

Otherwise, the returned `image` may still be in an inconsistent state. This way, `loadedImage` is assigned `true` after `image` is initialised; that way if another thread reads `loadedImage` and its value is `true`, it can safely access `image`.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16917#issuecomment-1838728030