RFR: 8290092: Temporary files are kept when call Clipboard.getSystemClipboard().getImage() [v2]
Ajit Ghaisas
aghaisas at openjdk.org
Tue Feb 7 11:54:57 UTC 2023
On Thu, 2 Feb 2023 16:44:11 GMT, Lukasz Kostyra <lkostyra at openjdk.org> wrote:
>> Windows implementation of GlassClipboard.cpp uses IStorage interface in `PopImage()` to allocate a temporary file, which is then used to capture Image data from system clipboard and move it to JVM side. In order for temporary file to be removed automatically on `IStorage::Release()`, `StgCreateDocfile` API requires passing STGM_DELETEONRELEASE flag - otherwise the file will be left behind when IStorage is released. Contents of temporary file are immediately copied to a JNI Byte Array after acquiring them from clibpoard, so it is safe to provide this flag and remove the file after `PopImage()` is done.
>>
>> Creating a unit test for this case would a bit difficult, as IStorage and its file are created with random temporary name each time, which we cannot easily access. On the other hand, just scouting the temp directory for any leftover .TMP files might prove false results, as other apps or even JFX itself might use IStorage temporary files for some other purposes than managing clipboard images. As such, because this is a simple API change, I did not make a test for this.
>>
>> Tested this change on Windows 10 and it doesn't break any existing tests. Calling `Clipboard.getSystemClipboard().getImage()` with an image stored inside the system clipboard now leaves no leftover files.
>
> Lukasz Kostyra has updated the pull request incrementally with two additional commits since the last revision:
>
> - ClipboardExtImageTest: Add missing copyright notice
> - Add manual test to check for leftover Clipboard files
On Windows - the test fails without the fix and passes with it.
On Mac - (as expected) the test passes with or without fix.
I have a minor comment on the test.
Also, the PR should contain only 2 files -
1. Modified file - modules/javafx.graphics/src/main/native-glass/win/GlassClipboard.cpp
2. Newly added test file - tests/manual/clipboard/ClipboardExtImageTest.java
Please remove all other files. They are not needed to be checked in.
tests/manual/clipboard/ClipboardExtImageTest.java line 193:
> 191: public void start(Stage primaryStage) throws Exception {
> 192: try {
> 193: /*if (!isWindows()) {
This test can be only Windows specific.
I would enable this check and get rid of `isWindows()` check everywhere.
-------------
Changes requested by aghaisas (Reviewer).
PR: https://git.openjdk.org/jfx/pull/994
More information about the openjfx-dev
mailing list