Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 318946 - EclipseContext may leak TrackableComputationExt
Summary: EclipseContext may leak TrackableComputationExt
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Runtime (show other bugs)
Version: 3.7   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 4.1   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-05 18:24 EDT by Stefan Mücke CLA
Modified: 2011-05-17 16:25 EDT (History)
2 users (show)

See Also:


Attachments
patch (1.07 KB, patch)
2010-07-06 02:54 EDT, Stefan Mücke CLA
ob1.eclipse: iplog+
Details | Diff
Patch O2 (4.01 KB, patch)
2010-07-06 11:39 EDT, Oleg Besedin CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Mücke CLA 2010-07-05 18:24:56 EDT
In a memory snapshot there were 44.482 listeners installed in a single EclipseContext; 35.580 of them were of type TrackableComputationExt. These TrackableComputationExt objects retained about 17 MB of data.

Currently I am not able to find any place where listeners are added to the collection of listeners. Seems to be 'magic'. And, there seems to be no place where listeners are removed from a context.
Comment 1 Stefan Mücke CLA 2010-07-05 18:43:49 EDT
Found. The EclipseContext listeners are only modified outside of the class, namely, in Computation. The class add a listener to the context when startListening(...) is called. TrackableComputationExt calls this method but fails to remove the listener afterwards.
Comment 2 Boris Bokowski CLA 2010-07-05 20:35:56 EDT
Those numbers seem way too high. However, I don't think it's a problem that the call to startListening() does not have a corresponding stopListening() call. The problem must be somewhere else, for example if we create TrackableComputationExt objects multiple times, for the same context and the same purpose.
Comment 3 Stefan Mücke CLA 2010-07-06 02:54:24 EDT
Created attachment 173500 [details]
patch

Actually the problem is in Computation.stopListening(...). This method doesn't do what it is supposed to do. It never removes the listener.

Please check if the code related to bug 304859 (see comments in class Computation) can be removed.
Comment 4 Stefan Mücke CLA 2010-07-06 08:26:12 EDT
I was too quick. My patch doesn't solve the problem, but I still think the patch is correct.

I am doing some further investigation.
Comment 5 Stefan Mücke CLA 2010-07-06 10:39:41 EDT
I have a question:

When a part is activated, the theme engine updates the context with some values. As a result, for example, CSSPropertyInnerKeylineSWTHandler.applyCSSProperty(...) gets called. This method fetches the css context, sets the 'innerKeyline' value, and then injects the context into the renderer:

    ContextInjectionFactory.inject(renderer, context);

This looks like a bug to me. Shouldn't this method be called only once? (i.e. not every time the property changes) The problem is that inject(...) will install a TrackAndRun for the relevant Requestors. But, tracking has already been activted with the previous call to inject(...), so that no injection would be necessary. Afterw the renewed call of inject(...) there will be two trackers, because the requesting object is still alive. Am I missing something?
Comment 6 Boris Bokowski CLA 2010-07-06 11:17:58 EDT
(In reply to comment #5)
>     ContextInjectionFactory.inject(renderer, context);
> 
> This looks like a bug to me. Shouldn't this method be called only once?

Yes, looks like a bug to me too. See bug 318798 comment 8. Bogdan is going to change this code to just do a java.lang.reflect.Method.invoke() instead of the heavyweight context stuff.
Comment 7 Stefan Mücke CLA 2010-07-06 11:23:50 EDT
This should fix the memory leak.
Comment 8 Oleg Besedin CLA 2010-07-06 11:35:38 EDT
(In reply to comment #5)
> I have a question:
> When a part is activated, the theme engine updates the context with some
> values. As a result, for example,
> CSSPropertyInnerKeylineSWTHandler.applyCSSProperty(...) gets called. This
> method fetches the css context, sets the 'innerKeyline' value, and then injects
> the context into the renderer:
>     ContextInjectionFactory.inject(renderer, context);
> This looks like a bug to me.

Yes, this is the bug 313950.
Comment 9 Oleg Besedin CLA 2010-07-06 11:39:22 EDT
Created attachment 173571 [details]
Patch O2

Yes, let's remove changes made for the bug 304859 and add explicit listener removal as suggested by Stefan. That should clear the picture a bit.
Comment 10 Oleg Besedin CLA 2010-07-06 11:42:17 EDT
Patch applied to CVS Head. The CSS classes over-using injection will be tracked in the bug 313950.