Memory leaks detected with SwtBot tests

[lredor]

Some memory leaks have been observed with the SwtBot tests suites. The goal of this issue is to bring together all the elements related to the different analyzes that will be carried out in order to resolve memory leaks.

[wldblm]

For this leak, when some tests fail, they retain a number of unnecessary elements, until the end of the whole testSuite. In the superclass AbstractSiriusSwtBotGefTestCase, the editor wasn't set to null during tearDown. Also, some of the tests overwrote editor with their own value but didn't dispose of it on tearDown.

To fix this here is what we did :

• We edited or added a dispose() function in the Tests that locally declared editor but never disposed it .
• We edited the tearDown() function of abstract superclass AbstractSiriusSwtBotGefTestCase so it will close() the editor and set it to null.

Leak founded by profiling RoutingStyleTest

[lredor]

In the superclass AbstractSiriusSwtBotGefTestCase, the editor wasn't set to null during tearDown.

It seems strange. I will have a look to your PR but the TestCaseCleaner(this).clearAllFields(); called in tearDown does this job, even if the field editor is "cloned" in the specific test class and in AbstractSiriusSwtBotGefTestCase. The clearAllFields considers all fields of all class AND super-classes.

We edited the tearDown() function of abstract superclass AbstractSiriusSwtBotGefTestCase so it will close().

In the same way, this should be done during the call to closeAllEditors();

Do you have a steps to validate? Indeed, I'm not sure that your propose fix is necessary. Or in any case, it doesn't cover the cases you mention but perhaps more specific cases that should be handled differently (probably with the code already in place).

[lredor]

The analysis done by @wldblm and @ebausson-obeo was not complete. So the proposed solution in PR #147 was flawed.

By analyzing the RoutingStyleTest tests (which were KO locally) and which are mentionned by Walid as one of test doing a leak on DDiagramEditorImpl through fFailedTest field of junit.framework.TestFailure class, I have detected that the leak exist only if the test fails during the failureTearDown() method. In this case, the method org.eclipse.sirius.tests.support.api.TestCaseCleaner.clearAllFields(), called, in theory, just after failureTearDown(), is never called and the fields are not cleaned (hence the memory leak).

I'll add the corresponding PR and associated steps to reproduce later.

Here is a sample of error seen locally with org.eclipse.sirius.tests.swtbot.RoutingStyleTest.testRoutingStyleWithOneEdgeSelected() :

java.lang.AssertionError: Error(s) raised during test : org.eclipse.sirius.tests.swtbot.RoutingStyleTest
. Log Plugin : org.eclipse.core.runtime
  . Error from plugin:org.eclipse.ui, message: Unhandled event loop exception, exception: java.lang.NullPointerException: Cannot invoke "String.length()" because "item.text" is null
   . Stack trace: java.lang.NullPointerException: Cannot invoke "String.length()" because "item.text" is null
    at org.eclipse.swt.widgets.ToolBar.layoutItems(ToolBar.java:752)
    at org.eclipse.swt.widgets.ToolItem.updateImages(ToolItem.java:1192)
    at org.eclipse.swt.widgets.ToolItem.setEnabled(ToolItem.java:758)
    at org.eclipse.jface.action.ActionContributionItem.updateToolItem(ActionContributionItem.java:807)
    at org.eclipse.jface.action.ActionContributionItem.update(ActionContributionItem.java:739)
    at org.eclipse.jface.action.ActionContributionItem.actionPropertyChange(ActionContributionItem.java:169)
    at org.eclipse.jface.action.AbstractAction.firePropertyChange(AbstractAction.java:52)
    at org.eclipse.jface.action.AbstractAction.firePropertyChange(AbstractAction.java:75)
    at org.eclipse.jface.action.Action.setEnabled(Action.java:541)
    at org.eclipse.gmf.runtime.common.ui.action.ActionMenuManager.itemRemoved(ActionMenuManager.java:443)
    at org.eclipse.sirius.diagram.ui.tools.internal.editor.tabbar.actions.TabbarRouterMenuManager.itemRemoved(TabbarRouterMenuManager.java:53)
    at org.eclipse.jface.action.ContributionManager.removeAll(ContributionManager.java:449)
    at org.eclipse.sirius.diagram.ui.tools.internal.editor.tabbar.actions.TabbarRouterMenuManager.dispose(TabbarRouterMenuManager.java:58)
    at org.eclipse.gmf.runtime.common.ui.action.ActionMenuManager$MenuCreatorAction.dispose(ActionMenuManager.java:127)
    at org.eclipse.jface.action.ActionContributionItem.handleWidgetDispose(ActionContributionItem.java:475)
    at org.eclipse.jface.action.ActionContributionItem.lambda$5(ActionContributionItem.java:448)
    at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
    at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4274)
    ...
    at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.hidePart(PartServiceImpl.java:1410)
    at org.eclipse.ui.internal.WorkbenchPage.hidePart(WorkbenchPage.java:1543)
    at org.eclipse.ui.internal.WorkbenchPage.hidePart(WorkbenchPage.java:1495)
    at org.eclipse.ui.internal.WorkbenchPage.closeEditors(WorkbenchPage.java:1465)
    at org.eclipse.ui.internal.WorkbenchPage.closeAllEditors(WorkbenchPage.java:1342)
    at org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase.lambda$2(AbstractSiriusSwtBotGefTestCase.java:392)
    at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
    at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4047)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3663)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
    ...
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:588)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1459)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1432)

    at org.junit.Assert.fail(Assert.java:89)
    at org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase.checkLogs(AbstractSiriusSwtBotGefTestCase.java:1510)
    at org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase.failureTearDown(AbstractSiriusSwtBotGefTestCase.java:1712)
    at org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase.tearDown(AbstractSiriusSwtBotGefTestCase.java:1925)
    at junit.framework.TestCase.runBare(TestCase.java:147)
    at org.eclipse.swtbot.swt.finder.SWTBotTestCase.runBare(SWTBotTestCase.java:240)
    at org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase.runBare(AbstractSiriusSwtBotGefTestCase.java:1947)
    ...
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
    at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:81)
    at org.eclipse.pde.internal.junit.runtime.PlatformUITestHarness.lambda$0(PlatformUITestHarness.java:45)
    at java.base/java.lang.Thread.run(Thread.java:833)

[lredor]

The PR #165 fixes the above leak. Here are some steps to validate:

• Launch test class org.eclipse.sirius.tests.swtbot.EdgeCreationTest
• Expected: 1 test is OK
• Make the following changes in the source code
  • Change the visibility of the method org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase.failureTearDown() from private to protected
  • Override the method failureTearDown in EdgeCreationTest and add the line fail("Force manual failure after failureTearDown()."); //$NON-NLS-1$ at the end of the method, just after calling super.failureTearDown();
• Launch test class org.eclipse.sirius.tests.swtbot.EdgeCreationTest
• Expected: 1 test is KO
• Make the following changes in the source code
  • Comment all tests preceding EdgeCreationTest in org.eclipse.sirius.tests.swtbot.suite.AllTestSuite.addGerritPart2(TestSuite).
  • Replace the content of org.eclipse.sirius.tests.swtbot.suite.AllTestSuite.suite() with

    public static Test suite() {
    TestSuite suite = new TestSuite("Sirius SwtBot tests"); //$NON-NLS-1$
    addGerritPart2(suite);
    return suite;
    }

• Launch test class org.eclipse.sirius.tests.swtbot.suite.AllTestSuite with YourKit (sample of startup options in "YourKit Java Profiler" tab of launch config: exceptions=off,disablestacktelemetry,alloceach=100,allocsizelimit=4096 to have memory analysis enabled)
• Take a memory snapshot just after the execution of EdgeCreationTest, as soon as it is written as failure in Junit view (or when the following test is executed)
• Stop the execution of the tests suite
• Expected: In Yourkit, you can observed the following leak:
• Apply the patch from PR #165 (Warning: the PR modifies the org.eclipse.sirius.tests.swtbot.support.api.AbstractSiriusSwtBotGefTestCase, so you have to apply it "manually". It is simple.)
• Launch test class org.eclipse.sirius.tests.swtbot.suite.AllTestSuite with YourKit
• Take a memory snapshot just after the execution of EdgeCreationTest, as soon as it is written as failure in Junit view (or when the following test is executed)
• Stop the execution of the tests suite
• Expected: In Yourkit, the leak is no longer visible.

[wldblm]

PopupMenuExtender has a selProvider parameter which could be the source of a leak. In the PopupMenuExtender class, the dispose() function seems to dispose of a number of things, but when switching to debug mode to reproduce the test case, the selProvider doesn't seem to be set to null.

ExtensionRegistry own a ListenerInfo class that implements a listener attribute which is the PopupMenuExtender. I debugged the ListenerInfo constructor. When the diagram is closed, the removeRegistryChangeListener() method is called with the PopupMenuExtender listener as parameter, which itself contains its selProvider, the SiriusDiagramGraphicalViewer. According to the investigation, the PopupMenuExtender has been removed from the list of listeners in the ExtensionRegistry class. We'll have to look into this further and understand why the YourKit analysis says it's still there.

Step to reproduce :

• Launching sirius with yourkit (tp 2023-03)
• Switch to modeling view
• open the aird then "diagramWithNodesAndEdges"
• right-click on edge1 -> format -> order -> sendToBack (or bringToFront if yours is already behind)
• then go to the edit menu -> undo
• close the editor without saving
• go to yourkit and take a snapshot of the memory

You will need the files inside the folder /data/unit/tools/zorder/of the class org.eclipse.sirius.tests.swtbot.layout.ZOrderActionsTest

[ebausson-obeo]

Disposable & IDisposable

A number of items about DDiagramEditor implements a variation of the 'Disposable' design pattern. The issue is there are multiple sources for said interface , and they do not descend from the same superclass/interface, and may have different lifecycles. We were not able to isolate a case where the wrong disposable was implemented by Sirius (unreleated 'Disposable' or 'IDisposable' interfaces are declared in emf, gef, gmf and eclipse), though our search wasn't exhaustive.

[ebausson-obeo]

ToggleRouterAction & AbstractActionHandler

The ToggleRouterAction keep an indirect reference to the DDiagramEditor despite said diagram being closed. Finding a way to cleanly dispose of said reference when closing the Diagram is the most promising lead to stop the leaks, as references to ToggleRouterAction are everywhere in memory dumps.

Step to reproduce :

• From master (as of 2023-11-15)
• Launch test class org.eclipse.sirius.tests.swtbot.layout.ZOrderActionsTest with YourKit
• Take a memory snapshot as the test end, or earlier as long as a few test have been executed

[ebausson-obeo]

SendBackwardAction & SendBackwardCommand

Sirius's SendBackwardAction and SendBackwardCommand (among others similar actions) keep a reference to the current selection, and through those to the DDiagramEditor. The reference might be kept until the next use of the action (in a different diagram), as the commands are singletons. It may be preferable during the closure of the editor to force the deallocation of said references.

Step to reproduce :

• From master (as of 2023-11-15)
• Launch test class org.eclipse.sirius.tests.swtbot.layout.ZOrderActionsTest with YourKit
• Take a memory snapshot as the test end, or earlier as long as a few test have been executed
  • One of the DDiagramEditorImpl will be retained though SendBackWardCommand.

[lredor]

Several problems has been analyzed/detailed by Etienne and Walid in the above comments. Here is an overview of the current state:

• A- Memory leak for tests failures
  • Fixed through PR #165
• B- Memory leak for PopupMenuExtender
  • Currently not reproduced by myself
• C- Memory leak for "Disposable & IDisposable" actions
  • Maybe a consequence of other memory leaks (to be confirmed later).
• D- Memory leak for "ToggleRouterAction & AbstractActionHandler" action
  • Detailed in another issue: #176
  • Fixed through PR #177
• E- Memory leak for "SendBackwardAction & SendBackwardCommand" action
  • Detailed in another issue: #174
  • Fixed through PR #175