From 1ecd0eb250c6587053037dec0bec8c70d26b02ee Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Sun, 31 May 2026 23:34:33 +0200 Subject: [PATCH] [Win32] Correct lifecycle of handles created via GC.getClipping() The GC's getClipping() operation stores region handles representing the clipping region (potentially at different zooms) that is combined with the passed region object. The handles for that passed region object are created lazy, so that the clipping region handle needs to be stored until that point in time. Otherwise it will leads to failures when lazily creating the handle for the passed region later on. Currently, these handles are stored in the GC operations and in case the GC or its operations are disposed too early, the region filled via a GC.getClipping() call will fail to create a proper region handle. This change moves the responsibility for managing the lifecycle of the stored clipping regions to the Region instance in which they are used. Copies of that region ensure via reference counting that only the last disposed Region instance consuming the clipping region will dispose the according handle. A regression test for the scenario that the GC is disposed before consuming the Region handle to which the clipping region is applied is added. --- .../win32/org/eclipse/swt/graphics/GC.java | 16 ++--- .../org/eclipse/swt/graphics/Region.java | 65 +++++++++++++++++-- .../Test_org_eclipse_swt_graphics_GC.java | 14 ++++ 3 files changed, 76 insertions(+), 19 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java index 75889472a5..253411851f 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java +++ b/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,16 +3941,10 @@ public void getClipping (Region region) { } private class GetClippingOperation extends Operation { - private final Map 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 @@ -3957,14 +3952,11 @@ public GetClippingOperation(Region region) { // 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(); } } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Region.java b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Region.java index 08ef72a40e..95d74ed6a8 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Region.java +++ b/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 zoomToRegionHandleMap = new HashMap<>(); + + void register(int zoom, Supplier 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. *

* 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 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 handleForZoomProvider; + private final ZoomToRegionMap zoomToRegionHandleMap; - OperationWithRegionHandle(OperationStrategy operationStrategy, Function 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(); + } + } } diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_GC.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_GC.java index 2806c1d865..8d5eafc830 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_GC.java +++ b/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);