microsoft / microsoft-ui-xaml

WinUI: a modern UI framework with a rich set of controls and styles to build dynamic and high-performing Windows applications.
MIT License
6.38k stars 683 forks source link

Loaded/Unloaded/IsLoaded APIs break down on quick swapping #1900

Open Sergio0694 opened 4 years ago

Sergio0694 commented 4 years ago

Describe the bug

As the title says, I've found a way to reproduce an annoying issue in the FrameworkElement.Loaded and FrameworkElement.Unloaded events, and the FrameworkElement.IsLoaded property. Basically, the two events will fire in a completely incorrect order or will not fire at all, and consequently the IsLoaded property will report a value that does not reflect the state of a given element.

In short:

This is especially an issue because devs are relying on those events to track the lifetime of visual items, eg. to subscribe/unsubscribe to external events, and whatnot. At the moment I've had to come up with specific workarounds in my app Legere to bypass this problem, but it's really not an ideal solution, plus this could be not doable at all in other scenarios where this issue is present.

Steps to reproduce the bug

Steps to reproduce the behavior:

  1. Download this repro: LoadedGlitchRepro.zip
  2. Build and start the app with the attached debugger
  3. Click on any of the command bar buttons

Expected behavior

You should see the output window display the Loaded event last for the second button you clicked. Instead you'll see Unloaded being raised last for the control that you can clearly see being loaded.

Additional steps

  1. Click on some of those buttons at random and just observe the output log. See the screenshot below for an example of the output window. You can see the Loaded and Unloaded events are fired in a completely unreliable and incorrect manner.

Screenshots

image

Version Info

NuGet package version:

Windows 10 version Saw the problem?
Insider Build (xxxxx)
November 2019 Update (18363)
May 2019 Update (18362) Yes
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Mobile
Xbox
Surface Hub
IoT

Additional context

The reason why I have a setup like this is that in Legere I have a series of post views that I reuse, to avoid the overhead of creating a completely new instance every time a post of a given type is loaded. They're initially all stored inside an invisible and collapsed Grid, and lazily loaded. When one view is needed, I load it with FindName(string), then place it in a host Border the user can interact with. When the user opens a post of another type, I remove that view and place it back in the original host (so that FindName will work again, as it needs the element to be in the visual tree for it to find it), then repeat the procedure to find and swap in the new view to use.

ranjeshj commented 4 years ago

@MikeHillberg can provide more details on this. I believe Loaded/Unloaded events are asynchronous events but use different mechanisms internally causing this weird behavior which is very unfortunate.

Sergio0694 commented 4 years ago

Hey @ranjeshj - thank you for your quick reply! That's what I was expecting, some timing issue resulting from those events being handled in an asynchronous manner, glad to hear at least should be easily triaged πŸ˜„.

I think I'll end up refactoring my own code in this scenario - I'm forced to put all my elements in the visual tree from the start because x:DeferLoadingStrategy="Lazy" won't work otherwise anyway, but I might try to get rid of that quick swapping by only having a single host grid containing all the possible items, and simply loading/showing/collapsing them when needed, without moving them around the visual tree. I guess I'll need another way to track the lifetime of each element then, but that's unrelated to this issue.

Anyway, I think this issue is still something that should be looked into, as the docs explicitly say that developers can (and should) rely on those events to track the lifetime of visual elements, so something like this is definitely not something one could or should expect to happen.

Thanks again! 😊

Sergio0694 commented 4 years ago

Never mind, I was being too optimistic there. No matter what I try I still end up falling face first because of this issue. Not removing items from the visual tree after calling FindName still doesn't solve the issue, it seems like FindName is just really messing up with the layout updating process.

image

@ranjeshj You can see in the screen, for instance:

Setting [true], loaded [false]

This is an output from an attached property in my control, which is set to "True" directly from XAML inside that control (precisely, in a ScrollViewer in that control). So if that line is executed it means the control is being loaded. But, as you can see from "loaded [false]", the IsLoaded property still returns false there. I mean that could be fine, maybe the property is assigned before the control is loaded in the visual tree (after calling FindName to create that instance).

"CONTROL UNLOADED!"

That's from a test Unloaded handler directly in that control, which is apparently being unloaded... Before ever being loaded at all, according to the framework πŸ€·β€β™‚

I'll stick to my workaround for now hoping it'll hold, and won't touch anything there until this issue is fixed. But at this point I'm left wondering whether this might have been causing other involuntary issues somewhere else without me noticing, because of those events being raised so unreliably.

MikeHillberg commented 4 years ago

We've done some work to correct these events, but ran into some compatibility issues and not shipped any fix yet.

The Loaded event is raised during a layout/render tick. So if you add an element to the tree, Loaded should raise on the next tick. The Unloaded event is also async -- it also doesn't raise when you remove an element from the tree -- but in this case the notification is posted to the UI thread, which can get dispatched before the next tick. So if you move an element in and out of the tree quickly, the events can get out of order and raise in unmatching numbers.

One workaround you can do is to look at an element's Parent property. When an element isn't in the live tree (the tree that's actually in the Window content), its Parent property is null, even if it's not the root of the tree. (This avoids some reference cycle leaks issues.) For example:

var button = new Button();
var grid = new Grid() { Children = { button } };

Debug.Assert(button.Parent == null);
Root.Children.Add(grid);
Debug.Assert(button.Parent != null);
Root.Children.Remove(grid);
Debug.Assert(button.Parent == null);
ArchieCoder commented 3 years ago

I raised directly the bug within my private channel. Let's see if it can get attention.

riverar commented 3 years ago

Any updates @MikeHillberg?

FYI @jonwis am curious if this bite us in Reunion areas such as app lifecycle.

jonwis commented 3 years ago

FYI @jonwis am curious if this bite us in Reunion areas such as app lifecycle.

App lifecycle (and many WinRT objects) raise events just as soon as they're raised by the underlying system. For example, the "user has locked the screen" event is raised by the shell and detected by Project Reunion's infrastructure. Project Reunion raises the AppLifecycle.ScreenLocked event, calling into the app's registered handler.

App runtimes and handlers have different ways of dealing with this incoming event, mostly related to their threading models. For some apps, the handler is just auto lock = m_lock.lock_exclusive(); m_selfLocked = true;, and this flag is observed by the app the next time it goes to redraw/process/etc. For other apps, it's a co_await this.Dispatcher(); m_button.Visible(false); in which the layout is modified from the dispatcher thread. For some runtimes, event handlers auto-route themselves back to the registering ASTA, so the handler can be just m_selfLocked = true;.

I don't forsee this being a problem with app lifecycle itself. XAML apps listening to an app lifecycle event would use the "dispatcher" mode of operation, to ensure the modifications to the XAML elements happen on the right thread during the next tick. (Which itself may be subject to the eventing issues in this thread, but not caused by Project Reunion or app lifecycle.)

HppZ commented 2 years ago

no fix yet?

gus33000 commented 2 years ago

Is there any news about this issue? Was this fixed in WinUI 3? Is a fix still not considered for downlevel Windows Versions' Windows.UI.Xaml.dll (and not WinUI 3)?

hawkerm commented 2 years ago

Think I'm hitting this with XAML Behaviors attach/detach with the loaded/unloaded events they rely on.

In my scenario I have a behavior attached to an item with a DataTemplate containing a behavior. I drag and transfer the item between two different ItemsRepeater collections (which reference the same template for the items). I see the loaded event of the behavior fire again with the item created in the new collection (with the same element contained in the template), but then immediate see the detached event fire from the unloaded event of the behavior from the old collection... 😟

sjb-sjb commented 2 years ago

This is a real killer issue. There are a lot of cases where you need callbacks / events to be hooked up, or other state maintained, while the element is loaded. That requires the Loaded and Unloaded events to be fired correctly.

Can we get this fixed please?

sjb-sjb commented 1 year ago

Still hoping for a solution to this one!

gus33000 commented 1 year ago

Is this issue being worked on still?

sjb-sjb commented 1 year ago

? Any update on this? This is a real problem for reliably hooking up events etc in the Loaded event and unhooking them in Unloaded, which is a very common pattern. This issue is hurting me badly in terms of software reliability.

Sergio0694 commented 1 year ago

Bumping this (see https://github.com/microsoft/microsoft-ui-xaml/discussions/8638).

torleifat commented 1 year ago

I'm not 100% sure, but I've seemed to hit the same issue where a SwapChainPanel's unload event never or rarely hit when the Page containing it is navigated away from. Or when the application is exited. Would definitely be a relief if we could have some predictability on the order and occurrence of these events.

JesseCol commented 12 months ago

I've marked this as a feature proposal because we'll likely solve this by adding a new API. (it would likely cause compat issues if we changed the timing of the existing events). Thanks!

gus33000 commented 12 months ago

I've marked this as a feature proposal because we'll likely solve this by adding a new API. (it would likely cause compat issues if we changed the timing of the existing events). Thanks!

I appreciate a way to fix this issue is in the works, but I personally have questions on how this could be done with a new api. Would this new api be available on currently supported windows versions where our apps run? (Windows 10 19045 as well as Windows 20348, Windows 11 22000 and Windows 11 22621). Will there be a way to dynamically detect this new api as well so our apps wouldn't crash when running on unpatched systems?

Thanks for the update!

JesseCol commented 11 months ago

Hi! Just to set expectations, we don't have any specific plans to address this soon, I'm mostly trying to categorize our issues.

In WinAppSDK releases (especially minor ones) we try to avoid changing behavior apps may be depending on. If you want your app to sniff to see if an API is present, you can generally use the ApiInformation type for this -- for example, you can use ApiInformation.IsApiContractPresent() to determine if a contract is available. Thanks!

riverar commented 11 months ago

I've marked this as a feature proposal because we'll likely solve this by adding a new API. (it would likely cause compat issues if we changed the timing of the existing events). Thanks!

The original API is behaving differently than documented, so realigning the API to the docs should be the priority here, not catering to some misbehaving apps right? Or is this more of an internal political issue because it breaks something big, like File Explorer?

michael-hawker commented 5 months ago

@Sergio0694 you mentioned in Discord:

Yes. Register Loaded and Unloaded, and from there use VisualTreeHelper.GetParent to check if you actually got loaded or unloaded. Track that and disregard duplicate events. Also call that before the first handler to track the initial state

You wouldn't happen to have a gist of an example of that somewhere or something to post here just so there's an kind of example of the exact pattern you describe for now?

vzhilong commented 2 months ago

Meet the same bug here. Yes you can double check the load state and return the wrong event, but all the children's animations still break down.