From 1cd900c856a80b370d3dad7e890a2a1654617599 Mon Sep 17 00:00:00 2001
From: Steven Kirk <grokys@gmail.com>
Date: Mon, 8 Apr 2019 18:39:39 +0200
Subject: [PATCH] Prevent leak when using TemplateBinding from a Setter.

When a `TemplateBinding` is used as a `Setter.Value`, then we need to make sure we don't use the `TemplateBinding` itself as the binding because this can cause a memory leak.

Replace `IRequiresTemplateInSetter` with `ISetterValue` which can be used to require a template in the setter for `Control` and can also be used to notify `TemplateBinding`  that it's in a setter and so should always clone itself.

Fixes #2420
---
 src/Avalonia.Controls/Control.cs                | 10 +++++++++-
 .../Styling/IRequiresTemplateInSetter.cs        | 10 ----------
 src/Avalonia.Styling/Styling/ISetterValue.cs    | 13 +++++++++++++
 src/Avalonia.Styling/Styling/Setter.cs          |  8 +-------
 .../Avalonia.Markup/Data/TemplateBinding.cs     | 17 ++++++++++++-----
 tests/Avalonia.Styling.UnitTests/SetterTests.cs | 14 ++++++--------
 6 files changed, 41 insertions(+), 31 deletions(-)
 delete mode 100644 src/Avalonia.Styling/Styling/IRequiresTemplateInSetter.cs
 create mode 100644 src/Avalonia.Styling/Styling/ISetterValue.cs

diff --git a/src/Avalonia.Controls/Control.cs b/src/Avalonia.Controls/Control.cs
index a7ee027e70..ca5edae4a9 100644
--- a/src/Avalonia.Controls/Control.cs
+++ b/src/Avalonia.Controls/Control.cs
@@ -1,6 +1,7 @@
 // Copyright (c) The Avalonia Project. All rights reserved.
 // Licensed under the MIT license. See licence.md file in the project root for full license information.
 
+using System;
 using System.ComponentModel;
 using Avalonia.Controls.Primitives;
 using Avalonia.Controls.Templates;
@@ -20,7 +21,7 @@ namespace Avalonia.Controls
     ///
     /// - A <see cref="Tag"/> property to allow user-defined data to be attached to the control.
     /// </remarks>
-    public class Control : InputElement, IControl, INamed, ISupportInitialize, IVisualBrushInitialize, IRequiresTemplateInSetter
+    public class Control : InputElement, IControl, INamed, ISupportInitialize, IVisualBrushInitialize, ISetterValue
     {
         /// <summary>
         /// Defines the <see cref="FocusAdorner"/> property.
@@ -90,6 +91,13 @@ namespace Avalonia.Controls
         /// <inheritdoc/>
         bool IDataTemplateHost.IsDataTemplatesInitialized => _dataTemplates != null;
 
+        /// <inheritdoc/>
+        void ISetterValue.Initialize(ISetter setter)
+        {
+            throw new InvalidOperationException(
+                "Cannot use a control as a Setter value. Wrap the control in a <Template>.");
+        }
+
         /// <inheritdoc/>
         void IVisualBrushInitialize.EnsureInitialized()
         {
diff --git a/src/Avalonia.Styling/Styling/IRequiresTemplateInSetter.cs b/src/Avalonia.Styling/Styling/IRequiresTemplateInSetter.cs
deleted file mode 100644
index 2d562b78d9..0000000000
--- a/src/Avalonia.Styling/Styling/IRequiresTemplateInSetter.cs
+++ /dev/null
@@ -1,10 +0,0 @@
-namespace Avalonia.Styling
-{
-    /// <summary>
-    /// This is an interface for advanced scenarios to assist users in correct style development.
-    /// You as a user will not need to use this interface directly.
-    /// </summary>
-    public interface IRequiresTemplateInSetter
-    {
-    }
-}
diff --git a/src/Avalonia.Styling/Styling/ISetterValue.cs b/src/Avalonia.Styling/Styling/ISetterValue.cs
new file mode 100644
index 0000000000..63c544cf7d
--- /dev/null
+++ b/src/Avalonia.Styling/Styling/ISetterValue.cs
@@ -0,0 +1,13 @@
+namespace Avalonia.Styling
+{
+    /// <summary>
+    /// Customizes the behavior of a class when added as a value to an <see cref="ISetter"/>.
+    /// </summary>
+    public interface ISetterValue
+    {
+        /// <summary>
+        /// Notifies that the object has been added as a setter value.
+        /// </summary>
+        void Initialize(ISetter setter);
+    }
+}
diff --git a/src/Avalonia.Styling/Styling/Setter.cs b/src/Avalonia.Styling/Styling/Setter.cs
index cfe17a4291..3702259f35 100644
--- a/src/Avalonia.Styling/Styling/Setter.cs
+++ b/src/Avalonia.Styling/Styling/Setter.cs
@@ -65,13 +65,7 @@ namespace Avalonia.Styling
 
             set
             {
-                if (value is IRequiresTemplateInSetter)
-                {
-                    throw new ArgumentException(
-                        "Cannot assign a control to Setter.Value. Wrap the control in a <Template>.",
-                        nameof(value));
-                }
-
+                (value as ISetterValue)?.Initialize(this);
                 _value = value;
             }
         }
diff --git a/src/Markup/Avalonia.Markup/Data/TemplateBinding.cs b/src/Markup/Avalonia.Markup/Data/TemplateBinding.cs
index fab9bdbf55..e64efd9df9 100644
--- a/src/Markup/Avalonia.Markup/Data/TemplateBinding.cs
+++ b/src/Markup/Avalonia.Markup/Data/TemplateBinding.cs
@@ -3,6 +3,7 @@ using System.Globalization;
 using System.Reactive.Subjects;
 using Avalonia.Data.Converters;
 using Avalonia.Reactive;
+using Avalonia.Styling;
 
 namespace Avalonia.Data
 {
@@ -12,8 +13,10 @@ namespace Avalonia.Data
     public class TemplateBinding : SingleSubscriberObservableBase<object>,
         IBinding,
         IDescription,
-        ISubject<object>
+        ISubject<object>,
+        ISetterValue
     {
+        private bool _isSetterValue;
         private IStyledElement _target;
         private Type _targetType;
 
@@ -35,10 +38,11 @@ namespace Avalonia.Data
         {
             // Usually each `TemplateBinding` will only be instantiated once; in this case we can
             // use the `TemplateBinding` object itself as the instanced binding in order to save
-            // allocating a new object. If the binding *is* instantiated more than once (which can
-            // happen if it appears in a `Setter` for example, then just make a clone and instantiate
-            // that.
-            if (_target == null)
+            // allocating a new object.
+            //
+            // If the binding appears in a `Setter`, then make a clone and instantiate that because
+            // because the setter can outlive the control and cause a leak.
+            if (_target == null && !_isSetterValue)
             {
                 _target = (IStyledElement)target;
                 _targetType = targetProperty?.PropertyType;
@@ -106,6 +110,9 @@ namespace Avalonia.Data
             }
         }
 
+        /// <inheritdoc/>
+        void ISetterValue.Initialize(ISetter setter) => _isSetterValue = true;
+
         protected override void Subscribed()
         {
             TemplatedParentChanged();
diff --git a/tests/Avalonia.Styling.UnitTests/SetterTests.cs b/tests/Avalonia.Styling.UnitTests/SetterTests.cs
index 7c1623cf21..898606881a 100644
--- a/tests/Avalonia.Styling.UnitTests/SetterTests.cs
+++ b/tests/Avalonia.Styling.UnitTests/SetterTests.cs
@@ -1,17 +1,15 @@
 // Copyright (c) The Avalonia Project. All rights reserved.
 // Licensed under the MIT license. See licence.md file in the project root for full license information.
 
+using System;
+using System.Globalization;
 using System.Reactive.Subjects;
-using Moq;
 using Avalonia.Controls;
-using Avalonia.Data;
-using Xunit;
-using System;
 using Avalonia.Controls.Templates;
-using Avalonia.Markup.Data;
-using Avalonia.Markup;
-using System.Globalization;
+using Avalonia.Data;
 using Avalonia.Data.Converters;
+using Moq;
+using Xunit;
 
 namespace Avalonia.Styling.UnitTests
 {
@@ -22,7 +20,7 @@ namespace Avalonia.Styling.UnitTests
         {
             var target = new Setter();
 
-            Assert.Throws<ArgumentException>(() => target.Value = new Border());
+            Assert.Throws<InvalidOperationException>(() => target.Value = new Border());
         }
 
         [Fact]