Skip to content

Commit 3bb6975

Browse files
HeikoKlareHeikoKlare
committed
[Win32] Avoid disposed image usage in GC operations
When an image is drawn on a GC that image may be disposed right afterwards. Since the GC can live much longer and a handle for that GC at a different zoom may be requested later on, such that the operations in the GC are reapplied, the stored image may then be disposed. To avoid that, this change ensures that the GC operations store copies of the images. In case an image is still under construction (i.e., if a GC is currently created on top of the image) the copy cannot be done immediately as the handles for that image are not yet created. Therefore, in such a case the original image is stored and for the case that it is disposed later on, an on-dispose listener is added such that a copy can then be created on demand.
1 parent 28f8835 commit 3bb6975

File tree

4 files changed

+165
-16
lines changed

4 files changed

+165
-16
lines changed

‎bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -491,13 +491,39 @@ public void copyArea (Image image, int x, int y) {
491491
storeAndApplyOperationForExistingHandle(new CopyAreaToImageOperation(image, x, y));
492492
}
493493

494-
private class CopyAreaToImageOperation extends Operation {
495-
private final Image image;
494+
private abstract class ImageOperation extends Operation {
495+
private Image image;
496+
497+
ImageOperation(Image image) {
498+
if (image.memGC != null) {
499+
setImage(image);
500+
image.addOnDisposeListener(disposingImage -> {
501+
setImage(new Image(disposingImage.device, disposingImage, SWT.IMAGE_COPY));
502+
registerForDisposal(image);
503+
});
504+
} else {
505+
Image imageCopy = new Image(image.device, image, SWT.IMAGE_COPY);
506+
setImage(imageCopy);
507+
registerForDisposal(imageCopy);
508+
}
509+
}
510+
511+
private void setImage(Image image) {
512+
this.image = image;
513+
}
514+
515+
protected Image getImage() {
516+
return image;
517+
}
518+
519+
}
520+
521+
private class CopyAreaToImageOperation extends ImageOperation {
496522
private final int x;
497523
private final int y;
498524

499525
CopyAreaToImageOperation(Image image, int x, int y) {
500-
this.image = image;
526+
super(image);
501527
this.x = x;
502528
this.y = y;
503529
}
@@ -507,7 +533,7 @@ void apply() {
507533
int zoom = getZoom();
508534
int scaledX = Win32DPIUtils.pointToPixel(drawable, this.x, zoom);
509535
int scaledY = Win32DPIUtils.pointToPixel(drawable, this.y, zoom);
510-
copyAreaInPixels(this.image, scaledX, scaledY);
536+
copyAreaInPixels(getImage(), scaledX, scaledY);
511537
}
512538
}
513539

@@ -1013,18 +1039,17 @@ public void drawImage (Image image, int x, int y) {
10131039
storeAndApplyOperationForExistingHandle(new DrawImageOperation(image, new Point(x, y)));
10141040
}
10151041

1016-
private class DrawImageOperation extends Operation {
1017-
private final Image image;
1042+
private class DrawImageOperation extends ImageOperation {
10181043
private final Point location;
10191044

10201045
DrawImageOperation(Image image, Point location) {
1021-
this.image = image;
1046+
super(image);
10221047
this.location = location;
10231048
}
10241049

10251050
@Override
10261051
void apply() {
1027-
drawImageInPixels(this.image, Win32DPIUtils.pointToPixel(drawable, this.location, getZoom()));
1052+
drawImageInPixels(getImage(), Win32DPIUtils.pointToPixel(drawable, this.location, getZoom()));
10281053
}
10291054

10301055
private void drawImageInPixels(Image image, Point location) {
@@ -1081,13 +1106,12 @@ void drawImage(Image srcImage, int srcX, int srcY, int srcWidth, int srcHeight,
10811106
storeAndApplyOperationForExistingHandle(new DrawImageToImageOperation(srcImage, new Rectangle(srcX, srcY, srcWidth, srcHeight), new Rectangle(destX, destY, destWidth, destHeight), simple));
10821107
}
10831108

1084-
private class DrawScalingImageToImageOperation extends Operation {
1085-
private final Image image;
1109+
private class DrawScalingImageToImageOperation extends ImageOperation {
10861110
private final Rectangle source;
10871111
private final Rectangle destination;
10881112

10891113
DrawScalingImageToImageOperation(Image image, Rectangle source, Rectangle destination) {
1090-
this.image = image;
1114+
super(image);
10911115
this.source = source;
10921116
this.destination = destination;
10931117
}
@@ -1096,7 +1120,7 @@ private class DrawScalingImageToImageOperation extends Operation {
10961120
void apply() {
10971121
int gcZoom = getZoom();
10981122
int srcImageZoom = calculateZoomForImage(gcZoom, source.width, source.height, destination.width, destination.height);
1099-
drawImage(image, source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, gcZoom, srcImageZoom);
1123+
drawImage(getImage(), source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, gcZoom, srcImageZoom);
11001124
}
11011125

11021126
private Collection<Integer> getAllCurrentMonitorZooms() {
@@ -1154,22 +1178,21 @@ private void drawImage(Image image, int srcX, int srcY, int srcWidth, int srcHei
11541178
drawImage(image, src.x, src.y, src.width, src.height, dest.x, dest.y, dest.width, dest.height, false, scaledImageZoom);
11551179
}
11561180

1157-
private class DrawImageToImageOperation extends Operation {
1158-
private final Image image;
1181+
private class DrawImageToImageOperation extends ImageOperation {
11591182
private final Rectangle source;
11601183
private final Rectangle destination;
11611184
private final boolean simple;
11621185

11631186
DrawImageToImageOperation(Image image, Rectangle source, Rectangle destination, boolean simple) {
1164-
this.image = image;
1187+
super(image);
11651188
this.source = source;
11661189
this.destination = destination;
11671190
this.simple = simple;
11681191
}
11691192

11701193
@Override
11711194
void apply() {
1172-
drawImage(image, source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, simple, getZoom());
1195+
drawImage(getImage(), source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, simple, getZoom());
11731196
}
11741197
}
11751198

‎bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ public final class Image extends Resource implements Drawable {
139139

140140
private Map<Integer, ImageHandle> zoomLevelToImageHandle = new HashMap<>();
141141

142+
private List<Consumer<Image>> onDisposeListeners;
143+
142144
private Image (Device device, int type, long handle, int nativeZoom) {
143145
super(device);
144146
this.type = type;
@@ -997,6 +999,21 @@ public static void drawScaled(GC gc, Image original, int width, int height, floa
997999
return null;
9981000
}
9991001

1002+
void addOnDisposeListener(Consumer<Image> onDisposeListener) {
1003+
if (onDisposeListeners == null) {
1004+
onDisposeListeners = new ArrayList<>();
1005+
}
1006+
onDisposeListeners.add(onDisposeListener);
1007+
}
1008+
1009+
@Override
1010+
public void dispose() {
1011+
if (onDisposeListeners != null) {
1012+
onDisposeListeners.forEach(listener -> listener.accept(this));
1013+
}
1014+
super.dispose();
1015+
}
1016+
10001017
@Override
10011018
void destroy () {
10021019
device.deregisterResourceWithZoomSupport(this);

‎tests/org.eclipse.swt.tests.win32/JUnit Tests/org/eclipse/swt/graphics/ImageWin32Tests.java‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,13 @@
1717
import static org.junit.jupiter.api.Assertions.assertEquals;
1818
import static org.junit.jupiter.api.Assertions.assertNotEquals;
1919

20+
import org.eclipse.swt.SWT;
2021
import org.eclipse.swt.internal.DPIUtil;
2122
import org.eclipse.swt.widgets.Display;
2223
import org.junit.jupiter.api.BeforeEach;
2324
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.params.ParameterizedTest;
26+
import org.junit.jupiter.params.provider.ValueSource;
2427

2528
/**
2629
* Automated Tests for class org.eclipse.swt.graphics.Image
@@ -67,4 +70,108 @@ public void testImageShouldHaveDimesionAsPerZoomLevel() {
6770
}
6871
}
6972

73+
@ParameterizedTest
74+
@ValueSource(booleans = { true, false } )
75+
public void testRetrieveImageDataAtDifferentZooms(boolean concurrentlyDisposeDrawnImage) {
76+
Color imageColor = display.getSystemColor(SWT.COLOR_RED);
77+
Image image = new Image (display, 1, 1);
78+
GC gc = new GC(image);
79+
80+
try {
81+
Image temporaryImage = createImageOfSizeOne(imageColor);
82+
gc.drawImage(temporaryImage, 0, 0);
83+
84+
if (concurrentlyDisposeDrawnImage) {
85+
temporaryImage.dispose();
86+
}
87+
ImageData resultImageDataAtDifferentZoom = image.getImageData(200);
88+
89+
assertEquals(imageColor.getRGB(), getRGBAtPixel(resultImageDataAtDifferentZoom, 0, 0));
90+
91+
if (!concurrentlyDisposeDrawnImage) {
92+
temporaryImage.dispose();
93+
}
94+
} finally {
95+
gc.dispose();
96+
image.dispose();
97+
}
98+
}
99+
100+
private Image createImageOfSizeOne(Color imageColor) {
101+
Image imageToDraw = new Image (display, 1, 1);
102+
GC imageToDrawGc = new GC(imageToDraw);
103+
imageToDrawGc.setBackground(imageColor);
104+
imageToDrawGc.fillRectangle(0, 0, 1, 1);
105+
imageToDrawGc.dispose();
106+
return imageToDraw;
107+
}
108+
109+
private RGB getRGBAtPixel(ImageData resultImageDataAtDifferentZoom, int x, int y) {
110+
return resultImageDataAtDifferentZoom.palette.getRGB(resultImageDataAtDifferentZoom.getPixel(x, y));
111+
}
112+
113+
@ParameterizedTest
114+
@ValueSource(booleans = { true, false } )
115+
public void testDrawImageAtDifferentZooms(boolean concurrentlyDisposeDrawnImage) {
116+
Color imageColor = display.getSystemColor(SWT.COLOR_RED);
117+
Image image = new Image (display, 1, 1);
118+
GC gc = new GC(image);
119+
Image temporaryImage = createImageOfSizeOne(imageColor);
120+
try {
121+
gc.drawImage(temporaryImage, 0, 0);
122+
} finally {
123+
if (concurrentlyDisposeDrawnImage) {
124+
temporaryImage.dispose();
125+
}
126+
}
127+
128+
Image targetCanvas = new Image(display, 5, 5);
129+
GC targetCanvasGc = new GC(targetCanvas);
130+
try {
131+
targetCanvasGc.getGCData().nativeZoom = 200;
132+
targetCanvasGc.drawImage(image, 0, 0);
133+
targetCanvasGc.getGCData().nativeZoom = 100;
134+
targetCanvasGc.drawImage(image, 3, 0);
135+
ImageData resultImageData = targetCanvas.getImageData(100);
136+
137+
assertEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 0, 0));
138+
assertEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 1, 0));
139+
assertNotEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 2, 0));
140+
assertEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 3, 0));
141+
assertNotEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 4, 0));
142+
} finally {
143+
if (!concurrentlyDisposeDrawnImage) {
144+
temporaryImage.dispose();
145+
}
146+
image.dispose();
147+
targetCanvasGc.dispose();
148+
gc.dispose();
149+
}
150+
}
151+
152+
@Test
153+
public void testDisposeDrawnImageBeforeRequestingTargetForOtherZoom() {
154+
Color imageColor = display.getSystemColor(SWT.COLOR_RED);
155+
Image imageToDraw = new Image (display, 1, 1);
156+
GC imageToDrawGc = new GC(imageToDraw);
157+
imageToDrawGc.setBackground(imageColor);
158+
imageToDrawGc.fillRectangle(0, 0, 1, 1);
159+
160+
Image targetCanvas = new Image(display, 5, 5);
161+
GC targetCanvasGc = new GC(targetCanvas);
162+
try {
163+
targetCanvasGc.drawImage(imageToDraw, 0, 0);
164+
imageToDrawGc.dispose();
165+
imageToDraw.dispose();
166+
ImageData resultImageData = targetCanvas.getImageData(200);
167+
168+
assertEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 0, 0));
169+
assertEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 1, 0));
170+
assertNotEquals(imageColor.getRGB(), getRGBAtPixel(resultImageData, 2, 0));
171+
} finally {
172+
targetCanvasGc.dispose();
173+
targetCanvas.dispose();
174+
}
175+
}
176+
70177
}

‎tests/org.eclipse.swt.tests.win32/META-INF/MANIFEST.MF‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,6 @@ Eclipse-BundleShape: dir
99
Bundle-RequiredExecutionEnvironment: JavaSE-17
1010
Automatic-Module-Name: org.eclipse.swt.tests.win32
1111
Import-Package: org.junit.jupiter.api;version="[5.13.0,6.0.0)",
12+
org.junit.jupiter.params;version="[5.13.0,6.0.0)",
13+
org.junit.jupiter.params.provider;version="[5.13.0,6.0.0)",
1214
org.junit.platform.suite.api;version="[1.13.0,2.0.0)"

0 commit comments

Comments
 (0)