Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 22 commits into from

Conversation

rajamah
Copy link
Member

@rajamah rajamah commented Apr 10, 2023

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-6176679: Application freezes when copying an animated gif image to the system clipboard

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13414/head:pull/13414
$ git checkout pull/13414

Update a local copy of the PR:
$ git checkout pull/13414
$ git pull https://git.openjdk.org/jdk.git pull/13414/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13414

View PR using the GUI difftool:
$ git pr show -t 13414

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13414.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 10, 2023

👋 Welcome back rmahajan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Apr 10, 2023

@rajamah The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Apr 10, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 11, 2023
@mlbridge
Copy link

mlbridge bot commented Apr 11, 2023

Copy link
Member

@jayathirthrao jayathirthrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using .gif image as input, its better if we can create ImageStream as we do in some of animation tests under test/jdk/javax/imageio/plugins/gif.

@aivanov-jdk
Copy link
Member

Instead of using .gif image as input, its better if we can create ImageStream as we do in some of animation tests under test/jdk/javax/imageio/plugins/gif.

@jayathirthrao Could you elaborate, please? How is ImageStream better than reading from a file directly?
Do you mean that it's better to store the GIF as a byte array or base64-encoded string so that there's no external .gif file? Do you mean generating the GIF by code on the fly? Anything else?

Comment on lines 32 to 33
import java.awt.*;
import java.awt.datatransfer.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please expand the wildcard imports.

import static java.util.concurrent.TimeUnit.MILLISECONDS;


public class bug6176679 implements ClipboardOwner, FlavorListener {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

* @test
* @key headful
* @bug 6176679
* @summary Tests that an application doesn't freeze when copying an animated gif image to the system clipboard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static final CountDownLatch latch = new CountDownLatch(1);

volatile static Image img = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 96 to 101
try {
test.copyImage();
latch.countDown();
} catch (Exception e) {
throw new RuntimeException(e);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}


}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a final blank line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Panel panel = new Panel(new GridLayout(1, 1));
panel.add(canvas);
frame.add(panel);
img = frame.getToolkit().getImage(System.getProperty("test.src", ".") + File.separator + FILENAME);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -109,7 +109,7 @@ public synchronized void reconstruct(int flags) {
src.checkSecurity(null, false);
}
int missinginfo = flags & ~availinfo;
if ((availinfo & ImageObserver.ERROR) == 0 && missinginfo != 0) {
if ((availinfo & (ImageObserver.ERROR | ImageObserver.FRAMEBITS) ) == 0 && missinginfo != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ((availinfo & (ImageObserver.ERROR | ImageObserver.FRAMEBITS) ) == 0 && missinginfo != 0) {
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.

@mlbridge
Copy link

mlbridge bot commented Apr 12, 2023

Mailing list message from Alan Snyder on client-libs-dev:

Isn?t there a deeper issue here?

I ran into a problem trying to analyze an image using a filter. The code was waiting for ALLBITS to be reported (as well as ERROR conditions), but for an animated GIF that never happens. I worked around that problem by stopping when a FRAME was reported, which was good enough for my purposes.

I did not see any way to indicate that I did not want an animation or to determine when every frame had been produced once.

Did I miss something?

There are many places where ALLBITS is tested. Are all of these places at risk when operating on an animated GIF?

Implementing animation by simulating an image of infinite size seems like a bad idea, to put it mildly.

@jayathirthrao
Copy link
Member

Instead of using .gif image as input, its better if we can create ImageStream as we do in some of animation tests under test/jdk/javax/imageio/plugins/gif.

@jayathirthrao Could you elaborate, please? How is ImageStream better than reading from a file directly? Do you mean that it's better to store the GIF as a byte array or base64-encoded string so that there's no external .gif file? Do you mean generating the GIF by code on the fly? Anything else?

Yes i mean generating GIF by code and not using .gif file as input.

@rajamah
Copy link
Member Author

rajamah commented Apr 13, 2023

Will the generated GIF file be animated ?, because this only happens with animated GIFs.
And why is it better than having the GIF file itself ?

@jayathirthrao
Copy link
Member

Will the generated GIF file be animated ?, because this only happens with animated GIFs. And why is it better than having the GIF file itself ?

As previously mentioned we have animation tests for GIF under test/jdk/javax/imageio/plugins/gif which don't use GIF file input.
We try to avoid adding new image file to JDK repo whenever we have some ImageIO test because it increases the size, so its better to use stream or create and delete temp file in regression tests.

@aivanov-jdk
Copy link
Member

Yes i mean generating GIF by code and not using .gif file as input.

Will the generated GIF file be animated ?, because this only happens with animated GIFs. And why is it better than having the GIF file itself ?

It should if you create several frames.

There's always an option to create another GIF image, smaller, and use its byte or base64 representation as the source.

We try to avoid adding new image file to JDK repo whenever we have some ImageIO test because it increases the size, so its better to use stream or create and delete temp file in regression tests.

Okay, now it's clear.

@mlbridge
Copy link

mlbridge bot commented Apr 13, 2023

Mailing list message from Aleksei Ivanov on client-libs-dev:

Hi Alan,

On 12/04/2023 14:05, Alan Snyder wrote:

Isn?t there a deeper issue here?

I ran into a problem trying to analyze an image using a filter. The code was waiting for ALLBITS to be reported (as well as ERROR conditions), but for an animated GIF that never happens. I worked around that problem by stopping when a FRAME was reported, which was good enough for my purposes.

This is what this fix does: ALLBITS is never set for animated images, so
FRAMEBITS is also added.

I did not see any way to indicate that I did not want an animation or to determine when every frame had been produced once.

In the test code, when MediaTracker.waitForAll() returns, which
indicates the image is loaded completely, only WIDTH, HEIGHT and
PROPERTIES bits are set. FRAMEBITS bit is set only after the image
starts animating.

Did I miss something?

There are many places where ALLBITS is tested. Are all of these places at risk when operating on an animated GIF?

I did a quick search for ALLBITS, I can see both FRAMEBITS | ALLBITS are
used in many places; however, there are quite a few places in the code
where ALLBITS is used exclusively, it may cause issues if used with
animated images.

Implementing animation by simulating an image of infinite size seems like a bad idea, to put it mildly.

<SNIP>

--
Regards,
Alexey

@rajamah
Copy link
Member Author

rajamah commented Apr 14, 2023

Made changes requested by the reviewers. Now we read animated gif image from byte array.
Thanks @aivanov-jdk for the help with this.

@mlbridge
Copy link

mlbridge bot commented Apr 15, 2023

Mailing list message from Alan Snyder on client-libs-dev:

On Apr 14, 2023, at 3:48 PM, Rajat Mahajan <rmahajan at openjdk.org> wrote:

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.

Is it really the case that multiple frames are copied one at a time?

I don?t see that, and don?t see how that could work.

I?m not saying it is wrong to copy only the first frame, as that is the only reasonable option if the goal is to transfer a BufferedImage.

I assume there is no Java representation of a multi-frame image that could be copied?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20230414/582a7883/attachment.htm>

@rajamah
Copy link
Member Author

rajamah commented Apr 17, 2023

Mailing list message from Alan Snyder on client-libs-dev:

On Apr 14, 2023, at 3:48 PM, Rajat Mahajan wrote:

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.

Is it really the case that multiple frames are copied one at a time?

I don?t see that, and don?t see how that could work.

I?m not saying it is wrong to copy only the first frame, as that is the only reasonable option if the goal is to transfer a BufferedImage.

I assume there is no Java representation of a multi-frame image that could be copied?

-------------- next part -------------- An HTML attachment was scrubbed... URL: https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20230414/582a7883/attachment.htm

Yes I see that the frame is copied one at a time. When I debug and step in reconstruct() is called per frame.
I am not aware of Java representation of a multi-frame image.

(byte) 0x00, (byte) 0x00, (byte) 0x02, (byte) 0x04, (byte) 0x8c,
(byte) 0x8f, (byte) 0x19, (byte) 0x05, (byte) 0x00, (byte) 0x3b
};
private void createGUI() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a blank line between methods, fields and methods, between class declarations.

A blank line between unrelated fields is also a good idea.

private void copyImage() {
Clipboard sys = Toolkit.getDefaultToolkit().getSystemClipboard();
sys.setContents(new MyTransferable(img), null);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The blank line in the end of the method is redundant.

frame = null;
}
}
private static class imgCanvas extends Canvas {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class names start with a capital letter, method names and fields start with a lower-case letter.

Is ImageCanvas a better name?

Other points from this comment have been addressed.

}
}

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a final blank line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the test should not be in test/jdk/javax/imageio/plugins/gif because it does not test the GIF Image IO plugin. A better location is under test/java/awt/Clipboard.

Now that there's no external GIF image file, the bugid directory has become unnecessary. We're striving to keep test structure flatter without creating a new folder for each test as it used to be done previously.

rajamah and others added 5 commits April 19, 2023 09:30
…on.java

Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
…on.java

Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
….java

Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
@openjdk openjdk bot removed the rfr Pull request is ready for review label Apr 19, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 19, 2023
@mlbridge
Copy link

mlbridge bot commented Apr 22, 2023

Mailing list message from Alan Snyder on client-libs-dev:

I was hoping to be able to run your test program, but I cannot because on macOS there is no support for transferring a native image.

It also appears that I cannot simulate the transfer code because that code depends upon the reconstruct method, which is not public.

Just looking at the code, I think it is not correct.

It looks to me that the ImageRepresentation.drawToBufImage method is called only once, and it will do nothing unless ALLBITS or FRAMEBITS is true.
I do not see any loop that would call it multiple times. (If I am correct about this, then only the first frame is drawn into the buffered image, which makes much more sense than drawing every frame into the buffered image. One might ask, however, what a user is trying to accomplish by copying an animated image to the system clipboard?)

So, it works if reconstruct() waits until ALLBITS or FRAMEBITS is true. But your change does not test for FRAMEBITS in the loop, so it will only work if FRAMEBITS is already true when reconstruct() is called.

How can that happen?

I think it works because you are displaying the image on the screen, and the animation code is reading frames.

I suggest trying the test without displaying the image.

Also, your test would better if it retrieved the native image from the system clipboard to verify that it was successfully transferred. You would need to clear the system clipboard before running the test to be sure you are not retrieving something from a previous test run.

@rajamah
Copy link
Member Author

rajamah commented Apr 30, 2023

Mailing list message from Alan Snyder on client-libs-dev:

I was hoping to be able to run your test program, but I cannot because on macOS there is no support for transferring a native image.

It also appears that I cannot simulate the transfer code because that code depends upon the reconstruct method, which is not public.

Just looking at the code, I think it is not correct.

It looks to me that the ImageRepresentation.drawToBufImage method is called only once, and it will do nothing unless ALLBITS or FRAMEBITS is true. I do not see any loop that would call it multiple times. (If I am correct about this, then only the first frame is drawn into the buffered image, which makes much more sense than drawing every frame into the buffered image. One might ask, however, what a user is trying to accomplish by copying an animated image to the system clipboard?)

So, it works if reconstruct() waits until ALLBITS or FRAMEBITS is true. But your change does not test for FRAMEBITS in the loop, so it will only work if FRAMEBITS is already true when reconstruct() is called.

How can that happen?

I think it works because you are displaying the image on the screen, and the animation code is reading frames.

I suggest trying the test without displaying the image.

Thanks for your suggestions. I have modified the code so it works for both cases where image is and is not displayed on screen.
I have modified the test so it tests for both these cases.

Also, your test would better if it retrieved the native image from the system clipboard to verify that it was successfully transferred. You would need to clear the system clipboard before running the test to be sure you are not retrieving something from a previous test run.

I am not sure what you mean by native image. Could you please clarify ?

@mlbridge
Copy link

mlbridge bot commented May 1, 2023

Mailing list message from Alan Snyder on client-libs-dev:

Your changes look reasonable.

I have a suggestion. Unless IDEA is lying to me, all uses of reconstruct() pass ALLBITS as the parameter.

The net effect is to wait for any of the three terminating conditions, ALLBITS, FRAMEBITS, and ERROR.

My suggestion would be to eliminate the parameter, add ALLBITS to the test in the loop, and rename the method waitForTermination() or something like that.

(The existing method can hang if ALLBITS is not true in the parameter, which makes the method unnecessarily fragile.)

I am not sure what you mean by native image. Could you please clarify ?

I think I was mistaken. I tried again and was able to run your test.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20230430/3e4de822/attachment-0001.htm>

@rajamah
Copy link
Member Author

rajamah commented May 1, 2023

Mailing list message from Alan Snyder on client-libs-dev:

Your changes look reasonable.

I have a suggestion. Unless IDEA is lying to me, all uses of reconstruct() pass ALLBITS as the parameter.

The net effect is to wait for any of the three terminating conditions, ALLBITS, FRAMEBITS, and ERROR.

My suggestion would be to eliminate the parameter, add ALLBITS to the test in the loop, and rename the method waitForTermination() or something like that.

(The existing method can hang if ALLBITS is not true in the parameter, which makes the method unnecessarily fragile.)

I am not sure what you mean by native image. Could you please clarify ?

I think I was mistaken. I tried again and was able to run your test.

-------------- next part -------------- An HTML attachment was scrubbed... URL: https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20230430/3e4de822/attachment-0001.htm

Thanks Alan for this suggestion. I have created a new JBS bug JDK-8307183 for refactoring this code as I thought it is not in scope of this bug and would do it as a follow up change to this one.

…on.java

Co-authored-by: Alexey Ivanov <alexey.ivanov@oracle.com>
@openjdk openjdk bot removed the rfr Pull request is ready for review label May 2, 2023
@rajamah rajamah requested a review from jayathirthrao May 3, 2023 03:31
Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now.

You have to address the trailing whitespace error too.

test/jdk/java/awt/Clipboard/CopyAnimatedGIFTest.java Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented May 4, 2023

@rajamah This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: aivanov, dmarkov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 397 new commits pushed to the master branch:

  • b5a4827: 8306871: Open source more AWT Drag & Drop tests
  • 47422be: 8307375: Alignment check on layouts used as sequence element is not correct
  • 3968ab5: 8307395: Add missing STS to Shenandoah
  • 12d6ec6: 8307236: Rendezvous GC threads under STS for monitor deflation
  • 6fe959c: 8307306: Change some ConstantPool::name_ref_at calls to uncached_name_ref_at
  • 3f6a354: 8305169: java/security/cert/CertPathValidator/OCSP/GetAndPostTests.java -- test server didn't start in timely manner
  • f143bf7: 8305084: Remove the removal warnings for finalize() from test/hotspot/jtreg/serviceability/dcmd/gc/FinalizerInfoTest.java and RunFinalizationTest.java
  • 746f8d1: 8305714: Add an extra test for JDK-8292755
  • 1a1ce66: 8305080: Suppress the 'removal' warning for finalize() from test/hotspot/jtreg/compiler/jvmci/common/testcases that used in compiler/jvmci/compilerToVM/ tests
  • 3b430b9: 8250596: Update remaining manpage references from "OS X" to "macOS"
  • ... and 387 more: https://git.openjdk.org/jdk/compare/507c49a3abc0b610a4f7cbc4d3c5aaaaf8ad3534...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jayathirthrao, @aivanov-jdk, @dmarkov20) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 4, 2023
@rajamah
Copy link
Member Author

rajamah commented May 5, 2023

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label May 5, 2023
@openjdk
Copy link

openjdk bot commented May 5, 2023

@rajamah
Your change (at version 92ace81) is now ready to be sponsored by a Committer.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented May 5, 2023

Going to push as commit 6c71859.
Since your change was applied there have been 399 commits pushed to the master branch:

  • 65a5488: 8306712: CDS DeterministicDump.java test fails with -XX:+UseStringDeduplication
  • d8b230c: 8307301: Update HarfBuzz to 7.2.0
  • b5a4827: 8306871: Open source more AWT Drag & Drop tests
  • 47422be: 8307375: Alignment check on layouts used as sequence element is not correct
  • 3968ab5: 8307395: Add missing STS to Shenandoah
  • 12d6ec6: 8307236: Rendezvous GC threads under STS for monitor deflation
  • 6fe959c: 8307306: Change some ConstantPool::name_ref_at calls to uncached_name_ref_at
  • 3f6a354: 8305169: java/security/cert/CertPathValidator/OCSP/GetAndPostTests.java -- test server didn't start in timely manner
  • f143bf7: 8305084: Remove the removal warnings for finalize() from test/hotspot/jtreg/serviceability/dcmd/gc/FinalizerInfoTest.java and RunFinalizationTest.java
  • 746f8d1: 8305714: Add an extra test for JDK-8292755
  • ... and 389 more: https://git.openjdk.org/jdk/compare/507c49a3abc0b610a4f7cbc4d3c5aaaaf8ad3534...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 5, 2023
@openjdk openjdk bot closed this May 5, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 5, 2023
@openjdk
Copy link

openjdk bot commented May 5, 2023

@aivanov-jdk @rajamah Pushed as commit 6c71859.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants