Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Fix MasterDetailPage/NavigationPage leaks on iPad #426

Merged
merged 2 commits into from Oct 12, 2016
Merged

Fix MasterDetailPage/NavigationPage leaks on iPad #426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69168d6
Remove Master page property changed handler to eliminate leak
hartez Oct 5, 2016
0692422
Fix memory leaks with MasterDetailPage and NavigationPage on iOS
hartez Oct 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla44166.cs
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ protected override void Init()
};
}

#if UITEST
#if UITEST
[Test]
public void Bugzilla44166Test()
{
Expand All @@ -71,7 +71,7 @@ public void Bugzilla44166Test()
RunningApp.WaitForElement(q => q.Marked("Back"));
RunningApp.Tap(q => q.Marked("Back"));

for(int n = 0; n < 10; n++)
for (var n = 0; n < 10; n++)
{
RunningApp.WaitForElement(q => q.Marked("GC"));
RunningApp.Tap(q => q.Marked("GC"));
Expand All @@ -82,7 +82,7 @@ public void Bugzilla44166Test()
}
}

var pageStats = string.Empty;
string pageStats = string.Empty;

if (_44166MDP.Counter > 0)
{
Expand All @@ -106,7 +106,7 @@ public void Bugzilla44166Test()

Assert.Fail($"At least one of the pages was not collected: {pageStats}");
}
#endif
#endif
}

[Preserve(AllMembers = true)]
Expand All @@ -121,7 +121,7 @@ public _44166MDP()

Master = new _44166Master();
Detail = new _44166Detail();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um...spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, had to reinstall VS yesterday, and spaces is the default in VS. Thought I'd caught all of them, but I guess not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from now on, I'll refer to @samhouts as Style Cop 👮


~_44166MDP()
{
Expand Down
8 changes: 7 additions & 1 deletion Xamarin.Forms.Platform.iOS/Renderers/NavigationRenderer.cs
View file
Open in desktop
Expand Up @@ -831,7 +831,13 @@ protected override void Dispose(bool disposing)
if (disposing)
{
((IPageController)Child).SendDisappearing();
Child = null;

if (Child != null)
{
Child.PropertyChanged -= HandleChildPropertyChanged;
Child = null;
}

_tracker.Target = null;
_tracker.CollectionChanged -= TrackerOnCollectionChanged;
_tracker = null;
Expand Down
79 changes: 45 additions & 34 deletions Xamarin.Forms.Platform.iOS/Renderers/TabletMasterDetailRenderer.cs
View file
Open in desktop
Expand Up @@ -57,10 +57,7 @@ public class TabletMasterDetailRenderer : UISplitViewController, IVisualElementR
IPageController PageController => Element as IPageController;
IElementController ElementController => Element as IElementController;

protected MasterDetailPage MasterDetailPage
{
get { return _masterDetailPage ?? (_masterDetailPage = (MasterDetailPage)Element); }
}
protected MasterDetailPage MasterDetailPage => _masterDetailPage ?? (_masterDetailPage = (MasterDetailPage)Element);

IMasterDetailPageController MasterDetailPageController => MasterDetailPage as IMasterDetailPageController;

Expand All @@ -71,36 +68,50 @@ UIBarButtonItem PresentButton

protected override void Dispose(bool disposing)
{
if (!_disposed && disposing)
{
if (Element != null)
{
PageController.SendDisappearing();
Element.PropertyChanged -= HandlePropertyChanged;
Element = null;
}

if (_tracker != null)
{
_tracker.Dispose();
_tracker = null;
}

if (_events != null)
{
_events.Dispose();
_events = null;
}

if (_masterController != null)
{
_masterController.WillAppear -= MasterControllerWillAppear;
_masterController.WillDisappear -= MasterControllerWillDisappear;
}

_disposed = true;
}
base.Dispose(disposing);
if (_disposed)
{
return;
}

_disposed = true;

if (disposing)
{
if (Element != null)
{
PageController.SendDisappearing();
Element.PropertyChanged -= HandlePropertyChanged;

if (MasterDetailPage?.Master != null)
{
MasterDetailPage.Master.PropertyChanged -= HandleMasterPropertyChanged;
}

Element = null;
}

if (_tracker != null)
{
_tracker.Dispose();
_tracker = null;
}

if (_events != null)
{
_events.Dispose();
_events = null;
}

if (_masterController != null)
{
_masterController.WillAppear -= MasterControllerWillAppear;
_masterController.WillDisappear -= MasterControllerWillDisappear;
}

ClearControllers();
}

base.Dispose(disposing);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is reshuffling to reduce nesting, right? or is there any change in it ?

Copy link
Contributor Author

@hartez hartez Oct 10, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fix to the Dispose method not marking the item as _disposed = true when running from the finalizer, plus it adds the ClearControllers() call that was missing from the tablet version.

}

public VisualElement Element { get; private set; }
Expand Down