Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Graphic is disposed" on Windows with pull/1496 #1639

Open
tobiasmelcher opened this issue Dec 9, 2024 · 7 comments · Fixed by #1643 · May be fixed by #1782
Open

"Graphic is disposed" on Windows with pull/1496 #1639

tobiasmelcher opened this issue Dec 9, 2024 · 7 comments · Fixed by #1643 · May be fixed by #1782

Comments

@tobiasmelcher
Copy link
Contributor

#1496 introduces an incompatibility which breaks existing implementations.

Following pattern is used where the base image is disposed directly after creating an instance of DecorationOverlayIcon.

Image tempImage = descriptor.createImage();

ImageDescriptor[] overlaysArray = new ImageDescriptor[6];
overlaysArray[IDecoration.BOTTOM_LEFT] = getDecorator(stateDecoratorLocation);
overlaysArray[IDecoration.TOP_RIGHT] = getDecorator(testPoolDecoratorLocation);

image = new DecorationOverlayIcon(tempImage, overlaysArray).createImage();
tempImage.dispose();  // <!--------------------------
this.imageCache.put(imageId, image);

This produces now a "Graphic is disposed" exception on Windows with following stack:

org.eclipse.swt.SWTException: Graphic is disposed
              at org.eclipse.swt.SWT.error(SWT.java:4922)
              at org.eclipse.swt.SWT.error(SWT.java:4837)
              at org.eclipse.swt.SWT.error(SWT.java:4808)
              at org.eclipse.swt.graphics.Image.getImageData(Image.java:1380)
              at org.eclipse.jface.resource.CompositeImageDescriptor$CachedImageImageDataProvider.getImageData(CompositeImageDescriptor.java:114)
              at org.eclipse.jface.resource.CompositeImageDescriptor.getZoomedImageData(CompositeImageDescriptor.java:457)
              at org.eclipse.jface.resource.CompositeImageDescriptor.drawImage(CompositeImageDescriptor.java:267)
              at org.eclipse.jface.viewers.DecorationOverlayIcon.drawCompositeImage(DecorationOverlayIcon.java:217)
              at org.eclipse.jface.resource.CompositeImageDescriptor.getImageData(CompositeImageDescriptor.java:375)
              at org.eclipse.swt.internal.DPIUtil.lambda$2(DPIUtil.java:543)
              at org.eclipse.swt.internal.DPIUtil.getElementAtZoom(DPIUtil.java:572)
              at org.eclipse.swt.internal.DPIUtil.validateAndGetImageDataAtZoom(DPIUtil.java:543)
              at org.eclipse.swt.graphics.Image.getImageMetadata(Image.java:768)
              at org.eclipse.swt.graphics.Image.getBounds(Image.java:1306)
              at org.eclipse.swt.graphics.Image.getBounds(Image.java:1301)
              at org.eclipse.swt.internal.ImageList.add(ImageList.java:61)
              at org.eclipse.swt.widgets.Button._setImage(Button.java:144)
              at org.eclipse.swt.widgets.Button.setImage(Button.java:875)

Could you please improve the enhancements of #1496 so that this exception does no longer occur?

Thanks and best regards,
Tobias

@tobiasmelcher tobiasmelcher changed the title "Graphic is disposed" on Windwos with pull/1496 "Graphic is disposed" on Windows with pull/1496 Dec 9, 2024
@HeikoKlare
Copy link
Contributor

Thank you for reporting the issue, Tobias!

fyi @amartya4256 @akoch-yatta @fedejeanne

@akoch-yatta
Copy link
Contributor

@tobiasmelcher thx for reporting that issue. I had a look into it, but I cannot reproduce a new issue with that. I took a simpler variant of your snippet:

public class Test {
	public static void main(String[] args) {
		Image tempImage = new Image(new Display(), 20, 20); 
		ImageDescriptor[] overlaysArray = new ImageDescriptor[6];

		Image image = new DecorationOverlayIcon(tempImage, overlaysArray).createImage();
		tempImage.dispose(); 
		image.getImageData(200);
		image.getImageData(100);
	}
}

With that I get the similar exception even in the 2022-12 release (just picked any old release).
The disposal check in Image::getImageData is quite old and the tempImage (in the snippet) is the exact same instance of the Image that is used in CompositeImageDescriptor.java:114 (wrapped into an ImageDataProvider). This indicates to me, that this image must not be disposed. Otherwise this exception will occur as soon as a second variant of the image must be created.

Perhaps my simplified snippet behaves different as in your use case, so do you have a complete reproducer for the that worked before in the same setup and setting? Please give me your full monitor setup then. My current assumption is, that your setup could cause that and the default cached variant of the ImageData is different than before, so the second variant shall be created.

Nevertheless, I didn't have a too deep look into the jface implementation, but it looks to me that disposing the tempImage will lead to issues when two different variants of that icon are requested based on the implementation in CachedImageImageDataProvider -> e.g. with two monitors (100 & 200) and the experimental rescaling at runtime set to true.

@tobiasmelcher
Copy link
Contributor Author

tobiasmelcher commented Dec 9, 2024

@akoch-yatta, here the snippet to reproduce the problem

public class Snippet {
	public static void main(String[] args) {
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setText("Test");
		shell.setSize(300, 200);

		Image tempImage = new Image(display, 20, 20);
		ImageDescriptor[] overlaysArray = new ImageDescriptor[6];

		Image image = new DecorationOverlayIcon(tempImage, overlaysArray).createImage();
		tempImage.dispose();

		Button button = new Button(shell, SWT.PUSH);
		button.setImage(image);

		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}
		image.dispose();
		display.dispose();
	}
}

The zoom value on my machine is 200.
I see following difference:

  • before pull request Refactor Image for storing ImageMetadata #1496 CompositeImageDescriptor$CachedImageImageDataProvider.getImageData(int) line: 111 is called only once with zoom level value 200.
  • on the current master branch CompositeImageDescriptor$CachedImageImageDataProvider.getImageData(int) line: 111 is called twice. First call with zoom level value 200, second call with value 100 and then the Exception is raised.
    Here the stack from the second call:
Thread [main] (Suspended (breakpoint at line 111 in CompositeImageDescriptor$CachedImageImageDataProvider))	
	CompositeImageDescriptor$CachedImageImageDataProvider.getImageData(int) line: 111	
	DecorationOverlayIcon(CompositeImageDescriptor).getZoomedImageData(ImageDataProvider) line: 457	
	DecorationOverlayIcon(CompositeImageDescriptor).drawImage(ImageDataProvider, int, int) line: 267	
	DecorationOverlayIcon.drawCompositeImage(int, int) line: 217	
	DecorationOverlayIcon(CompositeImageDescriptor).getImageData(int) line: 375	
	0x00000222a802bc10.getImageData(int) line: not available	
	DPIUtil.lambda$2(ImageDataProvider, Integer) line: 543	
	0x00000222a8029c00.apply(Object) line: not available	
	DPIUtil.getElementAtZoom(Function<Integer,T>, int) line: 572	
	DPIUtil.validateAndGetImageDataAtZoom(ImageDataProvider, int) line: 543	
	Image.getImageMetadata(int) line: 768	
	Image.getBounds(int) line: 1306	
	Image.getBounds() line: 1301	
	ImageList.add(Image) line: 61	
	Button._setImage(Image) line: 144	
	Button.setImage(Image) line: 875	
	Snippet.main(String[]) line: 25	

akoch-yatta added a commit to akoch-yatta/eclipse.platform.swt that referenced this issue Dec 10, 2024
This commit adapts the bounds calculation in Image to not create any handle, but use and scale the bounds of an existing handle if no handle for the desired zoom is available

Fixes eclipse-platform#1639
@akoch-yatta
Copy link
Contributor

@tobiasmelcher Thx for the Snippet and the further analysis. The issue was that getBounds created new handles on demand instead of scaling existing ones. I created a PR for that.

I want to emphasize, that the underlyind issue remains. The implementation of CachedImageImageDataProvider relies on the baseImage (=tempImage) not being disposed. The exception will reappear if a second variant of the image is requested (see my Snippet). Could you open an issue for jface with your usecase? Perhaps it is intended, but if not the implementation could be changed to no rely on the passed image not being disposed.

@tobiasmelcher
Copy link
Contributor Author

tobiasmelcher commented Jan 30, 2025

Hi colleagues,

the problem raised again. I am again getting the "Graphic is disposed" exception. Could you please fix again?

Thanks a lot,
Tobias

org.eclipse.swt.SWTException: Graphic is disposed
	at org.eclipse.swt.SWT.error(SWT.java:4932)
	at org.eclipse.swt.SWT.error(SWT.java:4847)
	at org.eclipse.swt.SWT.error(SWT.java:4818)
	at org.eclipse.swt.graphics.Image.getImageData(Image.java:1400)
	at org.eclipse.jface.resource.CompositeImageDescriptor$CachedImageImageDataProvider.getImageData(CompositeImageDescriptor.java:114)
	at org.eclipse.jface.resource.CompositeImageDescriptor.getZoomedImageData(CompositeImageDescriptor.java:457)
	at org.eclipse.jface.resource.CompositeImageDescriptor.drawImage(CompositeImageDescriptor.java:267)
	at org.eclipse.jface.viewers.DecorationOverlayIcon.drawCompositeImage(DecorationOverlayIcon.java:217)
	at org.eclipse.jface.resource.CompositeImageDescriptor.getImageData(CompositeImageDescriptor.java:375)
	at org.eclipse.swt.internal.DPIUtil.lambda$2(DPIUtil.java:538)
	at org.eclipse.swt.internal.DPIUtil.getElementAtZoom(DPIUtil.java:567)
	at org.eclipse.swt.internal.DPIUtil.validateAndGetImageDataAtZoom(DPIUtil.java:538)
	at org.eclipse.swt.graphics.Image$ImageDataProviderWrapper.getBounds(Image.java:2188)
	at org.eclipse.swt.graphics.Image.getBounds(Image.java:1322)
	at org.eclipse.swt.graphics.Image.getBounds(Image.java:1312)
	at org.eclipse.swt.widgets.Button.computeSizeInPixels(Button.java:361)

@tobiasmelcher tobiasmelcher reopened this Jan 30, 2025
@HeikoKlare
Copy link
Contributor

Thank you, @tobiasmelcher!

I can reproduce the issue at 200% with your snippet (#1639 (comment)). We will have a look.

akoch-yatta added a commit to vi-eclipse/eclipse.platform.swt that referenced this issue Jan 31, 2025
This commit unifies the getBounds calculation for ImageDataProvider and
ImageFileNameProvider by always scaling the bounds of an existing handle
of an image, if the bounds for a different zoom are requested.

fixes eclipse-platform#1639
@akoch-yatta
Copy link
Contributor

akoch-yatta commented Jan 31, 2025

This effect was re-introduced with c493ae8. I will adapt the causing place to not call getImageData anymore.

But I just want to raise the awareness, that the underlying issue remains. The implementation of CachedImageImageDataProvider relies on the baseImage (=tempImage) not being disposed. The exception will reappear if a second variant of the image is requested (see my Snippet).
We are currently discussing how to adapt the creation of handles in the win32 implementation and plan to create all handles for ImageDataProvider and ImageFileNameProvider fully on demand. I am pretty sure, that would lead in the same exception again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants