Community
Participate
Working Groups
To reproduce, run the following snippet (close the shell after its up, it will be re-opened again): public static void main(String[] args) { for (int i = 0; i < 5; ++i) { Display display = new Display(); Shell shell = new Shell(display); shell.setText("Menu Example"); shell.setSize(300, 200); Label label = new Label(shell, SWT.CENTER); label.setBounds(shell.getClientArea()); Menu menuBar = new Menu(shell, SWT.BAR); MenuItem fileMenuHeader = new MenuItem(menuBar, SWT.CASCADE); fileMenuHeader.setText("&File"); shell.setMenuBar(menuBar); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } } jemalloc reports the leak as: [320 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 (??:?) g_slice_alloc0 (??:?) g_type_create_instance (??:?) g_object_unref (??:?) g_object_new_with_properties (??:?) g_object_new (??:?) Java_org_eclipse_swt_internal_gtk_GTK_gtk_1accel_1group_1new (??:?) ?? (??:0) For only 1 execution (change the for loop to iterate only once), the leak is reported as: [64 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 (??:?) g_slice_alloc0 (??:?) g_type_create_instance (??:?) g_object_unref (??:?) g_object_new_with_properties (??:?) g_object_new (??:?) Java_org_eclipse_swt_internal_gtk_GTK_gtk_1accel_1group_1new (??:?) ?? (??:0)
There seems to be a commented out free call, already in the code (its been there at least since 2009): diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Decorations.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Decorations.java index 24d7070242..d95d7d8062 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Decorations.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Decorations.java @@ -276,7 +276,7 @@ void destroyAccelGroup () { long shellHandle = topHandle (); GTK3.gtk_window_remove_accel_group (shellHandle, accelGroup); //TEMPORARY CODE -// OS.g_object_unref (accelGroup); + OS.g_object_unref (accelGroup); accelGroup = 0; } If I comment the code in, the leak is gone from the snippet in the description. However, trying to debug a snippet with Eclipse results in a JVM crash: Java frames: (J=compiled Java code, j=interpreted, Vv=VM code) j org.eclipse.swt.internal.gtk3.GTK3.gtk_widget_remove_accelerator(JJII)V+0 J 6493 c1 org.eclipse.swt.widgets.MenuItem.updateAccelerator(JZ)V (189 bytes) @ 0x00007fffda06bbcc [0x00007fffda06b640+0x000000000000058c] j org.eclipse.swt.widgets.MenuItem.removeAccelerator(J)V+3 J 9987 c2 org.eclipse.swt.widgets.MenuItem.setAccelerator(I)V (88 bytes) @ 0x00007fffe04df6b8 [0x00007fffe04df600+0x00000000000000b8] J 11549 c1 org.eclipse.jface.action.ActionContributionItem.updateMenuItemText(Lorg/eclipse/swt/widgets/MenuItem;)V (427 bytes) @ 0x00007fffd95c80dc [0x00007fffd95c5a20+0x00000000000026bc] J 11515 c1 org.eclipse.jface.action.ActionContributionItem.updateMenuItem(Lorg/eclipse/swt/widgets/MenuItem;ZZZZZ)V (115 bytes) @ 0x00007fffd90d9414 [0x00007fffd90d9320+0x00000000000000f4] J 11495 c1 org.eclipse.jface.action.ActionContributionItem.update(Ljava/lang/String;)V (232 bytes) @ 0x00007fffd91a2f4c [0x00007fffd91a2240+0x0000000000000d0c] J 9967 c2 org.eclipse.jface.action.SubContributionItem.update(Ljava/lang/String;)V (11 bytes) @ 0x00007fffe04aab80 [0x00007fffe04aab20+0x0000000000000060] J 11551 c1 org.eclipse.jface.action.MenuManager.update(Ljava/lang/String;)V (291 bytes) @ 0x00007fffd97f2944 [0x00007fffd97f24c0+0x0000000000000484] J 9967 c2 org.eclipse.jface.action.SubContributionItem.update(Ljava/lang/String;)V (11 bytes) @ 0x00007fffe04aab80 [0x00007fffe04aab20+0x0000000000000060] J 11551 c1 org.eclipse.jface.action.MenuManager.update(Ljava/lang/String;)V (291 bytes) @ 0x00007fffd97f2944 [0x00007fffd97f24c0+0x0000000000000484] j org.eclipse.ui.internal.Workbench.updateActiveWorkbenchWindowMenuManager(Z)V+40 j org.eclipse.ui.internal.Workbench.lambda$0(Lorg/eclipse/jface/bindings/BindingManagerEvent;)V+9 j org.eclipse.ui.internal.Workbench$$Lambda$180.bindingManagerChanged(Lorg/eclipse/jface/bindings/BindingManagerEvent;)V+5 j org.eclipse.jface.bindings.BindingManager.fireBindingManagerChanged(Lorg/eclipse/jface/bindings/BindingManagerEvent;)V+45 j org.eclipse.jface.bindings.BindingManager.setActiveBindings(Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;)V+44 j org.eclipse.jface.bindings.BindingManager.recomputeBindings()V+147 j org.eclipse.jface.bindings.BindingManager.contextManagerChanged(Lorg/eclipse/core/commands/contexts/ContextManagerEvent;)V+8 j org.eclipse.core.commands.contexts.ContextManager.fireContextManagerChanged(Lorg/eclipse/core/commands/contexts/ContextManagerEvent;)V+41 j org.eclipse.core.commands.contexts.ContextManager.setActiveContextIds(Ljava/util/Set;)V+100 j org.eclipse.e4.ui.services.ContextServiceAddon$1.changed(Lorg/eclipse/e4/core/contexts/IEclipseContext;)Z+37 J 10002 c1 org.eclipse.e4.core.internal.contexts.TrackableComputationExt.update(Lorg/eclipse/e4/core/internal/contexts/ContextChangeEvent;)Z (305 bytes) @ 0x00007fffd906ca64 [0x00007fffd906ae20+0x0000000000001c44] J 4639 c1 org.eclipse.e4.core.internal.contexts.EclipseContext.processScheduled(Ljava/util/Set;)V (42 bytes) @ 0x00007fffd9b8adec [0x00007fffd9b8aaa0+0x000000000000034c] J 4564 c1 org.eclipse.e4.core.internal.contexts.EclipseContext.set(Ljava/lang/String;Ljava/lang/Object;)V (158 bytes) @ 0x00007fffd9b4625c [0x00007fffd9b45ae0+0x000000000000077c] j org.eclipse.e4.core.internal.contexts.EclipseContext.activate()V+23 J 12963 c1 org.eclipse.e4.ui.internal.workbench.swt.ShellActivationListener.handleEvent(Lorg/eclipse/swt/widgets/Event;)V (110 bytes) @ 0x00007fffd970d694 [0x00007fffd970c960+0x0000000000000d34] J 10555 c2 org.eclipse.swt.widgets.EventTable.sendEvent(Lorg/eclipse/swt/widgets/Event;)V (592 bytes) @ 0x00007fffe0ed5a9c [0x00007fffe0ed5a00+0x000000000000009c] J 5814 c1 org.eclipse.swt.widgets.Display.filterEvent(Lorg/eclipse/swt/widgets/Event;)Z (43 bytes) @ 0x00007fffd9eaf4cc [0x00007fffd9eaf380+0x000000000000014c] J 6939 c1 org.eclipse.swt.widgets.Widget.sendEvent(ILorg/eclipse/swt/widgets/Event;Z)V (88 bytes) @ 0x00007fffda198dfc [0x00007fffda1988c0+0x000000000000053c] J 7004 c1 org.eclipse.swt.widgets.Widget.sendEvent(I)V (8 bytes) @ 0x00007fffda1cca84 [0x00007fffda1cc940+0x0000000000000144] j org.eclipse.swt.widgets.Shell.gtk_focus_in_event(JJ)J+42 J 11699 c2 org.eclipse.swt.widgets.Widget.windowProc(JJJ)J (490 bytes) @ 0x00007fffe06509bc [0x00007fffe0650460+0x000000000000055c] J 10076 c2 org.eclipse.swt.widgets.Control.windowProc(JJJ)J (285 bytes) @ 0x00007fffe055d208 [0x00007fffe055cfe0+0x0000000000000228] J 12082 c2 org.eclipse.swt.widgets.Display.windowProc(JJJ)J (24 bytes) @ 0x00007fffe07dcc40 [0x00007fffe07dcac0+0x0000000000000180] v ~StubRoutines::call_stub J 7267 org.eclipse.swt.internal.gtk3.GTK3.gtk_main_do_event(J)V (0 bytes) @ 0x00007fffe094a191 [0x00007fffe094a140+0x0000000000000051] J 10723 c2 org.eclipse.swt.widgets.Display.eventProc(JJ)J (212 bytes) @ 0x00007fffe0f49764 [0x00007fffe0f496a0+0x00000000000000c4] v ~StubRoutines::call_stub J 7365 org.eclipse.swt.internal.gtk3.GTK3.gtk_main_iteration_do(Z)Z (0 bytes) @ 0x00007fffe096b591 [0x00007fffe096b540+0x0000000000000051] J 12406 c2 org.eclipse.swt.widgets.Display.readAndDispatch()Z (88 bytes) @ 0x00007fffe10e7688 [0x00007fffe10e7520+0x0000000000000168] j org.eclipse.jface.window.Window.runEventLoop(Lorg/eclipse/swt/widgets/Shell;)V+23 j org.eclipse.jface.window.Window.open()I+49 j org.eclipse.jface.dialogs.MessageDialogWithToggle.open(ILorg/eclipse/swt/widgets/Shell;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;ZLorg/eclipse/jface/preference/IPreferenceStore;Ljava/lang/String;ILjava/util/LinkedHashMap;)Lorg/eclipse/jface/dialogs/MessageDialogWithToggle;+57 j org.eclipse.debug.internal.ui.launchConfigurations.PerspectiveManager.shouldSwitchPerspective(Lorg/eclipse/ui/IWorkbenchWindow;Ljava/lang/String;Ljava/lang/String;)Z+324 j org.eclipse.debug.internal.ui.launchConfigurations.PerspectiveManager.access$2(Lorg/eclipse/debug/internal/ui/launchConfigurations/PerspectiveManager;Lorg/eclipse/ui/IWorkbenchWindow;Ljava/lang/String;Ljava/lang/String;)Z+4 j org.eclipse.debug.internal.ui.launchConfigurations.PerspectiveManager$2.runInUIThread(Lorg/eclipse/core/runtime/IProgressMonitor;)Lorg/eclipse/core/runtime/IStatus;+40 j org.eclipse.debug.internal.ui.launchConfigurations.PerspectiveManager$MyUIJob.lambda$0(Lorg/eclipse/core/runtime/IProgressMonitor;)V+22 j org.eclipse.debug.internal.ui.launchConfigurations.PerspectiveManager$MyUIJob$$Lambda$1216.run()V+8 J 12361 c2 org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Z)Z (188 bytes) @ 0x00007fffe10dbd20 [0x00007fffe10db900+0x0000000000000420] J 12406 c2 org.eclipse.swt.widgets.Display.readAndDispatch()Z (88 bytes) @ 0x00007fffe10e7724 [0x00007fffe10e7520+0x0000000000000204] j org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run()V+564 j org.eclipse.core.databinding.observable.Realm.runWithDefault(Lorg/eclipse/core/databinding/observable/Realm;Ljava/lang/Runnable;)V+12 j org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(Lorg/eclipse/e4/ui/model/application/MApplicationElement;Lorg/eclipse/e4/core/contexts/IEclipseContext;)Ljava/lang/Object;+57 j org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(Lorg/eclipse/e4/ui/model/application/MApplicationElement;)V+20 j org.eclipse.ui.internal.Workbench.lambda$3(Lorg/eclipse/swt/widgets/Display;Lorg/eclipse/ui/application/WorkbenchAdvisor;[I)V+393 j org.eclipse.ui.internal.Workbench$$Lambda$138.run()V+12 j org.eclipse.core.databinding.observable.Realm.runWithDefault(Lorg/eclipse/core/databinding/observable/Realm;Ljava/lang/Runnable;)V+12 j org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Lorg/eclipse/swt/widgets/Display;Lorg/eclipse/ui/application/WorkbenchAdvisor;)I+16 j org.eclipse.ui.PlatformUI.createAndRunWorkbench(Lorg/eclipse/swt/widgets/Display;Lorg/eclipse/ui/application/WorkbenchAdvisor;)I+2 j org.eclipse.ui.internal.ide.application.IDEApplication.start(Lorg/eclipse/equinox/app/IApplicationContext;)Ljava/lang/Object;+113 j org.eclipse.equinox.internal.app.EclipseAppHandle.run(Ljava/lang/Object;)Ljava/lang/Object;+138 j org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(Ljava/lang/Object;)Ljava/lang/Object;+85 j org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(Ljava/lang/Object;)Ljava/lang/Object;+79 j org.eclipse.core.runtime.adaptor.EclipseStarter.run(Ljava/lang/Object;)Ljava/lang/Object;+99 j org.eclipse.core.runtime.adaptor.EclipseStarter.run([Ljava/lang/String;Ljava/lang/Runnable;)Ljava/lang/Object;+132 v ~StubRoutines::call_stub j jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Ljava/lang/reflect/Method;Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+0 java.base@11.0.11 j jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+100 java.base@11.0.11 j jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+6 java.base@11.0.11 j java.lang.reflect.Method.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+59 java.base@11.0.11 j org.eclipse.equinox.launcher.Main.invokeFramework([Ljava/lang/String;[Ljava/net/URL;)V+202 j org.eclipse.equinox.launcher.Main.basicRun([Ljava/lang/String;)V+159 j org.eclipse.equinox.launcher.Main.run([Ljava/lang/String;)I+4 j org.eclipse.equinox.launcher.Main.main([Ljava/lang/String;)V+10 v ~StubRoutines::call_stub
Most likely, the crash is caused by an attempt to use already-freed 'accelGroup'. Judging from your patch and crash stack, something in SWT is unaware that 'accelGroup' is already freed. Namely, tracking 'accelGroup' on crash's stack, I see that it comes from 'MenuItem.setAccelerator()', which calls 'getAccelGroup()', which takes 'Decorations.accelGroup' form menu's parent. However, 'Decorations.destroyAccelGroup()' correctly zeroes the value after unref'ing. This probably means that multiple Decorations use the same value without properly ref'ing multiple times. Simeon, could you debug the rest? If you get stuck and need my help, I'll need an SWT snippet (best) or exact steps in Eclipse.
I would suggest to System.out the pair of {Decorations.hashCode(), accelGroup} in 'getAccelGroup()' and 'Decorations.destroyAccelGroup()'.
To reproduce the crash I believe its enough to start Eclipse with a clean workspace. I'm trying to write a snippet for that, I'm not sure yet what triggers the crash exactly. Meanwhile I've also browsed GTK+ code and I don't find any unrefing done for the result of gtk_accel_group_new(). Is SWT supposed to unref the handle?
Yes, "a new GtkAccelGroup object" in docs implies that the caller (SWT) is fully responsible of the object received. I suggest that you first debug it yourself with System.out and/or brekpoints, as I described above.
The crash can be reproduced by running the following snippet and closing one of the shells: public static void main(String[] args) { for (int i = 0; i < 1; ++i) { Display display = new Display(); Shell shell1 = new Shell(display); shell1.setText("Menu Example"); shell1.setSize(300, 200); Label label1 = new Label(shell1, SWT.CENTER); label1.setBounds(shell1.getClientArea()); Menu menu1 = new Menu(shell1, SWT.BAR); MenuItem item11 = new MenuItem(menu1, SWT.PUSH); item11.setText("Select &One\tCtrl+A"); item11.setAccelerator(SWT.MOD1 + 'A'); MenuItem item12 = new MenuItem(menu1, SWT.PUSH); item12.setText("Select &Two\tCtrl+C"); item12.setAccelerator(SWT.MOD1 + 'C'); shell1.setMenuBar(menu1); shell1.open(); Shell shell2 = new Shell(display); shell2.setText("Menu Example"); shell2.setSize(300, 200); Label label2 = new Label(shell2, SWT.CENTER); label2.setBounds(shell2.getClientArea()); Menu menu2 = new Menu(shell2, SWT.BAR); MenuItem item21 = new MenuItem(menu2, SWT.PUSH); item21.setText("Select &Four\tCtrl+A"); item21.setAccelerator(SWT.MOD1 + 'A'); MenuItem item22 = new MenuItem(menu2, SWT.PUSH); item22.setText("Select &Six\tCtrl+C"); item22.setAccelerator(SWT.MOD1 + 'C'); shell2.setMenuBar(menu2); shell1.open(); shell2.open(); label1.setParent(shell2); while (!shell2.isDisposed() && !shell2.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } } The call "label1.setParent(shell2)" seems to result in calling "Decorations.fixAccelGroup()", where new handles are created. I'm still not sure where the problem exactly is, maybe the menu items "don't know" they should be in new containers. I'll continue debugging.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/180940
I have debugged the crash and found the following: 1) 'GtkMenuItem' has internal 'GtkAccelLabel' that remembers its 'accel_group' 2) When 'gtk_widget_add_accelerator()' is called the second time, it's appended to some internal list and 'GtkAccelLabel' does NOT update its 'accel_group' -- See 'gtk_accel_label_set_accel_closure()' in GTK - it's skipped because closure is the same. See 'refetch_widget_accel_closure()' in GTK - it always picks first closure from the list, that's why it's the same. 3) When 'gtk_widget_remove_accelerator()' is called, 'GtkAccelLabel' briefly accesses its 'accel_group' -- See 'gtk_accel_label_set_accel_closure()' in GTK 4) SWT's 'Decorations.fixAccelGroup()' destroys 'accel_group' while it's still set to MenuItem. To summarize, 'Decorations.fixAccelGroup()' destroys 'accel_group' while it's still used by GTK. Two fixes are possible: 1) Call 'menuBar.removeAccelerators()' in 'Decorations.fixAccelGroup()' before 'destroyAccelGroup()' 2) ref/unref 'accel_group' when passing it to GTK. I understand that (1) is a clearer fix. Also, (2) causes a subtle memory leak to occur: as long as Shell is alive, every 'fixAccelGroup()' will add another accel group to the GTK's internal list, and it will not be released until all MenuItems are destroyed. I have tried (1) quickly and it seems to work. Simeon, can you do more testing?
Sadly, the problem here is even deeper. What worried me about the patch is that (in first version) I noticed two places that called 'gtk_widget_add_accelerator()'. I tested and found that calling 'gtk_widget_add_accelerator()' the second time causes the crash (and probably a leak during runtime) to return. Next, I wondered if it's possible to call both 'MenuItem.updateAccelerator()' and 'MenuItem.updateAcceleratorText()' to have a duplicate 'gtk_widget_add_accelerator()' ? I didn't find such a way - these two functions early return on mutually exclusive conditions related to 'MenuItem.accelerator'. Also, it seems that 'OS.ubuntu_menu_proxy_get()' guarding 'updateAcceleratorText()' is always false on modern Ubuntu 20.04, at least I didn't find a way to enable menu proxy. I then figured that 'MenuItem.updateAccelerator()' itself could be called multiple times. I studied the references and found 'Shell.setMenuBar()' that looked suspicious. Indeed, the following pseudocode causes the crash (or scary GTK-CRITICAL's to console): ---- shell.setMenuBar(menu); shell.setMenuBar(null); menu.dispose(); ---- This is what should happen: * Every 'gtk_widget_add_accelerator()' shall be balanced by matching 'gtk_widget_remove_accelerator()' * 'accelGroup' may only be destroyed when it's removed from all widgets already. Unfortunately SWT violates both. We fixed 'fixAccelGroup()', but there's also 'Shell.setMenuBar()' and possibly other ways to violate the rules. The first version of the patch worked around the crash, but runtime leak still remained - because unbalanced 'gtk_widget_add_accelerator()' will cause 'accelGroup' to remain in memory while any of the widgets are still there. That is, every 'fixAccelGroup()', 'setMenuBar()' etc will cause memory consumption to grow while the same 'Menu' is used. With all that, I think that 'menuBar.removeAccelerators()' shall be moved from 'fixAccelGroup()' to 'destroyAccelGroup()'. This will hopefully fix both crash scenarios. Given the complexity of this change, I suggest to make a proper test snippet and commit it to the repo with these tests: 1) Simeon's original snippet for the crash + Automated through shell.dispose() to not need user interaction + Remove anything not relevant to the crash 2) New 'setMenuBar(null)' + 'menu.dispose()' crash 3) A test for possible runtime leak through via calling 'fixAccelGroup()' multiple times for same Menu
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/180940 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=623f26d8117571ac812c9802f4ae075aacc3d040
