Xamarin.Forms icon indicating copy to clipboard operation
Xamarin.Forms copied to clipboard

[Core] Rework navigation task handling

Open kvpt opened this issue 5 years ago • 5 comments

Description of Change

To synchronize navigation actions, the variable CurrentNavigationTask is used to track the current navigation and await it if necessary. But this variable is never reset and can cause a memory leak. So I added a new variable in the navigation methods which contains the navigation task created by the method. At the end I compare it with the CurrentNavigationTask, if it is equal we can reset it, if not, it is that another navigation overwritten it, in this case it's this method which will do the reset.

I removed the try catch introduced in 2873, because I don't know if it is still necessary and I can't test on iOS. If the tests fails I will re-add it.

Issues Resolved

  • fixes #1429

API Changes

  • None

Platforms Affected

  • Core (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

  • Run the NavigationUnitTest
  • Run the reproduction case in #1429 with the profiler, then :
    • Go to Page2
    • Go back
    • Make a snapshot
    • Go to Live Objects
    • Search for Page2
    • If it succeed no Page2 should be present

PR Checklist

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

kvpt avatar Feb 03 '20 18:02 kvpt

Can you check test Issue9355Test_apple_iphone ?

rmarinho avatar Feb 06 '20 16:02 rmarinho

@rmarinho Sorry, unfortunately I don't have an Apple device to test. I don't know if it is related or not but there is a lot of "Bad Gateway" response from the test server in the failed tests attachment.

kvpt avatar Feb 06 '20 16:02 kvpt

@kvpt, @samhouts Can u point out to a xf-ci nightly build nuget package for us to try out ur fix. I dont see 4.6.0.607+125-pr.9411-sha.8ef9ab9d3-azdo.13494 in VS nuget package manager, I have xf-ci package source set to this https://aka.ms/xf-ci/index.json Thanks

ngalicoco avatar Jun 05 '20 20:06 ngalicoco

@ngalicoco I don't think such build exist. What you can do is to clone the branch and build the package yourself. The procedure is documented here. If you can't do that I will try to find some time this week to create a custom nuget package from the branch.

kvpt avatar Jun 09 '20 23:06 kvpt

@kvpt thanks for the info, any eta for this fix to go into standard xamarin nuget package

ngalicoco avatar Jun 09 '20 23:06 ngalicoco

I think at this point it's safe to say that we won't be taking this for Xamarin.Forms anymore. Thank you all so much for your time and effort on this. It hasn't gone unnoticed!

jfversluis avatar Jun 14 '23 08:06 jfversluis