Bug 574065 - [GTK3] Tree leaks native memory in gtk_tree_view_expand_row()
Summary: [GTK3] Tree leaks native memory in gtk_tree_view_expand_row()
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.15   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-SWT-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-08 03:15 EDT by Simeon Andreev CLA
Modified: 2021-11-08 17:57 EST (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-06-08 03:15:28 EDT
To reproduce:

1. Run Eclipse in a new workspace, with jemalloc.
2. Create an empty Java project.
3. Add snippet:
package test;

import java.util.concurrent.Executors;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.atomic.AtomicBoolean;

public class Test {

	public static void main(String[] args) {
        ThreadPoolExecutor executor = (ThreadPoolExecutor) 
        		Executors.newFixedThreadPool(8);
        executor.prestartAllCoreThreads();
        
        AtomicBoolean c = new AtomicBoolean(true);
        for (int i = 0; i < 3; ++i) {
        	final int j = i;
        	Thread t = new Thread(new Runnable() {
        			@Override
        			public void run() {
        				while (c.get()) {
        					System.out.println("hello from thread " + j);
        					try {
								Thread.sleep(5_000);
							} catch (InterruptedException e) {
								//
							}
        				}
        			}
        	});
        	t.start();
        }
        
        for (int i = 1; i <= 5; i++) 
        {
        	final int j = i;
            Runnable task = new Runnable() {
            	@Override
            	public void run() {
            		if (j == 3) {
            			System.out.println("hello world");
            		}
            		System.out.println(j);	
            	}
            };
 
            executor.execute(task);
        }
        c.set(false);
        executor.shutdown();
	}

}
4. Set breakpoint at line 40 (the line is "System.out.println("hello world");", its the last println() of the snippet).
5. Debug the snippet, don't switch to debug perspective when the dialog asks you to.
6. Terminate the snippet while still at the breakpoint set in step 4..
7. Observe jemalloc memory leak report:

[144 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
Java_org_eclipse_swt_internal_gtk_GTK_gtk_1tree_1view_1expand_1row (??:?)
?? (??:0)

Repeating debug and terminate (make sure the Debug view remains visible, e.g. by toggling off the system console pop-up on output), results in a bigger memory leak reported by jemalloc:

[720 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
Java_org_eclipse_swt_internal_gtk_GTK_gtk_1tree_1view_1expand_1row (??:?)
?? (??:0)

In our product (based on Eclipse 4.15), looping debugging, terminating and perspective switches over about 12 hours, we see ~1MB memory leaked:

[1097712 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
Java_org_eclipse_swt_internal_gtk_GTK_gtk_1tree_1view_1expand_1row (??:?)
0x00007fffe2a9251f (??:0)
Comment 1 Simeon Andreev CLA 2021-06-08 06:15:56 EDT
For the SWT snippet, it seems expanding and selecting elements is not enough. But after adding a remove element call, I can reproduce the leak:

	public static void main(String[] args) {
		int n = 1;
		for (int j = 0; j < n; ++j) {
			Display display = new Display();
			Shell shell = new Shell(display);
			shell.setText("Tree Example");
			shell.setLayout(new FillLayout());
			
			int COUNT = 50;
			final String [] itemStrings = new String [COUNT];
			for (int i = 0; i < COUNT; i++) {
				itemStrings [i] = "item " + i;
			}
			AtomicInteger c = new AtomicInteger(0);
			final Tree tree = new Tree(shell, SWT.MULTI | SWT.H_SCROLL | SWT.V_SCROLL | SWT.VIRTUAL);
			tree.addListener(SWT.SetData, new Listener() {
				public void handleEvent(Event event) {
					TreeItem item = (TreeItem)event.item;
					int index = event.index;
					item.setText(itemStrings [index]);
					System.out.println("setdata call " + c.incrementAndGet());
				}
			});
			tree.setItemCount(COUNT);
			TreeItem item0 = tree.getItem(0);
			item0.setItemCount(10);
			TreeItem item1= tree.getItem(1);
			item1.setItemCount(10);
			TreeItem item15 = item1.getItem(5);
			item15.setItemCount(5);
			shell.open();
			
			while (display.readAndDispatch()) {
				// process UI events
			}
			tree.setSelection(item15);
			while (display.readAndDispatch()) {
				// process UI events
			}
			item15.setExpanded(true);
			while (display.readAndDispatch()) {
				// process UI events
			}
			item0.setExpanded(true);
			while (display.readAndDispatch()) {
				// process UI events
			}
			tree.setItemCount(1);
			while (display.readAndDispatch()) {
				// process UI events
			}
			tree.setItemCount(1);
			tree.setSelection(item0);
			
			while (!shell.isDisposed()) {
				if (!display.readAndDispatch())
					display.sleep();
			}
			display.dispose();
		}
	}

I'm guessing some of the operations here are not required to reproduce the leak, I've not tried to reduce the snippet more than this.

n = 1:

[624 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
Java_org_eclipse_swt_internal_gtk_GTK_gtk_1tree_1view_1expand_1row (??:?)
?? (??:0)

n = 5:

[3120 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
Java_org_eclipse_swt_internal_gtk_GTK_gtk_1tree_1view_1expand_1row (??:?)
?? (??:0)
Comment 2 Simeon Andreev CLA 2021-06-08 07:41:18 EDT
Simplified snippet:

	public static void main(String[] args) {
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setText("Tree Example");
		shell.setLayout(new FillLayout());

		final Tree tree = new Tree(shell, SWT.NONE);
		tree.setItemCount(2);
		TreeItem item0 = tree.getItem(0);
		item0.setText("item0");
		TreeItem item1 = tree.getItem(1);
		item1.setText("item1");
		item1.setItemCount(6);
		TreeItem[] children = item1.getItems();
		for (int k = 0; k < children.length; ++k) {
			children[k].setText("child" + k);
		}
		shell.open();

		while (display.readAndDispatch()) {
			// process UI events
		}
		item1.setExpanded(true);
		while (display.readAndDispatch()) {
			// process UI events
		}
		tree.setItemCount(1);

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

I'll try writing a GTK+ snippet next. I don't see undispoed TreeItem objects after display.dispose().
Comment 3 Simeon Andreev CLA 2021-06-08 08:36:39 EDT
I can reproduce the leak with GTK+ snippet:

// gcc -g tree.c `pkg-config --cflags --libs gtk+-3.0` -o tree

#include <gtk/gtk.h>

enum
{
  COL1 = 0,
  NUM_COLS
} ;

int
main (int argc, char **argv)
{
  GtkWidget *window;
  GtkWidget *scrolled_window;
  GtkWidget *view;

  gtk_init (&argc, &argv);

  window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
  g_signal_connect (window, "delete_event", gtk_main_quit, NULL);

  gtk_window_set_default_size(GTK_WINDOW(window), 200, 200);

  scrolled_window = gtk_scrolled_window_new(NULL, NULL);

  GtkCellRenderer     *renderer;

  view = gtk_tree_view_new ();

  renderer = gtk_cell_renderer_text_new ();
  gtk_tree_view_insert_column_with_attributes (GTK_TREE_VIEW (view), -1, "column 1", renderer, "text", COL1, NULL);

  GtkTreeStore *treestore;
  GtkTreeIter   iter, child;

  treestore = gtk_tree_store_new(NUM_COLS, G_TYPE_STRING);

  gtk_tree_store_append(treestore, &iter, NULL);
  gtk_tree_store_set(treestore, &iter, COL1, "item", -1);
  gtk_tree_store_append(treestore, &iter, NULL);
  gtk_tree_store_set(treestore, &iter, COL1, "item", -1);
  GtkTreePath *path = NULL;
  path = gtk_tree_model_get_path (GTK_TREE_MODEL(treestore), &iter);
  int j;
  for (j = 0; j < 6; ++j)
  {
    gtk_tree_store_append(treestore, &child, &iter);
    gtk_tree_store_set(treestore, &child, COL1, "child", -1);
  }

  gtk_tree_view_set_model (GTK_TREE_VIEW (view), GTK_TREE_MODEL (treestore));

  if (path)
  {
    gtk_tree_view_expand_row(GTK_TREE_VIEW(view), path, FALSE);
    gtk_tree_path_free(path);
  }

  gtk_container_add (GTK_CONTAINER (window), scrolled_window);
  gtk_container_add (GTK_CONTAINER (scrolled_window), view);

  gtk_widget_show_all (window);

  GtkTreeIter iter1;
  if (gtk_tree_model_iter_nth_child(GTK_TREE_MODEL(treestore), &iter1, NULL, 1))
  {
    gtk_tree_store_remove(treestore, &iter1);
  }

  g_object_unref(treestore);

  gtk_main ();

  return 0;
}

For 6 children of the 2nd item (see loop in the snippet), jemalloc reports the leak as:

[240 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
?? (??:0)
__libc_start_main (/usr/src/debug/glibc-2.17-c758a686/csu/../csu/libc-start.c:274)
?? (??:0)

FOr 18 children, the leak is reported as:

[816 bytes leaked]
je_prof_backtrace (/home/sandreev/git/misc/jemalloc/src/prof.c:636 (discriminator 2))
je_malloc_default (/home/sandreev/git/misc/jemalloc/src/jemalloc.c:2289)
g_malloc (??:?)
g_slice_alloc (??:?)
_gtk_rbnode_new.isra.2 (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:61)
_gtk_rbtree_insert_after (/usr/src/debug/gtk+-3.22.30/gtk/gtkrbtree.c:456)
gtk_tree_view_build_tree (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:9570)
gtk_tree_view_real_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12865)
gtk_tree_view_expand_row (/usr/src/debug/gtk+-3.22.30/gtk/gtktreeview.c:12920)
?? (??:0)
__libc_start_main (/usr/src/debug/glibc-2.17-c758a686/csu/../csu/libc-start.c:274)
?? (??:0)

It would be good to know what GTK+ API expects when removing a tree element. Is the client supposed to remove all children of that element beforehand, so that they are freed? Or this is an actual leak in GTK+?

I grepped the GTK+ repository for gtk_tree_store_remove(), the only example I find is in: gtk/inspector/object-tree.c

Unfortunately I'm not familiar enough with GTK+ code to know whether this code is removing children of an element, before removing the element itself.
Comment 4 Simeon Andreev CLA 2021-06-08 09:24:56 EDT
According to this, the GTK+ snippet and SWT seem to be doing what client code is supposed to do:

https://en.wikibooks.org/wiki/GTK%2B_By_Example/Tree_View/Tree_Models#Removing_Rows

> Rows can easily be removed with gtk_list_store_remove and gtk_tree_store_remove.
> The removed row will automatically be removed from the tree view as well,
> and all data stored will automatically be freed,
> with the exception of G_TYPE_POINTER columns (see above).

I'm not sure how valid this is as a source though. The docu itself makes no mention about removing children first:

https://developer.gnome.org/gtk3/stable/GtkTreeStore.html#gtk-tree-store-remove

Calling gtk_tree_store_clear() (SWT Tree.removeAll() calls this), instead of removing rows one by 1, results in no memory leak. But calling gtk_tree_store_clear() after removing the 2nd row (index 1), the leak is seen.

Alexander, any idea about this?
Comment 5 Simeon Andreev CLA 2021-06-08 09:46:40 EDT
Collapsing the row prior to removing it results in no memory leak with the GTK+ snippet from comment 3.

Same if I do this in SWT:

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java
index 5bace6a763..f295791c1e 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java 
+++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java 
@@ -1257,10 +1257,18 @@ void destroyItem (TreeColumn column) {
        }
 }
 
-
+boolean ignoreCollapseCall = false;
 void destroyItem (TreeItem item) {
        long selection = GTK.gtk_tree_view_get_selection (handle);
        OS.g_signal_handlers_block_matched (selection, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
+       // bug 574065: prevent memory leak
+       long path = GTK.gtk_tree_model_get_path (modelHandle, item.handle);
+       if (GTK.gtk_tree_view_row_expanded (handle, path)) {
+               ignoreCollapseCall = true;
+               GTK.gtk_tree_view_collapse_row(handle, path);
+               ignoreCollapseCall = false;
+       }
+       GTK.gtk_tree_path_free (path);
        GTK.gtk_tree_store_remove (modelHandle, item.handle);
        OS.g_signal_handlers_unblock_matched (selection, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
        modelChanged = true;
@@ -2588,6 +2596,7 @@ long gtk_start_interactive_search(long widget) {
 
 @Override
 long gtk_test_collapse_row (long tree, long iter, long path) {
+       if (ignoreCollapseCall) return 0;
        int [] index = new int [1];
        GTK.gtk_tree_model_get (modelHandle, iter, ID_COLUMN, index, -1);
        TreeItem item = items [index [0]];

I don't think this is an option for SWT though, I'm guessing there are many unwanted side effects.
Comment 6 Andrey Loskutov CLA 2021-06-08 09:52:57 EDT
(In reply to Simeon Andreev from comment #5)
> Collapsing the row prior to removing it results in no memory leak with the
> GTK+ snippet from comment 3.

At this point I would recommend to create a RH ticket & pass this issue to the GTK team.

> Same if I do this in SWT:
> 
> I don't think this is an option for SWT though, I'm guessing there are many
> unwanted side effects.

Yep, that could probably be too much, I assume there could be listeners attached that could try to do something, also extra event processing overhead etc.
Comment 7 Simeon Andreev CLA 2021-06-08 10:42:55 EDT
(In reply to Andrey Loskutov from comment #6)
> (In reply to Simeon Andreev from comment #5)
> > Collapsing the row prior to removing it results in no memory leak with the
> > GTK+ snippet from comment 3.
> 
> At this point I would recommend to create a RH ticket & pass this issue to
> the GTK team.

Opened RHEL bugzilla ticket: https://bugzilla.redhat.com/show_bug.cgi?id=1969533
And RHEL support case: https://access.redhat.com/support/cases/#/case/02959748
Comment 8 Alexandr Miloslavskiy CLA 2021-11-08 17:57:43 EST
> Alexander, any idea about this?

Sorry, somehow I didn't notice the request earlier.

I have investigated and found it to be a GTK bug:
https://gitlab.gnome.org/GNOME/gtk/-/issues/4425

A possible workaround is to recursively delete items in 'Tree.releaseItems()' instead of hoping that GTK will do that properly. The downside is that it's going to be slower due to GTK overhead for deleting items.