Skip to content

Commit f0afb7f

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 re-applied, the stored image may be disposed by then. This can currently lead to errors because of access to disposed resources. To avoid that, this change ensures that in case a drawn image becomes disposed, a copy of it is created and stored for the lifetime of the GC.
1 parent 239428b commit f0afb7f

File tree

4 files changed

+164
-16
lines changed

4 files changed

+164
-16
lines changed

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

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -491,13 +491,38 @@ 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+
setImage(image);
499+
image.addOnDisposeListener(this::setCopyOfImage);
500+
}
501+
502+
private void setImage(Image image) {
503+
this.image = image;
504+
}
505+
506+
private void setCopyOfImage(Image image) {
507+
if (!GC.this.isDisposed()) {
508+
Image copiedImage = new Image(image.device, image, SWT.IMAGE_COPY);
509+
setImage(copiedImage);
510+
registerForDisposal(copiedImage);
511+
}
512+
}
513+
514+
protected Image getImage() {
515+
return image;
516+
}
517+
518+
}
519+
520+
private class CopyAreaToImageOperation extends ImageOperation {
496521
private final int x;
497522
private final int y;
498523

499524
CopyAreaToImageOperation(Image image, int x, int y) {
500-
this.image = image;
525+
super(image);
501526
this.x = x;
502527
this.y = y;
503528
}
@@ -507,7 +532,7 @@ void apply() {
507532
int zoom = getZoom();
508533
int scaledX = Win32DPIUtils.pointToPixel(drawable, this.x, zoom);
509534
int scaledY = Win32DPIUtils.pointToPixel(drawable, this.y, zoom);
510-
copyAreaInPixels(this.image, scaledX, scaledY);
535+
copyAreaInPixels(getImage(), scaledX, scaledY);
511536
}
512537
}
513538

@@ -1013,18 +1038,17 @@ public void drawImage (Image image, int x, int y) {
10131038
storeAndApplyOperationForExistingHandle(new DrawImageOperation(image, new Point(x, y)));
10141039
}
10151040

1016-
private class DrawImageOperation extends Operation {
1017-
private final Image image;
1041+
private class DrawImageOperation extends ImageOperation {
10181042
private final Point location;
10191043

10201044
DrawImageOperation(Image image, Point location) {
1021-
this.image = image;
1045+
super(image);
10221046
this.location = location;
10231047
}
10241048

10251049
@Override
10261050
void apply() {
1027-
drawImageInPixels(this.image, Win32DPIUtils.pointToPixel(drawable, this.location, getZoom()));
1051+
drawImageInPixels(getImage(), Win32DPIUtils.pointToPixel(drawable, this.location, getZoom()));
10281052
}
10291053

10301054
private void drawImageInPixels(Image image, Point location) {
@@ -1081,13 +1105,12 @@ void drawImage(Image srcImage, int srcX, int srcY, int srcWidth, int srcHeight,
10811105
storeAndApplyOperationForExistingHandle(new DrawImageToImageOperation(srcImage, new Rectangle(srcX, srcY, srcWidth, srcHeight), new Rectangle(destX, destY, destWidth, destHeight), simple));
10821106
}
10831107

1084-
private class DrawScalingImageToImageOperation extends Operation {
1085-
private final Image image;
1108+
private class DrawScalingImageToImageOperation extends ImageOperation {
10861109
private final Rectangle source;
10871110
private final Rectangle destination;
10881111

10891112
DrawScalingImageToImageOperation(Image image, Rectangle source, Rectangle destination) {
1090-
this.image = image;
1113+
super(image);
10911114
this.source = source;
10921115
this.destination = destination;
10931116
}
@@ -1096,7 +1119,7 @@ private class DrawScalingImageToImageOperation extends Operation {
10961119
void apply() {
10971120
int gcZoom = getZoom();
10981121
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);
1122+
drawImage(getImage(), source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, gcZoom, srcImageZoom);
11001123
}
11011124

11021125
private Collection<Integer> getAllCurrentMonitorZooms() {
@@ -1154,22 +1177,21 @@ private void drawImage(Image image, int srcX, int srcY, int srcWidth, int srcHei
11541177
drawImage(image, src.x, src.y, src.width, src.height, dest.x, dest.y, dest.width, dest.height, false, scaledImageZoom);
11551178
}
11561179

1157-
private class DrawImageToImageOperation extends Operation {
1158-
private final Image image;
1180+
private class DrawImageToImageOperation extends ImageOperation {
11591181
private final Rectangle source;
11601182
private final Rectangle destination;
11611183
private final boolean simple;
11621184

11631185
DrawImageToImageOperation(Image image, Rectangle source, Rectangle destination, boolean simple) {
1164-
this.image = image;
1186+
super(image);
11651187
this.source = source;
11661188
this.destination = destination;
11671189
this.simple = simple;
11681190
}
11691191

11701192
@Override
11711193
void apply() {
1172-
drawImage(image, source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, simple, getZoom());
1194+
drawImage(getImage(), source.x, source.y, source.width, source.height, destination.x, destination.y, destination.width, destination.height, simple, getZoom());
11731195
}
11741196
}
11751197

‎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)