[Win32] Fix potential memory leak in GC#drawImage #2499

When calling GC#drawImage(), an operation is created (like for every
execution on GC) which stores the image to draw. Since the image may be
disposed afterwards, a dispose listener is registered to the image, such
that the image can be copied and maintained inside the operation in case
of a disposal until that operation itself is disposed.
The listener is currently not de-registered when disposing the GC and
its operations, thus the GC operations (and thus also the containing GC)
will still be referenced (as listeners from the image) until that image
is disposed. Since images may be long living (or never be disposed at
all), this can lead to a memory leak.

This change removes the listener from the image upon disposal of a GC
and its operations such that no memory can occur.

Fixes #2499

Author: HeikoKlare

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

@@ -16,6 +16,7 @@
 
 import java.util.*;
 import java.util.List;
+import java.util.function.*;
 import java.util.stream.*;
 
 import org.eclipse.swt.*;
@@ -494,9 +495,11 @@ public void copyArea (Image image, int x, int y) {
 private abstract class ImageOperation extends Operation {
 	private Image image;
 
+	private final Consumer<Image> disposeCallback = this::setCopyOfImage;
+
 	ImageOperation(Image image) {
 		setImage(image);
-		image.addOnDisposeListener(this::setCopyOfImage);
+		image.addOnDisposeListener(disposeCallback);
 	}
 
 	private void setImage(Image image) {
@@ -515,6 +518,11 @@ protected Image getImage() {
 		return image;
 	}
 
+	@Override
+	void disposeAll() {
+		image.removeOnDisposeListener(disposeCallback);
+		super.disposeAll();
+	}
 }
 
 private class CopyAreaToImageOperation extends ImageOperation {

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

@@ -1020,10 +1020,18 @@ void addOnDisposeListener(Consumer<Image> onDisposeListener) {
 	onDisposeListeners.add(onDisposeListener);
 }
 
+void removeOnDisposeListener(Consumer<Image> onDisposeListener) {
+	if (onDisposeListeners == null) {
+		return;
+	}
+	onDisposeListeners.remove(onDisposeListener);
+}
+
 @Override
 public void dispose() {
 	if (onDisposeListeners != null) {
 		onDisposeListeners.forEach(listener -> listener.accept(this));
+		onDisposeListeners.clear();
 	}
 	super.dispose();
 }

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_GC.java

@@ -29,6 +29,7 @@
 import static org.junit.jupiter.api.Assumptions.assumeFalse;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
+import java.lang.ref.WeakReference;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.swt.SWT;
@@ -804,6 +805,24 @@ public void test_bug1288_createGCFromImageFromNonDisplayThread() throws Interrup
 	assertNull(exceptionReference.get(), "Creating a GC from an Image without a device threw an exception");
 }
 
+// Tests that a GC instance is cleaned from memory after it was disposed.
+// Primarily supposed to test that the operations (currently only implemented for Windows) do not produce any leaks.
+@Test
+public void test_noMemoryLeakAfterDispose() {
+	GC testGC = new GC(display);
+	Image image = new Image(display, 1, 1);
+	testGC.setFont(display.getSystemFont());
+	testGC.setClipping(new Rectangle(0, 0, 2, 2));
+	testGC.drawImage(image, 0, 0);
+	testGC.drawText("Test", 0, 0);
+	testGC.drawLine(0, 0, 5, 5);
+	WeakReference<GC> testGCReference = new WeakReference<>(testGC);
+	testGC.dispose();
+	testGC = null;
+	System.gc();
+	assertNull(testGCReference.get());
+}
+
 /* custom */
 Display display;
 Shell shell;