From 032e339299253eca2d7354e5236c49b2d2a310a0 Mon Sep 17 00:00:00 2001
From: Ranjesh <28935693+ranjeshj@users.noreply.github.com>
Date: Sun, 2 Feb 2020 03:36:28 -0800
Subject: [PATCH] Fix stack overflow with two way x:Bind to NumberBox.Value
 (#1905)

---
 dev/Generated/NumberBox.properties.cpp        |  2 +-
 .../InteractionTests/NumberBoxTests.cs        |  6 +++
 dev/NumberBox/NumberBox.cpp                   | 18 +++++++++
 dev/NumberBox/NumberBox.h                     |  3 ++
 dev/NumberBox/TestUI/NumberBoxPage.xaml       | 11 ++++-
 dev/NumberBox/TestUI/NumberBoxPage.xaml.cs    | 40 +++++++++++++++++--
 6 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/dev/Generated/NumberBox.properties.cpp b/dev/Generated/NumberBox.properties.cpp
index 480ac24d9c..0fd89ac118 100644
--- a/dev/Generated/NumberBox.properties.cpp
+++ b/dev/Generated/NumberBox.properties.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See LICENSE in the project root for license information.
 
 // DO NOT EDIT! This file was generated by CustomTasks.DependencyPropertyCodeGen
diff --git a/dev/NumberBox/InteractionTests/NumberBoxTests.cs b/dev/NumberBox/InteractionTests/NumberBoxTests.cs
index fc7a969b11..86e9d9dda5 100644
--- a/dev/NumberBox/InteractionTests/NumberBoxTests.cs
+++ b/dev/NumberBox/InteractionTests/NumberBoxTests.cs
@@ -426,6 +426,12 @@ public void ValueChangedTest()
                 Verify.AreEqual("NaN", newValueTextBlock.GetText());
                 Verify.AreEqual("100", oldValueTextBlock.GetText());
 
+                Log.Comment("Verify that setting two way bound value to NaN doesn't cause a crash");
+                Button twoWayBindNanbutton = FindElement.ByName<Button>("SetTwoWayBoundValueNaNButton");
+                twoWayBindNanbutton.InvokeAndWait();
+                TextBlock twoWayBoundNumberBoxValue = FindElement.ByName<TextBlock>("TwoWayBoundNumberBoxValue");
+                Verify.AreEqual("NaN", twoWayBoundNumberBoxValue.GetText());
+
                 Log.Comment("Verify that setting value to NaN doesn't have any effect");
                 Button nanbutton = FindElement.ByName<Button>("SetValueNaNButton");
                 nanbutton.InvokeAndWait();
diff --git a/dev/NumberBox/NumberBox.cpp b/dev/NumberBox/NumberBox.cpp
index 39c72f112a..413346104f 100644
--- a/dev/NumberBox/NumberBox.cpp
+++ b/dev/NumberBox/NumberBox.cpp
@@ -49,6 +49,24 @@ NumberBox::NumberBox()
     SetDefaultStyleKey(this);
 }
 
+void NumberBox::Value(double value)
+{
+    // When using two way bindings to Value using x:Bind, we could end up with a stack overflow because
+    // nan != nan. However in this case, we are using nan as a value to represent value not set (cleared)
+    // and that can happen quite often. We can avoid the stack overflow by breaking the cycle here. This is possible
+    // for x:Bind since the generated code goes through this property setter. This is not the case for Binding
+    // unfortunately. x:Bind is recommended over Binding anyway due to its perf and debuggability benefits.
+    if (!std::isnan(value) || !std::isnan(Value()))
+    {
+        static_cast<NumberBox*>(this)->SetValue(s_ValueProperty, ValueHelper<double>::BoxValueIfNecessary(value));
+    }
+}
+
+double NumberBox::Value()
+{
+    return ValueHelper<double>::CastOrUnbox(static_cast<NumberBox*>(this)->GetValue(s_ValueProperty));
+}
+
 winrt::AutomationPeer NumberBox::OnCreateAutomationPeer()
 {
     return winrt::make<NumberBoxAutomationPeer>(*this);
diff --git a/dev/NumberBox/NumberBox.h b/dev/NumberBox/NumberBox.h
index 64304f825f..11cc2b8c95 100644
--- a/dev/NumberBox/NumberBox.h
+++ b/dev/NumberBox/NumberBox.h
@@ -33,6 +33,9 @@ class NumberBox :
 
     NumberBox();
 
+    void Value(double value);
+    double Value();
+
     // IUIElement
     virtual winrt::AutomationPeer OnCreateAutomationPeer();
 
diff --git a/dev/NumberBox/TestUI/NumberBoxPage.xaml b/dev/NumberBox/TestUI/NumberBoxPage.xaml
index d80cc7caa6..6601216286 100644
--- a/dev/NumberBox/TestUI/NumberBoxPage.xaml
+++ b/dev/NumberBox/TestUI/NumberBoxPage.xaml
@@ -57,8 +57,10 @@
             <Button x:Name="SetTextButton" AutomationProperties.Name="SetTextButton" Content="Set text to 15" Click="SetTextButton_Click" Margin="0,4,0,0"/>
 
             <Button x:Name="SetValueButton" AutomationProperties.Name="SetValueButton" Content="Set value to 42" Click="SetValueButton_Click" Margin="0,4,0,0"/>
-            
+
             <Button x:Name="SetValueNaNButton" AutomationProperties.Name="SetValueNaNButton" Content="Set value to NaN" Click="SetNaNButton_Click" Margin="0,4,0,0"/>
+
+            <Button x:Name="SetTwoWayBoundValueNaNButton" AutomationProperties.Name="SetTwoWayBoundValueNaNButton" Content="Set two way bound value to NaN" Click="SetTwoWayBoundNaNButton_Click" Margin="0,4,0,0"/>
         </StackPanel>
 
         <Grid Grid.Column="1">
@@ -113,6 +115,13 @@
                     <TextBlock x:Name="VerticalOffsetDisplayBlock" Text="0"
                             AutomationProperties.Name="VerticalOffsetDisplayBlock"/>
                 </StackPanel>
+
+                <!-- NumberBox two way binding with x:Bind -->
+                <StackPanel>
+                    <controls:NumberBox Header="TwoWayBinding" x:Name="TwoWayBoundNumberBox"
+                              Value="{x:Bind DataModelWithINPC.Value, Mode=TwoWay}" />
+                    <TextBlock x:Name="TwoWayBoundNumberBoxValue" AutomationProperties.Name="TwoWayBoundNumberBoxValue" />
+                </StackPanel>
             </StackPanel>
         </Grid>
 
diff --git a/dev/NumberBox/TestUI/NumberBoxPage.xaml.cs b/dev/NumberBox/TestUI/NumberBoxPage.xaml.cs
index 74c0b30150..92986a2288 100644
--- a/dev/NumberBox/TestUI/NumberBoxPage.xaml.cs
+++ b/dev/NumberBox/TestUI/NumberBoxPage.xaml.cs
@@ -1,18 +1,21 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See LICENSE in the project root for license information.
 
+using Microsoft.UI.Xaml.Controls;
 using System;
+using System.Collections.Generic;
+using System.ComponentModel;
+using Windows.Globalization.NumberFormatting;
 using Windows.UI.Xaml;
 using Windows.UI.Xaml.Controls;
-using Microsoft.UI.Xaml.Controls;
-using Windows.Globalization.NumberFormatting;
-using System.Collections.Generic;
 
 namespace MUXControlsTestApp
 {
     [TopLevelTestPage(Name = "NumberBox")]
     public sealed partial class NumberBoxPage : TestPage
     {
+        public DataModelWithINPC DataModelWithINPC { get; set; } = new DataModelWithINPC();
+
         public NumberBoxPage()
         {
             this.InitializeComponent();
@@ -115,6 +118,12 @@ private void SetNaNButton_Click(object sender, RoutedEventArgs e)
             TestNumberBox.Value = Double.NaN;
         }
 
+        private void SetTwoWayBoundNaNButton_Click(object sender, RoutedEventArgs e)
+        {
+            DataModelWithINPC.Value = Double.NaN;
+            TwoWayBoundNumberBoxValue.Text = TwoWayBoundNumberBox.Value.ToString();
+        }
+
         private void TextPropertyChanged(DependencyObject o, DependencyProperty p)
         {
             TextTextBox.Text = TestNumberBox.Text;
@@ -125,4 +134,29 @@ private void ScrollviewerWithScroll_ViewChanged(object sender, ScrollViewerViewC
             VerticalOffsetDisplayBlock.Text = (sender as Windows.UI.Xaml.Controls.ScrollViewer).VerticalOffset.ToString();
         }
     }
+
+    public class DataModelWithINPC : INotifyPropertyChanged
+    {
+        private double _value;
+        
+        public event PropertyChangedEventHandler PropertyChanged;
+
+        protected virtual void OnPropertyChanged(string propertyName)
+        {
+            PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
+        }
+
+        public double Value
+        {
+            get => _value;
+            set
+            {
+                if (value != _value)
+                {
+                    _value = value;
+                    OnPropertyChanged(nameof(this.Value));
+                }
+            }
+        }
+    }
 }
\ No newline at end of file