Broken resource handling with operation pattern in GC

[HeikoKlare]

Description
With the following PRs, an operation pattern was introduced in GC:

• [win32] Apply Operation pattern on GC eclipse-platform/eclipse.platform.swt#2243
• [win32] Refresh GC handle when related image  eclipse-platform/eclipse.platform.swt#2263

With the introduction of an operation pattern in GC, resources are stored or even created inside those operations and are potentially accessed after they have already been disposed or are not disposed at all:

• Resources that are passed to the GC and then stored in an operation may be disposed outside afterwards, such that they are disposed when using them for creating the GC handle for another zoom later on, leading to an exception. This affects at least:
  • CopyAreaToImageOperation
  • DrawImageOperation
  • DrawScalingImageToImageOperation
  • DrawImageToImageOperation
• Resources that are created and stored inside the operations are not disposed at all. They introduce resources leaks
  • SetBackgroundPatternOperation
  • SetForegroundPatternOperation
  • SetTransformOperation

Reproduction
The disposed exceptions can be easily reproduced with a slight extension of the snippet in:

• [win32] Refresh GC handle when related image  eclipse-platform/eclipse.platform.swt#2263

Using one of the methods to pass a resource and the disposing it right afterwards used to work with the GC but breaks with the changed implementation. An example for the drawImage() method/operation is given here:

public static void main (String [] args) {
	Display display = new Display ();
	Shell shell = new Shell (display);
	shell.setText("GC Redraw");
	shell.setLayout(new FillLayout());
	int width = 150, height = 200;
	Image image = new Image (display, width, height);
	Image imageToDraw = new Image(display, 1, 1);

	GCData data = new GCData();
	GC gc = GC.win32_new(image, data);
	gc.setBackground(display.getSystemColor(SWT.COLOR_RED));
	gc.fillRectangle (0, 0, width, height);
	gc.drawLine (0, 0, width, height);
	gc.drawLine (0, height, width, 0);
	gc.drawText ("Default Image", 10, 10);
	gc.drawImage(imageToDraw, 0, 0);
	imageToDraw.dispose();
	final Point origin = new Point (0, 0);
	final Canvas canvas = new Canvas (shell, SWT.NONE);
	canvas.addListener (SWT.Paint, e -> {
		GC gc2 = e.gc;
		gc2.getGCData().nativeZoom = 150;
		gc2.drawImage (image, origin.x, origin.y);
		gc2.getGCData().nativeZoom = 200;
		gc2.drawImage (image, origin.x + 300, origin.y);
	});
	shell.setSize (1000, 1000);
	shell.open ();
	while (!shell.isDisposed ()) {
		if (!display.readAndDispatch ()) display.sleep ();
	}
	gc.dispose ();
	image.dispose();
	display.dispose ();
}

This yields:

Exception in thread "main" java.lang.IllegalArgumentException: Argument not valid
	at org.eclipse.swt.SWT.error(SWT.java:4927)
	at org.eclipse.swt.SWT.error(SWT.java:4861)
	at org.eclipse.swt.SWT.error(SWT.java:4832)
	at org.eclipse.swt.graphics.GC$DrawImageOperation.drawImageInPixels(GC.java:1031)
	at org.eclipse.swt.graphics.GC$DrawImageOperation.apply(GC.java:1027)
	at org.eclipse.swt.graphics.GC.createGcHandle(GC.java:5875)
	at org.eclipse.swt.graphics.GC.refreshFor(GC.java:5763)

The non-disposed resources is something I sporadically see in an SDK if according methods are used. E.g.:

java.lang.Error: SWT Resource was not properly disposed
	at org.eclipse.swt.graphics.Resource.initNonDisposeTracking(Resource.java:191)
	at org.eclipse.swt.graphics.Resource.<init>(Resource.java:124)
	at org.eclipse.swt.graphics.Pattern.<init>(Pattern.java:170)
	at org.eclipse.swt.graphics.Pattern.copy(Pattern.java:226)
	at org.eclipse.swt.graphics.GC$SetBackgroundPatternOperation.<init>(GC.java:4678)
	at org.eclipse.swt.graphics.GC.setBackgroundPattern(GC.java:4671)
	at org.eclipse.e4.ui.workbench.renderers.swt.CTabRendering.drawSelectedTab(CTabRendering.java:558)

Expected Behavior
Resources shall be properly handled inside GC, i.e.:

• They must not be used after disposal. Probably a copy of them needs to be stored instead.
• They must not be leaked, i.e., all resources created inside GC must be disposed later on.

Necessary configuration:
n/a

Additional knowledge
We had similar issues inside other resources, such as the Region where other Region objects are passed via methods (and then need to be represented in operations). There, we stored their operations instead of the Region itself (see Region#OperationWithRegion as an example). In GC we now use copy() operations of the original resources instead of storing the operations, which leads to the requirement of the explicit disposal. I am not sure why that was not done in a uniform way. But that is of course something that cannot be applied to images.

[arunjose696]

As the resource in GC mostly not disposed are Images. One feasible way to create image copies is using:

this.image = new Image(image.device, image, SWT.IMAGE_COPY);

This makes a deep copy, which might use more memory. If that’s acceptable, all operation constructors could use this to store copies of images instead of the original Image object. Is there a better way to create and store copies of Images in operations?

To resolve non disposed resources adding dispose() or disposeAll() method in the GC.Operation class could help. This method would go through all resource fields and dispose of them properly. Operations that need special disposal can override this method.

[arunjose696]

This method would go through all resource fields and dispose of them properly.

For this, we could either:

1)Use reflection to check if member variables are instances of Resource and dispose of them at end of apply(), or
2)Keep a list of disposables (e.g., List) and dispose them when apply() ends.

[arunjose696]

An issue with creating a copy of an image in the constructor of operation is that, if something is drawn on the image using a GC and then we create a copy of the image before disposing the GC, the copy doesn't reflect the drawing done by GC.

To investigate, I created a reproducible snippet and posted it in this Stack Overflow question. One user mentioned that this behavior doesn't occur on macOS, Is this related to how GC behaves on Windows possibly due to buffering/lazy loading. Is there a way to flush the drawing operations to the image before copying it?

[fedejeanne]

IIRC we agreed to report regressions on the official repo?