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

[Android] Fix content pages not Garbage collected #14717

Merged
merged 3 commits into from
Jan 7, 2022
Merged

[Android] Fix content pages not Garbage collected #14717

merged 3 commits into from
Jan 7, 2022

Conversation

danielkraut
Copy link
Contributor

Description of Change

Changes I am certail of that fixes (at least sometimes, read next paragraph) content page instances not garbage collected:

  • ShellToolbarTracker - calling RemoveDrawerListener(...) in dispose method
    • after adding this the simplest content page with no content were garbage collected
  • ShellContentFragment - explicitly setting page TitleView to null
    • this change allows pages to be collected when a page has set custom content via Shell.TitleView (you need to add at least some layout with content, e.g. StackLayout with Label)
    • This is my first time browsing throught Xamarin.Forms codebase and I have no idea if this is the best place to set title to null, I would appreciate other devs feedback but it certainly fixed my issue

Those changes allow simple pages to be collected by GC. By simple I mean pages with default controls (buttons, labels, etc.) and with custom TitleView, but in my application I have used third-party controls (Syncfusion) and those pages are still not collected. I am certain it is a Shell issue because using those controls in non-Shell application seems fine.

Other changes don't necessarily affect my issue but are something I noticed when debugging Xamarin Forms. Calling _shellPageContainer.Dispose(); is must-have in my opinion but calling RemoveView/RemoveAllViews is something I need more feedback from other devs if it is necessary or not.

Issues Resolved

#14657 - It doesn't fix the issue completely, more investigation is still required.

API Changes

None

Platforms Affected

  • Android

Behavioral/Visual Changes

Applications should use less memory because simple page instances will be garbage collected.

Before/After Screenshots

Not applicable

Testing Procedure

I was able to test only Android platform (Android 10 and 11).

In generall, just implement a finalizer on any page that is created and destroyed on the fly, i.e. a page when navigated to, a new instance is created unlike a pages which are added to flyout menu, those are usually have single instance for the lifetime of application. That finalizer is never called.

Also I have created automatic test to demonstrated the issue. Callign

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

removing drawer listener (_drawerToggle) allows shell pages to be collected by GC

issue #14657
Shell TitleView is explicitly set to null, because otherwise pages would not be disposed of. Also views are explicitly removed during disposal.

issue #14657
@net-foundation-cla
Copy link

net-foundation-cla bot commented Oct 11, 2021

CLA assistant check
All CLA requirements met.

@danielkraut danielkraut changed the title Fix 14657 [Android] Fix content pages not Garbage collected Oct 11, 2021
@danielkraut
Copy link
Contributor Author

Thank you @jsuarezruiz for the approval, although I hope you've read the Description of change because this is my first time looking at the Xamarin codebase, so any suggestions from more experienced devs would be much appreciated.

Also I didn't find in the documentation how to run the CI pipeline so maybe someone with know-how and privileges could run it for me?

@jfversluis
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jfversluis
Copy link
Member

Hey @danielkraut! I just triggered the pipeline :) I did read your comments and had a first quick look through your code. It seems to make sense to me so far. Thank you for this!

@jfversluis
Copy link
Member

Hey @danielkraut thanks for your patience on this one! I've been looking at it for a bit and seems all OK to me. If you have been digging into the more complex scenarios, feel free to do a follow up PR. Thanks for all your time and efforts on this one!

@jfversluis jfversluis merged commit d59f34f into xamarin:5.0.0 Jan 7, 2022
@danielkraut
Copy link
Contributor Author

Hey @danielkraut thanks for your patience on this one! I've been looking at it for a bit and seems all OK to me. If you have been digging into the more complex scenarios, feel free to do a follow up PR. Thanks for all your time and efforts on this one!

Hi @jfversluis, yes I have been doing much more and it has been time consuming. I could not wait for this to be merged and actually did not most of the Shell advantages so in the mean time I have rewritten our application without the Shell. I have found that also not all pages are being GC'd. I though I would provide another PR but have found this - once you set NavigationPage/Shell.TitleView to any "complex" view (StackLayout, Grid, etc.) your pages will not be GC'd in Debug build. In Release build they eventually will be, but you have to make sure to do a clean and then build (I also manually removed the obj folder and uninstalled the application from target device just to be sure, this method worked) every time, otherwise this might not work. That process is quite slow as you might imagine. Our CI creates new build when we deploy our app so this is not a real problem in production but it is quite painful to debug. Also this problem starts from XF 5.0.0.2021 so my guess is it is related to XAML hot reload. I don't know how would I troubleshoot hot reload issues so I eventually gave up.

Summary - ContentPages are not garbage collected if you:

  • Use Xamarin Forms 5.0.0.2012 or higher, and
  • use Debug build, and
  • set TitleView to any Layout or other complex View

In Release build they should be but make sure you create clean build to be sure.

@jfversluis Since we don't use XF Shell any more I will not test changes in this PR in production but I think that you can close issue #14657, if GC releated issues are okay in debug build.

@jfversluis
Copy link
Member

Thank you so much for your detailed explanation and investigation Daniel, very much appreciated! I hope you will stick with us for .NET MAUI and continue to be a contributor this way :)

@sahurgithub
Copy link

sahurgithub commented Jun 16, 2022

I'm not sure if anyone else is having this issue but the change on line 173 seems to be removing the TitleView from the Page. This gets triggered when switching tabs as well and when the user goes back to a tab which has a TitleView, its now gone because the code set it to null.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants