Bug 573473 - [GTK3] ImageLoader.getImageFormat(long) leaks native memory
Summary: [GTK3] ImageLoader.getImageFormat(long) leaks native memory
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.13   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.20 M3   Edit
Assignee: Simeon Andreev CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-11 04:45 EDT by Simeon Andreev CLA
Modified: 2021-05-17 07:17 EDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simeon Andreev CLA 2021-05-11 04:45:48 EDT
To reproduce, run snippet with VM arguments "-Xmx128M -Xms128M -XX:+AlwaysPreTouch", path to image file must be adjusted:

	public static void main(String[] args) {
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setSize(600, 400);
		shell.setLayout(new FillLayout());
		shell.setText("Demo ImageLoader leak");
		Composite composite = new Composite(shell, SWT.NONE);
		composite.setLayout(new FillLayout(SWT.VERTICAL));

		shell.open();

		AtomicBoolean c = new AtomicBoolean(true);
		Thread t = new Thread(new Runnable() {
			@Override
				public void run() {
					while (c.get()) {
						if (!display.isDisposed()) {
							display.asyncExec(new Runnable() {
								@Override
								public void run() {
									if (!display.isDisposed()) {
										Image image = new Image(display, "/home/sandreev/git/eclipse/eclipse.platform.ui/bundles/org.eclipse.jface/bin/org/eclipse/jface/fieldassist/images/contassist_ovr.png");
										image.dispose();
									}
								}
							});
						}
						try {
							Thread.sleep(100);
						} catch (InterruptedException e) {
						}
					}
				}	
		});
		t.start();

		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		c.set(false);
		try {
			t.join();
		} catch (InterruptedException e) {
		}
		display.dispose();
	}

Observe growing memory consumption. valgrind reports the leak e.g. like this:

==14341== 8,840 bytes in 2,210 blocks are definitely lost in loss record 88 of 15,501
==14341==    at 0x4C2B017: malloc (vg_replace_malloc.c:380)
==14341==    by 0x2A31F68D: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.1)
==14341==    by 0x2A33869E: g_strdup (in /usr/lib64/libglib-2.0.so.0.5600.1)
==14341==    by 0x269DBA0F: Java_org_eclipse_swt_internal_gtk_GDK_gdk_1pixbuf_1format_1get_1name (in /home/sandreev/git/eclipse/eclipse.platform.swt.binaries/bundles/org.eclipse.swt.gtk.linux.x86_64/libswt-pi3-gtk-4944r22.so)

The code is:

int getImageFormat(long loader) {
	long format = GDK.gdk_pixbuf_loader_get_format(loader);
	long name = GDK.gdk_pixbuf_format_get_name(format);
	String nameStr = Converter.cCharPtrToJavaString(name, false);
	switch (nameStr) {
		case "bmp": return SWT.IMAGE_BMP;
		case "gif": return SWT.IMAGE_GIF;
		case "ico": return SWT.IMAGE_ICO;
		case "jpeg": return SWT.IMAGE_JPEG;
		case "png": return SWT.IMAGE_PNG;
		case "svg": return SWT.IMAGE_SVG;
		default: return SWT.IMAGE_UNDEFINED;
	}
}

Its missing an OS.g_free() call for the result of GDK.gdk_pixbuf_format_get_name().

Comment from Alexander Miloslavskiy:

> GTK sources are pretty clear [2]: g_strdup() makes a copy and
> returns to the caller, therefore, nobody except the caller can clean it up.
>
> Also, if you check GTK sources, you'll see this:
>
> 'gtk\gtkselection.c'
>         Line 464
>                 name = gdk_pixbuf_format_get_name (fmt);
>                 <...>
>                 g_free (name);
>
>
> Here, GTK itself uses 'g_free()' to free the memory.
Comment 1 Eclipse Genie CLA 2021-05-12 08:00:17 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/180519
Comment 3 Simeon Andreev CLA 2021-05-17 05:19:23 EDT
Should be introduced with bug 545032, commit: https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=0b22592e3f6c547b0bec5a5b491bd41a583e8e60 (there are a lot of commits on the bug though, and some reverts, might not be the exact commit; the memory leak should still be in 4.13 and onward though)