xamarin / Xamarin.Forms

Xamarin.Forms is no longer supported. Migrate your apps to .NET MAUI.
https://aka.ms/xamarin-upgrade
Other
5.63k stars 1.87k forks source link

Bug 60966 - Can't Navigate To The Same Page Twice #1897

Open AceCoderLaura opened 6 years ago

AceCoderLaura commented 6 years ago

COPIED FROM: https://bugzilla.xamarin.com/show_bug.cgi?id=60966

Laura:

Hi, I've created a sample project that demonstrates a bug with the navigation page. It is impossible to navigate to the same page twice. Why is this? Navigating to a new Page object every time I want to go back to the same page is hurting my application's performance.

https://www.dropbox.com/s/c8aqloj60vpvg7h/NavigationCircleBug.7z?dl=0

Melbourne Developer:

This is a colossal monster bug that leaves us flailing and gasping for air.

How are we supposed to build an app where the pages keep getting recreated again and again? It's chewing memory like you wouldn't believe.

We need to be able to navigate to a page twice so that it can be reused. We know that this is possible because you can hit the back button to go back. So why I can't we just navigate to a page twice or more?

jassmith commented 6 years ago

Any reason you can't remove pages that are currently in the back stack before trying to add them to the back stack a second time?

What your demo does is what I would call continuous forward navigation. The app never expects the user to hit "back" because the navigation tree has cycles. You need to break the cycles by removing the old page before adding it to the backstack again.

AceCoderLaura commented 6 years ago

@jassmith Of course we could remove it from the back stack, but that would ruin the backwards navigation, we've tried this in practise and it makes navigating the application confusing.

This is not a long-term solution.

jassmith commented 6 years ago

Unfortunately this is a straight up limitation coming from the underlying toolkit. Navigation.Push maps to this call here: https://developer.apple.com/documentation/uikit/uinavigationcontroller/1621887-pushviewcontroller

As you can see they plainly state that attempting to push a view controller which is already on the back stack will throw an exception. So to support this concept I would have to do the following:

1) Automatically remove the native view from the backstack without removing it from the Forms back stack. (This will be a pretty decent hack on its own)

2) Insert the native view into the UINavigationController using the push method as requested, now that it is cleared from the back stack.

3) Then when the native view is popped for whatever reason, re-insert it in the most recent location in the backstack it previously was at.

This can all of course then happen recursively, which gets stupid pretty quick from an implementation standpoint.

When iOS and Android both support this concept properly (UWP already does) we will implement it.

MelbourneDeveloper commented 6 years ago

@jassmith , I see your point, but this doesn't change the fact that there is an issue and there needs to be a solution to this problem. We will rephrase the problem in a different way and open a new ticket. The basic issue is this:

To mint a new page we have to create a bunch of resources. It costs performance to do so, and then when we are finished with the page, we need to unhook any object references in order to stop memory leaks from occurring. Experience has shown this is no trivial matter in .NET and Silverlight. We've had to use the most sophisticated memory profilers to find memory leaks in controls etc. that are not supposed to leak, but do. There are no such profilers for Android, or iOS, and we are yet to find one for UWP.

We absolutely MUST recycle pages. It's the only feasible strategy from a performance standpoint, and a memory usage standpoint.

jassmith commented 6 years ago

Please dont file a new bug, I will re-open this bug if you modify it better match what would be a reasonable suggestion.

jassmith commented 6 years ago

Here is my suggestion:

1) We add an API lets say

Page.SetRendererRetainMode (RendererRetainMode.Retain);

public enum RendererRetainMode {
  Default,
  Retain,
}

Then you can do the following:

Page.SetRendererRetainMode (RendererRetainMode.Retain);
navPage.Navigation.RemovePage (myPage);
await navPage.Navigation.PushAsync (myPage);

You can subscribe to events on the NavigationPage to see when myPage is popped and re-insert it at its last location. You will need to do that bookkeeping yourself, however this will be much faster because there is no renderer recreation.

That said, if you try to do this where you push the same page twice in a row this will result in very strange behavior and I strongly suggest you avoid letting that happen. This is also why we will not be supporting that kind of behavior generally. If you want to hack a feature into the base toolkit that it doesn't support like that, fine, but the best I think we can offer is to add a renderer retention mechanism.

It is also on you to reset the RetainMode back to default or the renderer will be leaked memory.

MelbourneDeveloper commented 6 years ago

@jassmith , I've read over this a few times. Essentially what you are suggesting here is really what @AceCoderLaura has been asking for. The only difference being that we flag the page with a value of RendererRetainMode.Retain . I guess that would be OK, but it seems like a bit of overkill. Surely, you know if a page has ever been rendered before, and if the programmer is trying to Push it again, they obviously want to navigate to it, so isn't this a bit redundant?

MelbourneDeveloper commented 6 years ago

@jassmith , really, the question is more like this:

If you were a new developer coming in to Xamarin Forms now, and you had these requirements:

How would you go about it?

Surely other people would be facing this issue. We're talking about devices with limited resources. It doesn't make sense to create objects again and again only have them be destroyed by the garbage collector. When you are working with limited resources, the principle is to always recycle. Currently, Xamarin Forms really steers developers away from this pattern when it should be encouraging them to recycle as much as possible.

AceCoderLaura commented 6 years ago

We want singleton pages without sacrificing the backwards navigation.

programmation commented 6 years ago

I think the fact that Xamarin Forms does page-based navigation instead of viewmodel-based navigation may be working against it here. Navigating by viewmodel, you can cache pages (much as ListView caches cells) and simply (re-)apply the viewmodel binding context while maintaining back stack out of the viewmodels. XF's page-based navigation makes that much harder because it's starting at the wrong end of the equation (the page).

jassmith commented 6 years ago

bold italics just make this harder for me to read, please stop that...

You are all worried about how Xamarin.Forms should work if it were a standalone toolkit, it is not. It is however fundamentally limited by the behavior of the toolkits it is a projection of.

I will attempt to address your concerns here but let me state a guiding principle. Xamarin.Forms must strive to never "hack" features into an underlying toolkit. We have done so in the past, every single time it has backfired gloriously when one of a hundred bad things happens. The most common of which is eventually the host toolkits implement that feature in a fundamentally different way and now you're stuck.

@MelbourneDeveloper

I guess that would be OK, but it seems like a bit of overkill. Surely, you know if a page has ever been rendered before, and if the programmer is trying to Push it again, they obviously want to navigate to it, so isn't this a bit redundant?

That's not what I am proposing. I am proposing an API to you that allows you to remove a Page from the live tree (Parent.Parent.Parent.Parent no longer traces back to the Application before hitting null) without Xamarin.Forms disposing of its renderers, thus forcing them to be recreated when you turn around and immediately add it to the tree again. There are actually quite a few other cases where this kind of API would be useful, many similarly related to this issue.

What you do with that API is up to you. I simply described how I would use it to achieve your two stated goals (full back functionality + no unneeded page creation). I am essentially proposing a model whereby you could create a pool of pages, and continually reuse them over and over and over without new renderers or pages needing to be created.

Yes I am insisting the management of the backstack be left up to you, but fortunately there are APIs already for you to do that. Why does forms not do this you ask? Removing a page from the backstack to insert it further down the line has side effects. Those side effects depending on exactly what you do could result in visual glitches (flickering) or layouting updates. Every entry into the backstack must be thought of as an independent parenting of the page as it might be called upon to be displayed twice at once.

This presents another issue, which is that X.F. strictly requires that a view have a singular parent. There is simply no real way around this, single parentage is a fundamental requirement of Forms. It also happens to be a fundamental requirement of all major platforms and every toolkit I am aware of in existence. This is not to be confused with the concept of unguided navigation (think UWP nav) which I discuss further down.

Instead what I would rather do is give you the tools needed to implement a behavioral pattern the iOS docs explicitly call out as bad, and let you deal with the consequences of doing so.

@AceCoderLaura

We want singleton pages without sacrificing the backwards navigation.

What I have proposed gives you the capacity to implement that fully. Full stop, no sacrifice. Yes we don't do it for you, but all you have to do is keep track of a List<Page> and make sure that the backstack always looks like the List<Page> minus the leading duplicates, which again there are API's for you to do that with.

@programmation

I think the fact that Xamarin Forms does page-based navigation instead of viewmodel-based navigation may be working against it here. Navigating by viewmodel, you can cache pages (much as ListView caches cells) and simply (re-)apply the viewmodel binding context while maintaining back stack out of the viewmodels. XF's page-based navigation makes that much harder because it's starting at the wrong end of the equation (the page).

Such is the life of living with mobile. iOS and Android both use this model and show no signs of changing. Android technically can do both actually, but thats neither here nor there as they realize views dynamically. It would be more accurate to say that Push Model navigation (how Forms does navigation) is a strict subset of unguided navigation (what you call viewmodel based navigation). Forms intentionally limits its navigation model to better map to the push navigation model present on the dominant mobile platforms.

That said, it is 100% possible to implement a Page, lets say call it FramePage (because we stupidly have something called Frame already), which instead of working based on push navigation instead works unguided navigation. You wouldn't be able to cleanly map to things like UINavigationController. The question of animations and transitions would be an interesting one, and certainly I am very open to seeing this implemented (even if iOS would be a nightmare to semantically map). The advantage of a FramePage is it would allow "spaghetti" navigation in the sense that you can go from any page to any page and back to a third page if you want. It doesn't maintain a true parented back stack like a NavigationPage does (all pages in the back stack are considered Live and Parented in a NavigationPage, even if they are not currently shown or realized).

programmation commented 6 years ago

@jassmith That was not what I was referring to. In mentioning ViewModel-based navigation I was not thinking about undirected travel around an app's various views. Instead I was suggesting that in the context of a navigation that we want to operate like a stack, ViewModel-based navigation makes it easier to push a ViewModel onto the stack, and then decide how to display it - either by creating a Page+Renderer to bind to, or re-using a Page+Renderer from a cache and binding to that. However after thinking about it in the iOS context it's not clear to me how one would implement this in a sufficiently generic way to make putting it into Xamarin Forms worthwhile. I still think that ViewModel-based navigation gives more flexibility, but unless Xamarin Forms adds more MVVM features it's hard to see how something like this could be achieved. The ViewModel stack would be the master view of the world, but Xamarin Forms would have to then manage the push/pop of Pages+Renderers to achieve the right visual effect while at the same time maximising the re-use of Page+Renderer combinations. It's unfortunate that there are still so many internal and sealed classes in Xamarin Forms since they make it quite difficult for anyone not working directly in the Xamarin Forms code to shape it to what's needed for this to work. That said, I don't yet see how it would be possible to get away with less than 2 of each unique Page+Renderer combination, since the push/pop mechanism always works with pairs of (e.g.) UIViewControllers.

MelbourneDeveloper commented 6 years ago

@jassmith

Instead what I would rather do is give you the tools needed to implement a behavioral pattern the iOS docs explicitly call out as bad, and let you deal with the consequences of doing so.

This doesn't fill me with much confidence. I'm also worried about further bloating and bifurcating the Xamarin Forms codebase. I do understand that there are underlying constraints that you have to work with. But, Xamarin Forms is a layer on top of the lower level components, so it needs to be robust and uniform across platforms.

Currently, our XF UWP app leaks memory like crazy. This is mostly because inside our app XAML is rendered dynamically, and we do not know ahead of time which controls need to have their event handlers and binding etc. unhooked before the page can properly be destructed. In Silverlight, we wasted countless man weeks using profilers to find where object meshes had not been properly broken. If we don't have a way to recycle pages, we're going to have to go back to this scenario again, and spend hours and hours chasing event handlers that have not been unhooked. This is simply not possible when we are talking about three different platforms. This issue may not be very apparent in small scale XF apps right now, but as your customer's apps get larger, and Xamarin programmers start shooting toward more large scale enterprise level apps as we are, you will see how much of a problem memory management is.

Anyway, as for your suggestion, we'd be happy if you would look in to something of this nature, but it's not worth doing if the attitude is:

give you the tools needed to implement a behavioral pattern the iOS docs explicitly call out as bad, and let you deal with the consequences of doing so

That's not going to help Microsoft, and that's not going to help us. There needs to be a robust solution that does not go outside the normal confines of XF functionality.

I think that you should talk this over with others in your team, and take the time to scope a solution that will benefit everyone - not just an extra option to do something that's not recommended. You know that this will just end up creating more headaches than it's worth.

jassmith commented 6 years ago

@MelbourneDeveloper

But, Xamarin Forms is a layer on top of the lower level components, so it needs to be robust and uniform across platforms.

Thats exactly why we support push navigation rather than undirected navigation. We can uniformly support push navigation.

That's not going to help Microsoft, and that's not going to help us. There needs to be a robust solution that does not go outside the normal confines of XF functionality.

Our stance on this is firm. We will not be supporting features wildly against what the underlying toolkits support. I am trying to work with you to get what you want, but I do need to work with you to come to an acceptable point where we can commit to supporting it and you are sure it lets you build what you need.

I think that you should talk this over with others in your team, and take the time to scope a solution that will benefit everyone - not just an extra option to do something that's not recommended. You know that this will just end up creating more headaches than it's worth.

I have. The problem is as best I can tell you are simply asking us to hide from you the fact that you are doing something not recommended. It would still be doing something not recommended to iOS. That recommendation comes from them, not us. Just because we add first party support for doing something not recommended to a platform doesn't all of a sudden make it better.

jassmith commented 6 years ago

@programmation Ah I see what you meant now. Yes we are not going to add an MVVM toolkit which is what would be required to implement what you are speaking of. I have pushed for that to be part of a Xamarin.Forms.MVVM nuget many times but generally always get rebuked. If we did have such a thing we could make what you are speaking of a reality which would help a bit.

MelbourneDeveloper commented 6 years ago

@jassmith take a deep breath and go back a few steps. I understand that this original bug report was about navigating to the same page twice. But, that's not the underlying issue. The underlying issue is that because we can't navigate to a page twice, we have to insure that the object mesh is completely broken when a page is removed from the back stack. If we don't, memory leaks will ensue. We know this because we've dealt with this for years in Silverlight. This has created weeks and weeks of man hours looking for a needle in a haystack. It also creates a tonne of performance problems because components need to be constantly created and destroyed.

We don't necessarily need to recycle pages again and again. But, if we do not, we're going to need some alternative that doesn't involve scanning through visual tree and unhooking event handlers and so on in a scorched earth approach as we've had to do in Silverlight.

Our app leaks memory badly in UWP. Our app doesn't leak too much memory on Android, but the finalizer on many of our objects are not getting called, so there is no doubt we have a problem. I can open a separate ticket and create a sample app to display how this is an issue, and how this will affect many other Xamarin Forms developers, but I shouldn't need to because this is only common sense. The issue is obvious. We do not want to get caught up on one solution which is to recycle pages. Recycling pages would be a possible solution to the problem, but it's not the only solution.

To reiterate, the problem is about memory leakage. It is there. It is a real problem, and other developers will be experiencing the same problem. If we can't recycle pages, we need another way to deal with the issue. I'm happy to document this up in more detail as a separate issue if necessary.

jassmith commented 6 years ago

To reiterate, the problem is about memory leakage.

I know, I want to give you the ability to re-use pages. I just want you to have to be explicit about it.

Our app leaks memory badly in UWP.

I would like to figure out why. We go pretty scorched earth on renderers when they are no longer used.

AceCoderLaura commented 6 years ago

I just want to re-use my pages. Why does this have to be so difficult?

AceCoderLaura commented 6 years ago

What is happening with this issue?

MelbourneDeveloper commented 6 years ago

@jassmith

I would like to figure out why. We go pretty scorched earth on renderers when they are no longer used.

How is the garbage collector supposed to collect memory if all the pages are in the back stack with all their event handlers in tact?

PureWeen commented 6 years ago

But that's different than leaks memory badly that's just making the decision to retain something in memory

Leaking would be if there was left over memory when those renderers were disposed of meaning that each time they were recreated and then disposed of it would cause a gradually increasing amount of allocations. If this is happening on UWP that's really bad and we'd like to fix it.

The idea with keeping the pages on the back stack and telling them not to dispose of the renderers is just saying hey keep this stuff in memory meaning the overall allocation will just remain constant opposed to them being disposed, collected, created, disposed, collected, etc...

jassmith commented 6 years ago

How is the garbage collector supposed to collect memory if all the pages are in the back stack with all their event handlers in tact?

Well it sounds like you are leaving them in the back stack then... Im not sure what you expect at that point. However I realize you need to see some sort of progress and unfortunately we are at an impasse between you and the dev team.

The reality of the situation is you are asking for a feature we have no intention to deliver. While I think there are many viable ways to achieve what you want given the proposed API above, this does not seem to be to your liking. Your next stop therefor is further up the chain from me. I have alerted my management to the existence of this issue.

MelbourneDeveloper commented 6 years ago

@jassmith , there's no need to get hostile.

We framed the issue in the wrong way from the ground up. We attempted to solve an issue by recycling pages. That turned out to be impossible, so we logged a ticket. You've brought up some reasons as to why recycling pages is not a good idea, and that's fair enough. We should have framed the issue from the point of view of how difficult it is to get the garbage collector to finalize objects once the app is finished with them. If you go back over my original comments, you will see that what I am talking about is how hard it is to scan through the visual tree once objects have been created and to detach all of the object linkages.

@PureWeen , fair point. But, the issue, as I have outlined above, is that simply popping pages out of the back stack is not going to solve the issue either. As I've mentioned, we've been battling this same issue in Silverlight for almost a decade and the same problem occurs there despite writing tonnes of code to scan through the visual tree to break object linkages. We're now abandoning Silverlight, but the same issues exist in XF as you can see in ticket #2399 .

When I get a chance, I will document up the issue in a new ticket. For my two cents, I still believe that recycling pages would be the best strategy to deal with the issue because it saves on performance, and it's not necessary to scan through the visual tree to break up object linkages when the page is finished with. But, if this is not feasible, it doesn't change the fact that by default, breaking object linkages is extremely onerous at best, and unless the developers are very savvy about what can a memory leak, the app will leak memory.

MelbourneDeveloper commented 6 years ago

@jassmith ,

I realize that what I need to do is create some sample apps to demonstrate the problem(s). Memory leakage is a broad issue and it can't be solved with one simple brush stroke. However, recycling pages would go a long way toward alleviating this issue.

So, my question is: is recycling pages feasible or not? You mentioned that it might, but it would have to be done in an explicit way. Is this an option still on the table?

If I can show you the extent of the problem with samples, would it sway you on whether or not recycling pages could be a possible recommended approach to help avoid memory leaks?