microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.36k stars 678 forks source link

x:Bind initialization timing conflict when used in ResourceDictionary - NullReferenceException #5786

Open michael-hawker opened 3 years ago

michael-hawker commented 3 years ago

Describe the bug I somehow stumbled upon a NullReferenceException in the generated x:Bind code when the element containing the binding was null. However, it only occurs in a certain scenario when the resource is not directly referenced from outside the ResourceDictionary. i.e. if the resource is only referenced from within the resource dictionary the binding to it fails; however, when the resource is used within the main visuals below it, the binding works.

However, it seems to only reproduce with specific sets of XAML trees as well. There's definitely some weird timing/initialization issue going on here.

Steps to reproduce the bug Steps to reproduce the behavior:

  1. Install the Microsoft.Toolkit.Uwp.UI package version 7.1-rc1 or later.
  2. Create a page with the following resources:
  xmlns:ui="using:Microsoft.Toolkit.Uwp.UI"/>

  <Page.Resources>
    <ui:AttachedDropShadow
      x:Key="CommonShadow"
      CastTo="{x:Bind ShadowTarget}"
      Offset="4" />

    <Style TargetType="Button">
      <Setter Property="ui:Effects.Shadow" Value="{StaticResource CommonShadow}" />
      <Setter Property="HorizontalAlignment" Value="Center" />
      <Setter Property="Background" Value="Red" />
    </Style>
  </Page.Resources>
  1. This is a base working case for the page content (as expected):
  <Grid>
    <Grid x:Name="ShadowTarget" />
    <Image
      Width="100"
      Height="100"
      ui:Effects.Shadow="{StaticResource CommonShadow}"
      Source="ms-appx:///Assets/Photos/Owl.jpg" />
  </Grid>
  1. There are a few different scenarios which can show the problem occurring, this is a minimal (if not practical) version of it which shows the same failure line:
  <Grid>
    <Grid x:Name="ShadowTarget" />
    <!--  Uncomment the button and it works  -->
    <!--<Button
      Margin="0,8,0,0"
      VerticalAlignment="Top"
      Content="Hello" />-->
    <Image
      Width="100"
      Height="100"
      Source="ms-appx:///Assets/Photos/Owl.jpg" />
  </Grid>

If you uncomment out the button, then the binding is successful.

  1. In this more complex tree, it seems to fail even though the button implicitly is still referencing it like the above example. I'm not sure about what this tree causes the difference:
  <ScrollViewer>
    <Grid>
      <Grid x:Name="ShadowTarget" />
      <StackPanel VerticalAlignment="Center" Spacing="32">
        <Button Content="I Have a Shadow!" />
        <Image
          Width="100"
          Height="100"

          Source="ms-appx:///Assets/Photos/Owl.jpg" />
        <Rectangle
          Width="200"
          Height="100"

          Fill="#80FF0000"
          RadiusX="4"
          RadiusY="4" />
        <Button Content="I Also have a Shadow!" />
      </StackPanel>
    </Grid>
  </ScrollViewer>

This now fails even though it appears similar to step 4 scenario when the button is uncommented. If you add the ui:Effects.Shadow="{StaticResource CommonShadow}" property to the blank lines in the first Image and/or the last Rectangle then the sample works fine again! 🤷‍♂️

  1. It appears to work if you remove the ScrollViewer...?

Expected behavior Binding doesn't throw null reference exception, still should bind...

Screenshots image

image

Version Info

NuGet package version: None, WUX OS XAML UWP

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

Additional context Discovered in final testing of our Attached Shadow Toolkit PR here: https://github.com/CommunityToolkit/WindowsCommunityToolkit/pull/4179

StephenLPeters commented 3 years ago

@RealTommyKlein can you weigh in?

michael-hawker commented 3 years ago

Hmmm, was just looking at a WinUI 3 (internal) Bug around Binding and Event Loading, though not sure if it's supposed to effect x:Bind as well, but also for UWP. Wonder if this is one of those UWP type cases where Binding/Loading is non-deterministic...?

However, it's weird as it either always seems to succeed or always fail, but just depends on the composition of the Visual Tree...

FYI @evelynwu-msft too.

evelynwu-msft commented 3 years ago

This feels like a completely different issue from 32092234.

RealTommyKlein commented 3 years ago

This is from the framework optimizing incorrectly, and not creating the CommonShadow object when it's actually needed. Xaml defers creating ResourceDictionary entries until they're actually referenced/needed (e.g. from a StaticResource reference or someone accessing ResourceDictionary entries in code-behind).

In the examples, Step 3 works because the Image has a StaticResource reference to CommonShadow object, so the framework will create it. Step 4 doesn't work when the Button is commented because nothing in the code actually references CommonShadow. However, when the Button is uncommented, the implicit style for Button defined in the dictionary needs to be applied (and therefore instantiated itself) - when the implicit style is made, its ui:Effects.Shadow setter references CommonShadow, causing CommonShadow to be created and making everything work again.

One workaround is to use x:Name for CommonShadow instead of x:Key, which will force the framework to instantiate it. However, the framework should really be force instantiating any objects with x:ConnectionId on them (which are secretly added by the Xaml compiler before the .xaml is fed to the parser - in this case CommonShadow would have one). x:ConnectionId indicates some Xaml compiler generated code-behind needs them (e.g. x:Bind).

evelynwu-msft commented 1 year ago

We already detect if x:Name is present on a resource and force it to be undeferred so it should be straightforward to extend the same logic to x:ConnectionId.