Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leaks in activation. #689

Merged
merged 12 commits into from
Aug 1, 2014
Merged

Fix memory leaks in activation. #689

merged 12 commits into from
Aug 1, 2014

Conversation

jlaanstra
Copy link
Member

This PR fixes two memory leaks that cause views to stick around forever. This also fixes #684.

Memory Leak No. 1:

The ActivationForViewFetcher for Xaml wants to be notified of changes to the IsHitTestVisible property and subscribes to it, but this subscription sticks around, even if the view is unloaded. Because IsHitTestVisible is a dependency property, this leaks the view.

This has been fixed by only listening to IsHitTestVisible when the view is loaded.

Memory Leak No. 2:

The activation logic subscribes to the ViewModel property of a view to be notified of changes. Similar to leak no. 1, this subscription sticks around, even if a view is unloaded.

This has been fixed by disposing this subscription when the view is unloaded.

Long-term Improvement

These fixes made the already complicated activation logic a little more complicated. I believe it has been discussed before why the activation logic consists of two observables, instead of one and I cannot remember what the reasons for having two were. Now might be a good time to refactor this into a single observable returning a bool indicating active or inactive.

Thoughts?

@anaisbetts
Copy link
Member

Now might be a good time to refactor this into a single observable returning a bool indicating active or inactive.

I'm not sure we can do that, it'd be a breaking change, no? Or do you just mean the internals for activation

@jlaanstra
Copy link
Member Author

I'm not sure we can do that, it'd be a breaking change, no? Or do you just mean the internals for activation.

There would indeed be a breaking change in the IActivationForViewFetcher interface. We should not change it if people are actually implementing this themselves, but I think most people use the builtin stuff. Is anyone implementing this himself?

The behavior of the activation stuff will not change.

@jlaanstra
Copy link
Member Author

The interface has been adjusted. Paging @rikbosch to check if I did not fuck up Winforms activation.

@rikbosch
Copy link
Contributor

rikbosch commented Aug 1, 2014

@jlaanstra it's looking OK.

I've removed the unused observable declarations

@jlaanstra
Copy link
Member Author

@rikbosch Thanks missed them apparently. @paulcbetts :shipit:

@anaisbetts anaisbetts merged commit 7d0d42e into master Aug 1, 2014
@anaisbetts
Copy link
Member

Cool! Thanks @jlaanstra @rikbosch!

@anaisbetts anaisbetts deleted the fix-memoryleaks branch August 1, 2014 11:48
@lock lock bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

class DependencyObjectObservableForProperty has a memory leak
3 participants