RFR: 6176679: Application freezes when copying an animated gif image to the system clipboard

Wed Apr 12 20:16:46 UTC 2023

On Mon, 10 Apr 2023 21:32:30 GMT, Rajat Mahajan <rmahajan at openjdk.org> wrote:

> **Problem:**
> 
> On pressing the Copy button we keep waiting in AWT-EventQueue thread in reconstruct() function as we detect that we have missing information for the animated image since we are copying single frame at a time.
> Due to this infinite wait the application freezes.
> 
> **Proposed Fix:**
> 
> There are conditions in the reconstruct() function that avoid entering the wait() code if we have some error reading the image , etc. So, I added the condition to avoid entering the wait() code if we are copying single frame at a time. This sounded logical to me since if we have incomplete information(single frame) about the animated image we shouldn’t keep waiting, as it leads to infinite wait. 
> After this change I see the GIF image being correctly copied and animated.
> 
> 
> **Testing:**
> 
> Added a test for this (bug6176679.java) and tested locally on my Windows machine and on mach5.

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/sun/awt/image/ImageRepresentation.java line 1:

> 1: /*

Please update the copyright year to 2023.

src/java.desktop/share/classes/sun/awt/image/ImageRepresentation.java line 112:

> 110:         }
> 111:         int missinginfo = flags & ~availinfo;
> 112:         if ((availinfo & (ImageObserver.ERROR |  ImageObserver.FRAMEBITS) ) == 0 && missinginfo != 0) {

Suggestion:

        if ((availinfo & (ImageObserver.ERROR | ImageObserver.FRAMEBITS)) == 0
            && missinginfo != 0) {

The space between the closing parentheses is redundant as well as the extra space after `|` operator. The line could be wrapped to fit into 80-column limit.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 1:

> 1: /*

I think the test should not be in this directory: it does not test the GIF Image IO plugin. A better location is under `java/awt/Clipboard`.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 28:

> 26:  * @key headful
> 27:  * @bug 6176679
> 28:  * @summary Tests that an application doesn't freeze when copying an animated gif image to the system clipboard

Please wrap the line to fit into 80-column limit.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 33:

> 31: 
> 32: import java.awt.*;
> 33: import java.awt.datatransfer.*;

Please expand the wildcard imports.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 40:

> 38: 
> 39: 
> 40: public class bug6176679 implements ClipboardOwner, FlavorListener {

Are `ClipboardOwner` and `FlavorListener` really used? If not, they can be dropped.

The class name should have a descriptive name, like `CopyAnimatedGIFTest`.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 47:

> 45:     private static final CountDownLatch latch = new CountDownLatch(1);
> 46: 
> 47:     volatile static Image img = null;

Please use blessed modifier order: `static volatile`.

I think `img` should not be declared volatile, it's used only on EDT. It may even be non-static because it's accessed only from methods of the test class and you create an instance of it.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 62:

> 60:         panel.add(canvas);
> 61:         frame.add(panel);
> 62:         img = frame.getToolkit().getImage(System.getProperty("test.src", ".") + File.separator + FILENAME);

Could you wrap this long line, please?

The 80-column limit isn't strictly followed, yet it's better to fit into 100-column limit because the code becomes harder to read.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 101:

> 99:             } catch (Exception e) {
> 100:                 throw new RuntimeException(e);
> 101:             }

No checked exception is thrown within this try-block, thus it can be removed.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 111:

> 109:         {
> 110:             EventQueue.invokeAndWait(bug6176679::dispose);
> 111:         }

Suggestion:

        } else {
            EventQueue.invokeAndWait(bug6176679::dispose);
        }

Let's follow Java Coding Style conventions.

I think it also deserves a comment why the frame is disposed of only when the test doesn't fail. Perhaps, we can drop disposing altogether and let jtreg do the cleanup.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 131:

> 129:     }
> 130: 
> 131:     static class imgCanvas extends Canvas {

Suggestion:

    static class ImgCanvas extends Canvas {

Class names should start with a capital letter. Is `ImageCanvas` a better name?

I'd pass the image to the constructor of `ImgCanvas` and mark the `img` field `final` (and `private`). Then the `if` in paint can also be removed.

If the image isn't loaded, that is `img.getWidth()` and `img.getHeight()` return `-1`, the test should bail out before even creating the canvas.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 164:

> 162:                 df[0] = DataFlavor.imageFlavor;
> 163:             }
> 164:         }

You create the instance of `MyTransferable` with the image only, so you can declare the constructor to accept `Image` instead of `Object`.

Would `ImageTransferable` describe the purpose better?

As with the Canvas class above, I'd mark both `img` and `df` as `final` (and `private`).

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 173:

> 171:         public boolean isDataFlavorSupported(DataFlavor flavor) {
> 172:             return true;
> 173:         }

The `@Override` annotation is missing. Add it to `getTransferData` too.

It's not important in this case, yet the method should rather return `df[0].equals(flavor)`.

test/jdk/javax/imageio/plugins/gif/6176679/bug6176679.java line 184:

> 182: 
> 183: 
> 184: }

Please add a final blank line.

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

PR Review: https://git.openjdk.org/jdk/pull/13414#pullrequestreview-1381935281
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164601058
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164600192
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164573562
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164548806
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164533378
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164548321
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164553188
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164589759
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164554274
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164559765
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164566145
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164567413
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164570631
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1164571335