Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 320887 - [Compatibility] Eclipse context of PageSites are never disposed
Summary: [Compatibility] Eclipse context of PageSites are never disposed
Status: RESOLVED FIXED
Alias: None
Product: e4
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 1.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.1 M1   Edit
Assignee: Remy Suen CLA
QA Contact: Remy Suen CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-26 07:50 EDT by Remy Suen CLA
Modified: 2010-07-29 07:44 EDT (History)
4 users (show)

See Also:
pwebster: review+
ob1.eclipse: review+


Attachments
PageSite patch v1 (665 bytes, patch)
2010-07-26 07:50 EDT, Remy Suen CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Suen CLA 2010-07-26 07:50:32 EDT
Created attachment 175215 [details]
PageSite patch v1

We activate/deactivate them properly but never actually dispose them _individually_. So technically speaking, they're just going to lie around forever until you actually close the view. And keeping shared parts in mind, you'd have to close them in every single perspective you have up.
Comment 1 Paul Webster CLA 2010-07-26 12:27:00 EDT
I think we should consider this for the 7 p.m. build.  The current behaviour means that those contexts will exist for every editor that was opened, for as long as the outline view is open in any perspective.

PW
Comment 2 Paul Webster CLA 2010-07-26 13:09:16 EDT
Oleg or John, could you look at the code for this (we're just matching up a eclipseContext.dispose() with the object that does the create).

Boris, could you consider for 4.0?

PW
Comment 3 Boris Bokowski CLA 2010-07-26 13:16:34 EDT
Oleg, could you please do an additional review?
Comment 4 Oleg Besedin CLA 2010-07-26 14:06:46 EDT
> +		e4Context.dispose();

In this particular case this is not going to do much of a difference.

The only "external" thing hapenning on #dipose() is that whoever was injected with the context will be notified. However, look at how this context is used:

 serviceLocator.setContext(e4Context);
and
 IHandlerService handlerService = new LegacyHandlerService(e4Context);

So, while it might be used down the road for injection from either ServiceLocator, or the LegacyHandlerService, there is no immediate point where it is injected anywhere.

From the "internal side, the parent context keeps child contexts as a weak reference, so in theory, using #dispose() won't [directly] help garbage collection.

And, as a side reference, this is what LegacyHandlerService#dispose() looks like:
	public void dispose() {
		E4Util.message("LegacyHandlerService.dispose: should it do something?"); //$NON-NLS-1$
	}


To summarize:

 This is a right thing to do, but not now, not in RC4, unless there is a *very* demonstrable benefit.
Comment 5 Boris Bokowski CLA 2010-07-26 14:12:49 EDT
Moving to 4.1. We're leaking like a sieve anyway (bug 320857).
Comment 6 Oleg Besedin CLA 2010-07-26 14:23:29 EDT
+1 for 4.1.
Comment 7 Remy Suen CLA 2010-07-29 07:44:14 EDT
Fix released for 4.1.