[Win32] Correct lifecycle of handles created via GC.getClipping()

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

@@ -19,6 +19,7 @@
 
 import org.eclipse.swt.*;
 import org.eclipse.swt.graphics.Image.*;
+import org.eclipse.swt.graphics.Region.*;
 import org.eclipse.swt.internal.*;
 import org.eclipse.swt.internal.gdip.*;
 import org.eclipse.swt.internal.win32.*;
@@ -3940,31 +3941,22 @@ public void getClipping (Region region) {
 }
 
 private class GetClippingOperation extends Operation {
-	private final Map<Integer, Long> zoomToRegionHandle = new HashMap<>();
+	private final ZoomToRegionMap zoomToRegionHandle = new ZoomToRegionMap();
 
 	public GetClippingOperation(Region region) {
-		region.set(zoom -> {
-			if (!zoomToRegionHandle.containsKey(zoom)) {
-				System.err.println("No clipping handle for zoom " + zoom + " has been created on this GC");
-				return zoomToRegionHandle.values().iterator().next();
-			}
-			return zoomToRegionHandle.get(zoom);
-		}, getZoom());
+		region.set(zoomToRegionHandle, getZoom());
 	}
 
 	// Whenever the GC handle is recalculated for a new zoom, we compute and store the clipping
 	// at the times when getClipping(Region) was originally called, such that the region to which
 	// that clipping is set can retrieve it from the storage when required.
 	@Override
 	void apply() {
-		zoomToRegionHandle.computeIfAbsent(getZoom(), __ -> getClippingRegion());
+		zoomToRegionHandle.register(getZoom(), () -> getClippingRegion());
 	}
 
 	@Override
 	void disposeAll() {
-		for (long handle : zoomToRegionHandle.values()) {
-			OS.DeleteObject(handle);
-		}
 		super.disposeAll();
 	}
 }

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

@@ -242,6 +242,7 @@ void destroy () {
 	device.deregisterResourceWithZoomSupport(this);
 	zoomToHandle.values().forEach(RegionHandle::destroy);
 	zoomToHandle.clear();
+	operations.forEach(Operation::dereference);
 	operations.clear();
 	this.isDestroyed = true;
 }
@@ -477,18 +478,47 @@ public boolean isEmpty () {
 	});
 }
 
+static class ZoomToRegionMap {
+	private boolean disposed;
+	private Map<Integer, Long> zoomToRegionHandleMap = new HashMap<>();
+
+	void register(int zoom, Supplier<Long> handleSupplier) {
+		if (disposed) {
+			return;
+		}
+		zoomToRegionHandleMap.computeIfAbsent(zoom, __ -> handleSupplier.get());
+	}
+
+	private long get(int zoom) {
+		if (!zoomToRegionHandleMap.containsKey(zoom)) {
+			System.err.println("No handle for " + zoom + " has been created");
+			return zoomToRegionHandleMap.values().iterator().next();
+		}
+		return zoomToRegionHandleMap.get(zoom);
+	}
+
+	void dispose() {
+		if (!disposed) {
+			zoomToRegionHandleMap.values().forEach(handle -> OS.DeleteObject(handle));
+			zoomToRegionHandleMap.clear();
+			disposed = true;
+		}
+	}
+}
+
 /**
  * Specific method for {@link GC#getClipping(Region)} because the current GC
  * clipping settings at that specific point in time of executing the getClipping
- * method need to be stored.
+ * method need to be stored. The responsibility for managing the map (including
+ * disposal of added handles) is handed over with the call of this method.
  * <p>
  * The context zoom is used as a hint for the case of creating temporary
  * handles, such that they can be created for a zoom for which we know that the
  * supplier is capable of providing a proper handle.
  */
-void set(Function<Integer, Long> handleForZoomSupplier, int contextZoom) {
+void set(ZoomToRegionMap zoomToRegionHandleMap, int contextZoom) {
 	this.temporaryHandleZoomHint = contextZoom;
-	final Operation operation = new OperationWithRegionHandle(Operation::set, handleForZoomSupplier);
+	final Operation operation = new OperationWithRegionHandle(Operation::set, zoomToRegionHandleMap);
 	storeAndApplyOperationForAllHandles(operation);
 }
 
@@ -670,6 +700,7 @@ Region copy() {
 	Region region = new Region();
 	region.temporaryHandleZoomHint = temporaryHandleZoomHint;
 	region.operations.addAll(operations);
+	region.operations.forEach(Operation::reference);
 	return region;
 }
 
@@ -719,6 +750,7 @@ private interface OperationStrategy {
 
 private abstract static class Operation {
 	private final OperationStrategy operationStrategy;
+	private int referenceCount = 1;
 
 	Operation(OperationStrategy operationStrategy) {
 		this.operationStrategy = operationStrategy;
@@ -737,6 +769,20 @@ void apply(RegionHandle regionHandle) {
 	abstract void intersect(long handle, int zoom);
 
 	abstract void translate(long handle, int zoom);
+
+	void reference() {
+		referenceCount++;
+	}
+
+	void dereference() {
+		referenceCount--;
+		if (referenceCount == 0) {
+			destroy();
+		}
+	}
+
+	void destroy() {
+	}
 }
 
 private static class OperationWithRectangle extends Operation {
@@ -907,16 +953,16 @@ void translate(long handle, int zoom) {
 }
 
 private static class OperationWithRegionHandle extends Operation {
-	private final Function<Integer, Long> handleForZoomProvider;
+	private final ZoomToRegionMap zoomToRegionHandleMap;
 
-	OperationWithRegionHandle(OperationStrategy operationStrategy, Function<Integer, Long> handleForZoomSupplier) {
+	OperationWithRegionHandle(OperationStrategy operationStrategy, ZoomToRegionMap zoomToRegionHandleMap) {
 		super(operationStrategy);
-		this.handleForZoomProvider = handleForZoomSupplier;
+		this.zoomToRegionHandleMap = zoomToRegionHandleMap;
 	}
 
 	@Override
 	void set(long handle, int zoom) {
-		OS.CombineRgn(handle, handleForZoomProvider.apply(zoom), 0, OS.RGN_COPY);
+		OS.CombineRgn(handle, zoomToRegionHandleMap.get(zoom), 0, OS.RGN_COPY);
 	}
 
 	@Override
@@ -939,5 +985,10 @@ void add(long handle, int zoom) {
 		throw new UnsupportedOperationException();
 	}
 
+	@Override
+	void destroy() {
+		zoomToRegionHandleMap.dispose();
+	}
+
 }
 }

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

@@ -49,6 +49,7 @@
 import org.eclipse.swt.graphics.Point;
 import org.eclipse.swt.graphics.RGB;
 import org.eclipse.swt.graphics.Rectangle;
+import org.eclipse.swt.graphics.Region;
 import org.eclipse.swt.graphics.Transform;
 import org.eclipse.swt.internal.DPIUtil;
 import org.eclipse.swt.tests.graphics.ImageDataTestHelper;
@@ -781,6 +782,19 @@ public void test_setBackgroundLorg_eclipse_swt_graphics_Color() {
 			"No exception thrown for color disposed");
 }
 
+@Test
+public void test_getClipping() {
+	gc.setClipping(0,5,10,20);
+	Region r = new Region();
+	try {
+		gc.getClipping(r);
+		gc.dispose();
+		assertEquals(new Rectangle(0, 5, 10, 20), r.getBounds());
+	} finally {
+		r.dispose();
+	}
+}
+
 @Test
 public void test_setClippingIIII() {
 	gc.setClipping(0,5,10,20);