Bug 318944

Comment 1 Boris Bokowski 2010-07-05 17:41:24 EDT

The class CSSSWTEngineImpl doesn't have any fields - how can it leak memory?

Comment 2 Stefan Mücke 2010-07-05 17:46:16 EDT

Good question. Don't believe what your profiler is telling you ;-).
The 'elementsContext' field is in the super class AbstractCSSEngine.  CSSSWTEngineImpl was the actual class used, but the leak may be in any of the super classes.

Comment 3 Stefan Mücke 2010-07-05 17:56:55 EDT

I think I have found the bug. Open the Call Hierarchy on AbstractCSSEngine.getElementsContext(). There are only two methods where getElementsContext() is called. Method AbstractCSSEngine.getElement(...) is the only place where elements are put into the context. However, there is no place in the code where elements are removed from the context. The context seems to be an ever growing collection of widgets and child element contexts. However, since there was not a large number of widgets in the snapshot, it seems to me as if the elements context is creating child contexts containing the same widgets.

Comment 4 Boris Bokowski 2010-07-05 17:59:04 EDT

Bogdan, this one is for you :-)

Comment 5 Angelo ZERR 2010-07-06 07:17:43 EDT

(In reply to comment #4)
> Bogdan, this one is for you :-)

Hi, 

The Map elementsContext store the CSS element context for each widget. The Map is never clear but if you see the method AbstractCSSEngine#dispose(), it is setted to null : 

public void dispose() {
		reset();
		// Call dispose for each CSSStylableElement which was registered
		Collection contexts = elementsContext.values();
		for (Iterator iterator = contexts.iterator(); iterator.hasNext();) {
			CSSElementContext context = (CSSElementContext) iterator.next();
			Element element = context.getElement();
			if (element instanceof CSSStylableElement) {
				((CSSStylableElement) element).dispose();
			}
		}
		elementsContext = null;
		if (resourcesRegistry != null)
			resourcesRegistry.dispose();
	}
}

Regards Angelo

Comment 6 Stefan Mücke 2010-07-08 17:52:06 EDT

Created attachment 173826 [details]
Patch

This patch hooks a widget when an element context is created and stored in the internal map. When a widget is disposed, the element context will be removed.

This patch has been tested. Previously, switching the perspective 50 times resulted in a leak of 6 MB. This leak has gone.

Comment 7 Boris Bokowski 2010-07-12 16:51:51 EDT

Bogdan, any reason why we wouldn't apply this patch?

Comment 8 Boris Bokowski 2010-07-16 11:37:52 EDT

I have applied the patch. We need to get this into RC2 so that we all run with the changes for long enough to make sure everything still works. If we find problems we can always revert, we have the patch on the bug.

Comment 9 Boris Bokowski 2010-07-16 11:38:11 EDT

Comment on attachment 173826 [details]
Patch

Thank you for the patch, Stefan!

Comment 10 Mike Marchand 2020-03-19 13:43:53 EDT

I should perhaps not be commenting on an old bug, but I don't believe this was correctly resolved.  I am currently experiencing issues with the AbstractCSSEngine leaking memory.  

The dispose listener for Widgets only had the effect of preventing Widgets from leaking when disposed, but all other data types that end up in the elementsContext map are never removed from the map when they go out of scope but the engine remains in scope.  I believe we should be using a WeakHashMap<Object, CSSStylableElement> instead.

I am seeing this leak as a fairly serious problem because I am using org.eclipse.nebula.widgets.nattable.NatTable.  The NatTable implementation eventually gives the CSSSWTEngineImpl a NatTableWrapper which does not extend Widget.  The wrapper class is holding into the NatTable and there is a significant amount of data that is still referenced by the NatTable and it's members.  

I can try to be more aggressive with the NatTable API to try and clear as many references as I can when it is disposed, but at the end of the day, the AbstractCSSEngine will still be holding onto many references that it defintely should not be.

Comment 11 Mickael Istria 2020-03-19 13:45:21 EDT

@Mike: can you please open a new bug about that?

Comment 12 Mickael Istria 2020-03-19 13:46:12 EDT

(In reply to Mickael Istria from comment #11)
> @Mike: can you please open a new bug about that?

And then add a link to it here?

Comment 13 Mike Marchand 2020-03-19 14:10:47 EDT

New bug created: https://bugs.eclipse.org/bugs/show_bug.cgi?id=561274