Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Sergio0694 opened this issue Jan 28, 2020 · 22 comments
Open

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

Sergio0694 opened this issue Jan 28, 2020 · 22 comments
Labels
area-Lifetime feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Controls Issue for the Controls team wct

Comments

@Sergio0694
Copy link
Member

Sergio0694 commented Jan 28, 2020

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:

  • An element loaded in the tree has Unloaded raised last
  • An element loaded in the tree reports IsLoaded being false (!)

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

  • Windows 10 Pro 18362.592
  • Tried targeting both the SDK 17763 and the SDK 18632

NuGet package version:

  • Microsoft.UI.Xaml 2.1.190606001 (though this shouldn't be relevant here)
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.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jan 28, 2020
@ranjeshj
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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);

@chrisglein chrisglein added area-Lifetime needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) and removed needs-triage Issue needs to be triaged by the area owners labels Mar 6, 2020
@ArchieCoder
Copy link

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

@riverar
Copy link
Contributor

riverar commented Feb 17, 2021

Any updates @MikeHillberg?

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

@jonwis
Copy link
Member

jonwis commented Feb 17, 2021

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
Copy link

HppZ commented Dec 1, 2021

no fix yet?

@gus33000
Copy link

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
Copy link

hawkerm commented Aug 29, 2022

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
Copy link

sjb-sjb commented Nov 15, 2022

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
Copy link

sjb-sjb commented Mar 27, 2023

Still hoping for a solution to this one!

@gus33000
Copy link

gus33000 commented May 4, 2023

Is this issue being worked on still?

@sjb-sjb
Copy link

sjb-sjb commented Jun 24, 2023

? 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
Copy link
Member Author

Bumping this (see #8638).

@bpulliam bpulliam added team-Controls Issue for the Controls team and removed team-Framework labels Aug 22, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the needs-triage Issue needs to be triaged by the area owners label Aug 22, 2023
@torleifat
Copy link

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.

@bpulliam bpulliam removed the needs-triage Issue needs to be triaged by the area owners label Aug 29, 2023
@duncanmacmichael duncanmacmichael added the bug Something isn't working label Nov 3, 2023
@JesseCol JesseCol added feature proposal New feature proposal and removed bug Something isn't working labels Nov 28, 2023
@JesseCol
Copy link

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
Copy link

gus33000 commented Dec 1, 2023

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
Copy link

JesseCol commented Dec 4, 2023

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
Copy link
Contributor

riverar commented Dec 13, 2023

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
Copy link
Collaborator

@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
Copy link

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.

michael-hawker added a commit to zadjii-msft/PowerToys that referenced this issue Dec 13, 2024
Replaces PR #218

IconBox is a custom control that's a ContentControl, it's generic (toolkitable) and should be able to be styled and templated.
It knows how to take an IconSource and create the underlying IconElement as its content.
It can also take any general value as a `SourceKey` and via an implementation of the SourceRequested event, translate a bound general object into the `IconSource` required. This is how caching can be provided by an application as well, for instance (like we'll do here).
This uses the deferred events pattern to await the call to the `SourceRequested` event which may need to load data asynchronously

We create a static x:Bind helper `IconCacheProvider` to encapsulate our shared logic for our eventual Icon cache.

Renamed IconCacheService.xaml.cs -> IconCacheService.cs
Removed old broken behavior (believe ultimate issue was due to instability in loaded/unloaded events, i.e. issue microsoft/microsoft-ui-xaml#1900)
XAML Styler also did its thing...
michael-hawker added a commit to zadjii-msft/PowerToys that referenced this issue Dec 13, 2024
Replaces PR #218

IconBox is a custom control that's a ContentControl, it's generic (toolkitable) and should be able to be styled and templated.
It knows how to take an IconSource and create the underlying IconElement as its content.
It can also take any general value as a `SourceKey` and via an implementation of the SourceRequested event, translate a bound general object into the `IconSource` required. This is how caching can be provided by an application as well, for instance (like we'll do here).
This uses the deferred events pattern to await the call to the `SourceRequested` event which may need to load data asynchronously

We create a static x:Bind helper `IconCacheProvider` to encapsulate our shared logic for our eventual Icon cache.

Renamed IconCacheService.xaml.cs -> IconCacheService.cs
Removed old broken behavior (believe ultimate issue was due to instability in loaded/unloaded events, i.e. issue microsoft/microsoft-ui-xaml#1900)
XAML Styler also did its thing...
zadjii-msft added a commit to zadjii-msft/PowerToys that referenced this issue Dec 13, 2024
Fixes #213
Replaces PR #218

FYI @Ryken100 (thanks for the info and assist in debugging the issue and discussing possible avenues of resolution)
Thanks @zadjii-msft for validating the end path in #218

Before:
```xml
                        <Border x:Name="IconBorder"
                                Grid.Column="0"
                                Width="16"
                                Height="16"
                                Margin="0,0,0,0">
                            <!-- LoadIconBehavior will magically fill this border up with an icon -->
                            <Interactivity:Interaction.Behaviors>
                                <cmdpalUI:LoadIconBehavior Source="{x:Bind Icon, Mode=OneWay}"/>
                            </Interactivity:Interaction.Behaviors>
                        </Border>
```

After:
```xml
                        <cpcontrols:IconBox
                            Grid.Column="0"
                            Width="16"
                            Height="16"
                            Margin="0,0,0,0"
                            SourceKey="{x:Bind Icon, Mode=OneWay}"
                            SourceRequested="{x:Bind help:IconCacheProvider.SourceRequested}" />
```

The IconCacheProvider is the translation layer between having a light-weight control and our specific app's logic/desire for an icon cache, using the deferred event pattern:

```cs
public static partial class IconCacheProvider
{
    private static readonly IconCacheService IconService = new(Microsoft.UI.Dispatching.DispatcherQueue.GetForCurrentThread());

    public static async void SourceRequested(IconBox sender, SourceRequestedEventArgs args)
    {
        if (args.Key == null)
        {
            return;
        }

        if (args.Key is IconDataType iconData)
        {
            var deferral = args.GetDeferral();

            args.Value = await IconService.GetIconSource(iconData);

            deferral.Complete();
        }
    }
}
```

## Details

`IconBox` is a custom control that's a ContentControl, its generic (toolkitable) and should be able to be styled and templated (haven't tested, but no reason it shouldn't as a XAML `ContentControl`, should help @niels9001 a ton). 

It knows how to take an `IconSource` and create the underlying `IconElement` as its content.

It can also take any general value as a `SourceKey` and via an implementation of the `SourceRequested` event, translate a bound general object into the `IconSource` required. This is how caching can be provided by an application as well, for instance (like we'll do here). This uses the deferred events pattern to await the call to the `SourceRequested` event which may need to load data asynchronously

We create a static x:Bind helper `IconCacheProvider` to encapsulate our shared logic for our eventual Icon cache.

Also:
- Renamed IconCacheService.xaml.cs -> IconCacheService.cs 
- Removed old broken behavior (believe ultimate issue was due to instability in loaded/unloaded events, i.e. issue microsoft/microsoft-ui-xaml#1900)
- XAML Styler also did its thing... (some conflict here in config from PowerToys to resolve later, imagine this is also a consequence of us not having CI setup in fork...)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Lifetime feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Controls Issue for the Controls team wct
Projects
None yet
Development

No branches or pull requests