Fix race condition in ImageHandleManager with ConcurrentHashMap

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

@@ -19,6 +19,7 @@
 import java.io.*;
 import java.util.*;
 import java.util.Map.*;
+import java.util.concurrent.*;
 import java.util.function.*;
 import java.util.stream.*;
 
@@ -138,7 +139,7 @@ public final class Image extends Resource implements Drawable {
 	private final ImageHandleManager imageHandleManager = new ImageHandleManager();
 
 	private class ImageHandleManager {
-		private Map<Integer, DestroyableImageHandle> zoomLevelToImageHandle = new HashMap<>();
+		private final ConcurrentMap<Integer, DestroyableImageHandle> zoomLevelToImageHandle = new ConcurrentHashMap<>();
 
 		InternalImageHandle get(int zoom) {
 			final DestroyableImageHandle imageHandle = zoomLevelToImageHandle.get(zoom);
@@ -154,14 +155,7 @@ InternalImageHandle getOrCreate(int zoom, Supplier<DestroyableImageHandle> creat
 				return null;
 			}
 
-			DestroyableImageHandle imageHandle = (DestroyableImageHandle) get(zoom);
-			if (imageHandle != null) {
-				return imageHandle;
-			}
-
-			imageHandle = creator.get();
-			zoomLevelToImageHandle.put(zoom, imageHandle);
-			return imageHandle;
+			return zoomLevelToImageHandle.computeIfAbsent(zoom, k -> creator.get());
 		}
 
 		boolean contains(int zoom) {
@@ -187,7 +181,6 @@ void destroyHandles(Predicate<Integer> filter) {
 				if (filter.test(zoomToHandle.getKey())) {
 					DestroyableImageHandle imageHandle = zoomToHandle.getValue();
 					it.remove();
-					zoomLevelToImageHandle.remove(imageHandle.zoom(), imageHandle);
 					imageHandle.destroy();
 				}
 			}
@@ -2139,7 +2132,7 @@ protected ElementAtZoom<ImageData> loadImageData(int zoom) {
 }
 
 private abstract class ImageFromImageDataProviderWrapper extends AbstractImageProviderWrapper {
-	private final Map<Integer, ImageData> cachedImageData = new HashMap<>();
+	private final ConcurrentMap<Integer, ImageData> cachedImageData = new ConcurrentHashMap<>();
 
 
 	void initImage() {
@@ -2423,7 +2416,7 @@ public boolean equals(Object otherProvider) {
 }
 
 private abstract class BaseImageProviderWrapper<T> extends DynamicImageProviderWrapper {
-	private final Map<Integer, ImageData> cachedImageData = new HashMap<>();
+	private final ConcurrentMap<Integer, ImageData> cachedImageData = new ConcurrentHashMap<>();
 
 	protected final T provider;
 

examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet_ImageHandleRaceCondition.java

@@ -0,0 +1,168 @@
+/*******************************************************************************
+ * Copyright (c) 2026 Contributors and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     Contributors - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.swt.snippets;
+
+import java.util.concurrent.*;
+import java.util.concurrent.atomic.*;
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.graphics.*;
+import org.eclipse.swt.widgets.Display;
+
+/**
+ * Snippet to demonstrate potential race condition in ImageHandleManager.
+ * 
+ * This snippet creates multiple threads that concurrently access the same Image
+ * at the same zoom level, which could trigger the race condition in 
+ * ImageHandleManager.getOrCreate() if it's not properly synchronized.
+ * 
+ * The race condition occurs when:
+ * 1. Thread A calls getOrCreate(zoom) - checks if handle exists (returns null)
+ * 2. Thread B calls getOrCreate(zoom) - checks if handle exists (returns null)
+ * 3. Thread A creates new handle via creator.get()
+ * 4. Thread B creates ANOTHER handle via creator.get() (wasteful!)
+ * 5. Both threads put() their handles, one overwrites the other (resource leak!)
+ * 
+ * For instructions on how to use this snippet, see:
+ * http://www.eclipse.org/swt/
+ */
+public class Snippet_ImageHandleRaceCondition {
+
+	private static final int NUM_THREADS = 20;
+	private static final int ITERATIONS_PER_THREAD = 100;
+	private static final int TARGET_ZOOM = 150;
+
+	// Track handle creation to detect duplicate creations
+	private static final AtomicInteger handleCreationCount = new AtomicInteger(0);
+	private static final ConcurrentHashMap<Integer, Integer> handleCreationsByZoom = new ConcurrentHashMap<>();
+
+	public static void main(String[] args) throws Exception {
+		Display display = new Display();
+
+		System.out.println("=== ImageHandleManager Race Condition Test ===");
+		System.out.println("This test attempts to trigger the race condition by having");
+		System.out.println("multiple threads concurrently access the same Image at the same zoom level.\n");
+
+		// Create an ImageDataProvider that tracks handle creations
+		ImageDataProvider provider = new ImageDataProvider() {
+			@Override
+			public ImageData getImageData(int zoom) {
+				// Track each time we're asked to create image data for a zoom level
+				int count = handleCreationsByZoom.compute(zoom, (k, v) -> v == null ? 1 : v + 1);
+				handleCreationCount.incrementAndGet();
+
+				// Add a small delay to increase chance of race condition
+				try {
+					Thread.sleep(1);
+				} catch (InterruptedException e) {
+					Thread.currentThread().interrupt();
+				}
+
+				// Create image data
+				int size = 16 * zoom / 100;
+				ImageData imageData = new ImageData(size, size, 24, 
+					new PaletteData(0xFF, 0xFF00, 0xFF0000));
+
+				// Fill with a simple pattern
+				for (int y = 0; y < size; y++) {
+					for (int x = 0; x < size; x++) {
+						imageData.setPixel(x, y, (x + y) % 2 == 0 ? 0xFFFFFF : 0x000000);
+					}
+				}
+
+				if (count > 1) {
+					System.err.println("WARNING: Image data requested " + count + " times for zoom " + zoom + 
+						" (possible race condition!)");
+				}
+
+				return imageData;
+			}
+		};
+
+		Image image = new Image(display, provider);
+
+		// Force initial creation at 100% zoom
+		image.getImageData(100);
+		handleCreationsByZoom.clear();
+		handleCreationCount.set(0);
+
+		System.out.println("Starting concurrent access test with " + NUM_THREADS + " threads...\n");
+
+		// Create thread pool
+		ExecutorService executor = Executors.newFixedThreadPool(NUM_THREADS);
+		CountDownLatch startLatch = new CountDownLatch(1);
+		CountDownLatch doneLatch = new CountDownLatch(NUM_THREADS);
+
+		// Submit tasks that will all try to access the image at the same zoom level
+		for (int i = 0; i < NUM_THREADS; i++) {
+			final int threadId = i;
+			executor.submit(() -> {
+				try {
+					// Wait for all threads to be ready
+					startLatch.await();
+
+					// Each thread tries to get the image handle multiple times
+					for (int j = 0; j < ITERATIONS_PER_THREAD; j++) {
+						display.syncExec(() -> {
+							try {
+								// This should trigger getOrCreate() in ImageHandleManager
+								@SuppressWarnings("unused")
+								ImageData data = image.getImageData(TARGET_ZOOM);
+							} catch (Exception e) {
+								System.err.println("Thread " + threadId + " iteration " + j + " failed: " + e.getMessage());
+								e.printStackTrace();
+							}
+						});
+					}
+				} catch (InterruptedException e) {
+					Thread.currentThread().interrupt();
+				} finally {
+					doneLatch.countDown();
+				}
+			});
+		}
+
+		// Start all threads at once
+		startLatch.countDown();
+
+		// Wait for completion
+		doneLatch.await();
+		executor.shutdown();
+		executor.awaitTermination(30, TimeUnit.SECONDS);
+
+		// Report results
+		System.out.println("\n=== Test Results ===");
+		System.out.println("Total image data creations: " + handleCreationCount.get());
+		System.out.println("Creations by zoom level:");
+		handleCreationsByZoom.forEach((zoom, count) -> 
+			System.out.println("  Zoom " + zoom + ": " + count + " creation(s)"));
+
+		int creationsForTargetZoom = handleCreationsByZoom.getOrDefault(TARGET_ZOOM, 0);
+		if (creationsForTargetZoom > 1) {
+			System.err.println("\n❌ RACE CONDITION DETECTED!");
+			System.err.println("Image data was created " + creationsForTargetZoom + 
+				" times for zoom " + TARGET_ZOOM);
+			System.err.println("Expected: 1 creation");
+			System.err.println("This indicates that multiple threads created duplicate handles.");
+		} else if (creationsForTargetZoom == 1) {
+			System.out.println("\n✓ No race condition detected (or it didn't manifest in this run)");
+			System.out.println("Note: Race conditions are non-deterministic; absence of detection");
+			System.out.println("doesn't guarantee the code is thread-safe.");
+		} else {
+			System.out.println("\n⚠ Zoom level " + TARGET_ZOOM + " was never accessed");
+		}
+
+		image.dispose();
+		display.dispose();
+	}
+}

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

@@ -1183,5 +1183,122 @@ public void test_gcOnImageGcDrawer_imageDataAtNonDeviceZoom() {
 	}
 }
 
+/**
+ * Test for race condition fix in ImageHandleManager.
+ * This test verifies that concurrent access to Image.getImageData() at the same zoom level
+ * doesn't create duplicate handles or cause resource leaks.
+ * 
+ * The fix uses ConcurrentHashMap instead of HashMap to ensure thread-safe access.
+ */
+@Test
+public void test_ConcurrentImageHandleAccess() throws Exception {
+	final int NUM_THREADS = 10;
+	final int ITERATIONS_PER_THREAD = 50;
+	final int TARGET_ZOOM = 150;
+
+	// Track how many times image data is created for each zoom level
+	final AtomicInteger creationCount = new AtomicInteger(0);
+	final java.util.concurrent.ConcurrentHashMap<Integer, AtomicInteger> creationsByZoom = new java.util.concurrent.ConcurrentHashMap<>();
+
+	// Create an ImageDataProvider that tracks handle creations
+	ImageDataProvider testProvider = new ImageDataProvider() {
+		@Override
+		public ImageData getImageData(int zoom) {
+			// Track creation attempts
+			creationsByZoom.computeIfAbsent(zoom, k -> new AtomicInteger(0)).incrementAndGet();
+			creationCount.incrementAndGet();
+
+			// Create image data
+			int size = 16 * zoom / 100;
+			ImageData imageData = new ImageData(size, size, 24, 
+				new PaletteData(0xFF, 0xFF00, 0xFF0000));
+
+			// Fill with a simple pattern
+			for (int y = 0; y < size; y++) {
+				for (int x = 0; x < size; x++) {
+					imageData.setPixel(x, y, (x + y) % 2 == 0 ? 0xFFFFFF : 0x000000);
+				}
+			}
+
+			return imageData;
+		}
+	};
+
+	Image testImage = new Image(display, testProvider);
+
+	try {
+		// Force initial creation at 100% zoom to initialize the image
+		testImage.getImageData(100);
+
+		// Reset counters
+		creationsByZoom.clear();
+		creationCount.set(0);
+
+		// Create threads that will concurrently access the image
+		java.util.concurrent.ExecutorService executor = java.util.concurrent.Executors.newFixedThreadPool(NUM_THREADS);
+		java.util.concurrent.CountDownLatch startLatch = new java.util.concurrent.CountDownLatch(1);
+		java.util.concurrent.CountDownLatch doneLatch = new java.util.concurrent.CountDownLatch(NUM_THREADS);
+		final java.util.concurrent.atomic.AtomicReference<Throwable> errorRef = new java.util.concurrent.atomic.AtomicReference<>();
+
+		// Submit tasks that will all try to access the image at the same zoom level
+		for (int i = 0; i < NUM_THREADS; i++) {
+			executor.submit(() -> {
+				try {
+					// Wait for all threads to be ready
+					startLatch.await();
+
+					// Each thread tries to get the image data multiple times
+					for (int j = 0; j < ITERATIONS_PER_THREAD; j++) {
+						display.syncExec(() -> {
+							try {
+								// This should trigger getOrCreate() in ImageHandleManager
+								ImageData data = testImage.getImageData(TARGET_ZOOM);
+								assertNotNull(data, "ImageData should not be null");
+							} catch (Throwable e) {
+								errorRef.compareAndSet(null, e);
+							}
+						});
+					}
+				} catch (InterruptedException e) {
+					Thread.currentThread().interrupt();
+					errorRef.compareAndSet(null, e);
+				} finally {
+					doneLatch.countDown();
+				}
+			});
+		}
+
+		// Start all threads at once
+		startLatch.countDown();
+
+		// Wait for completion
+		assertTrue(doneLatch.await(30, java.util.concurrent.TimeUnit.SECONDS), 
+			"Test threads should complete within timeout");
+
+		executor.shutdown();
+		assertTrue(executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS),
+			"Executor should shutdown within timeout");
+
+		// Check if any errors occurred
+		Throwable error = errorRef.get();
+		if (error != null) {
+			throw new AssertionError("Error during concurrent access", error);
+		}
+
+		// Verify that image data was created exactly once for the target zoom
+		// (With the fix, ConcurrentHashMap.computeIfAbsent ensures single creation)
+		int creationsForTargetZoom = creationsByZoom.getOrDefault(TARGET_ZOOM, new AtomicInteger(0)).get();
+
+		// With the race condition fix, we expect exactly 1 creation for the target zoom
+		// Without the fix, multiple threads might create duplicate handles
+		assertEquals(1, creationsForTargetZoom,
+			"Image data should be created exactly once for zoom " + TARGET_ZOOM + 
+			" (race condition would cause multiple creations)");
+
+	} finally {
+		testImage.dispose();
+	}
+}
+
 }