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 several Android memory leaks #360

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
6 participants
@hartez
Member

hartez commented Sep 15, 2016

Description of Change

Fixes for several memory leaks on Android. Changes include:

  • Calling Destroy() on Map during disposal (leaking the Map object)
  • Cleaning up OnFocusChangeListener on ViewRenderer during disposal (leaking various renderers)
  • Cleaning up listeners and tag on ButtonRenderer during disposal (leaking ButtonRenderer)
  • Fixing bug where animated fragment transactions leaked FragmentContainer and PageContainer

Bugs Fixed

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@rmarinho

some small questions and comments

Show outdated Hide outdated Xamarin.Forms.Maps.Android/MapRenderer.cs
if (disposing && !_disposed) {
_disposed = true;
if (_disposed)
{

This comment has been minimized.

@rmarinho

rmarinho Sep 15, 2016

Member

Could drop { here

@rmarinho

rmarinho Sep 15, 2016

Member

Could drop { here

Show outdated Hide outdated Xamarin.Forms.Maps.Android/MapRenderer.cs
if (mapModel != null) {
MessagingCenter.Unsubscribe<Map, MapSpan> (this, MoveMessageName);
((ObservableCollection<Pin>)mapModel.Pins).CollectionChanged -= OnCollectionChanged;
if (disposing)

This comment has been minimized.

@rmarinho

rmarinho Sep 15, 2016

Member

if !disposing return ?

@rmarinho

rmarinho Sep 15, 2016

Member

if !disposing return ?

This comment has been minimized.

@hartez

hartez Sep 15, 2016

Member

Can't return right here, still need to call base.Dispose(disposing).

@hartez

hartez Sep 15, 2016

Member

Can't return right here, still need to call base.Dispose(disposing).

}
// We do *not* eagerly dispose of the _pageContainer here; doing so causes a memory leak
// if animated fragment transitions are enabled (it removes some info that the animation's
// onAnimationEnd handler requires to properly clean things up)

This comment has been minimized.

@rmarinho

rmarinho Sep 15, 2016

Member

OHHH

@rmarinho
{
tcs.TrySetResult(true);
fragment.UserVisibleHint = true;
fragment.UserVisibleHint = !removed;

This comment has been minimized.

@rmarinho

rmarinho Sep 15, 2016

Member

where does remove get set? after animation ?

@rmarinho

rmarinho Sep 15, 2016

Member

where does remove get set? after animation ?

This comment has been minimized.

@hartez

hartez Sep 15, 2016

Member

It's one of the parameters to SwitchContentAsync (the method we're in right now).

@hartez

hartez Sep 15, 2016

Member

It's one of the parameters to SwitchContentAsync (the method we're in right now).

Show outdated Hide outdated Xamarin.Forms.Maps.Android/MapRenderer.cs
NativeMap.MyLocationEnabled = false;
NativeMap.SetOnCameraChangeListener(null);
NativeMap.InfoWindowClick -= MapOnMarkerClick;
NativeMap.Dispose();
}

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 16, 2016

Member
NativeMap = null;
@StephaneDelcroix

StephaneDelcroix Sep 16, 2016

Member
NativeMap = null;

This comment has been minimized.

@hartez

hartez Sep 16, 2016

Member

NativeMap refers to MapView.Map, which is read-only.

@hartez

hartez Sep 16, 2016

Member

NativeMap refers to MapView.Map, which is read-only.

Control.RemoveOnAttachStateChangeListener(this);
Control.Tag = null;
_textColorSwitcher = null;
}

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 16, 2016

Member
Control = null;
@StephaneDelcroix

StephaneDelcroix Sep 16, 2016

Member
Control = null;

This comment has been minimized.

@hartez

hartez Sep 16, 2016

Member

Not here. The base class ViewRenderer<TView, TNativeView> still has some cleanup to do on Control, then it gets set to null.

@hartez

hartez Sep 16, 2016

Member

Not here. The base class ViewRenderer<TView, TNativeView> still has some cleanup to do on Control, then it gets set to null.

Show outdated Hide outdated Xamarin.Forms.Maps.Android/MapRenderer.cs
((ObservableCollection<Pin>)mapModel.Pins).CollectionChanged -= OnCollectionChanged;
if (disposing)
{
_disposed = true;

This comment has been minimized.

@campersau

campersau Sep 16, 2016

Not needed. This is already done above.

@campersau

campersau Sep 16, 2016

Not needed. This is already done above.

This comment has been minimized.

@hartez

hartez Sep 16, 2016

Member

Good catch. Fixed.

@hartez

hartez Sep 16, 2016

Member

Good catch. Fixed.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Sep 16, 2016

Member

Needs rebase

Member

rmarinho commented Sep 16, 2016

Needs rebase

Clean up listeners and tag on ButtonRenderer during disposal
Clean up OnFocusChangeListener on ViewRenderer during disposal
Prevent memory leak of PageContainer/FragmentContainer when animating fragment transitions
Call Destroy() on Map during disposal

Rebasing

@rmarinho rmarinho merged commit 6aa96a4 into master Sep 27, 2016

2 of 4 checks passed

Android-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests failed: 1, pass…
Details
iOS9-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Exit code 1 (new)
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle8 :: Windows Debug : Tests passed: 3437, ignored: 8
Details

@rmarinho rmarinho deleted the fix-bugzilla39489-part2 branch Sep 27, 2016

@hartez hartez referenced this pull request Oct 1, 2016

Merged

Set UserVisibleHint for new fragment to true #411

4 of 4 tasks complete

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment