Fix leaking of dictionary changes between cloned controls

riganti · Dec 8, 2024 · fcf2744 · fcf2744
1 parent 212c13c
commit fcf2744
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 7 deletions.
diff --git a/src/Framework/Framework/Controls/DotvvmControlProperties.cs b/src/Framework/Framework/Controls/DotvvmControlProperties.cs
@@ -45,12 +45,12 @@ private uint hashSeed
         }
         private bool ownsKeys
         {
-            readonly get => (flags >> 30) != 0;
+            readonly get => ((flags >> 30) & 1) != 0;
             set => flags = (flags & ~(1u << 30)) | ((uint)BoolToInt(value) << 30);
         }
         private bool ownsValues
         {
-            readonly get => (flags >> 31) != 0;
+            readonly get => ((flags >> 31) & 1) != 0;
             set => flags = (flags & ~(1u << 31)) | ((uint)BoolToInt(value) << 31);
         }
 
@@ -260,6 +260,7 @@ public void Set(DotvvmPropertyId p, object? value)
             else
             {
                 Debug.Assert(values is Dictionary<DotvvmPropertyId, object?>);
+                OwnValues();
                 valuesAsDictionary[p] = value;
             }
         }
@@ -320,10 +321,12 @@ public bool TryAdd(DotvvmPropertyId p, object? value)
                     return Object.ReferenceEquals(existingValue, value);
                 else
                 {
+                    OwnValues();
                     valuesAsDictionary.Add(p, value);
                     return true;
                 }
 #else
+                OwnValues();
                 return valuesAsDictionary.TryAdd(p, value) || Object.ReferenceEquals(valuesAsDictionary[p], value);
 #endif
             }
@@ -408,23 +411,40 @@ internal void CloneInto(ref DotvvmControlProperties newDict)
             else if (this.keys == null)
             {
                 var dictionary = this.valuesAsDictionary;
-                if (dictionary.Count > 30)
+                if (dictionary.Count > 8)
                 {
                     newDict = this;
                     newDict.keys = null;
-                    newDict.valuesAsDictionary = new Dictionary<DotvvmPropertyId, object?>(dictionary);
+                    Dictionary<DotvvmPropertyId, object?>? newValues = null;
                     foreach (var (key, value) in dictionary)
                         if (CloneValue(value) is {} newValue)
+                        {
+                            if (newValues is null)
+                                // ok, we have to copy it
+                                newValues = new Dictionary<DotvvmPropertyId, object?>(dictionary);
                             newDict.valuesAsDictionary[key] = newValue;
+                        }
+
+                    if (newValues is null)
+                    {
+                        newDict.valuesAsDictionary = dictionary;
+                        newDict.ownsValues = false;
+                        this.ownsValues = false;
+                    }
+                    else
+                    {
+                        newDict.valuesAsDictionary = newValues;
+                        newDict.ownsValues = true;
+                    }
                     return;
                 }
-                // move to immutable version if it's reasonably small. It will be probably cloned multiple times again
+                // move to immutable version if it's small. It will be probably cloned multiple times again
                 SwitchToPerfectHashing();
             }
 
             newDict = this;
             newDict.ownsKeys = false;
-            newDict.ownsValues = false;
+            this.ownsKeys = false;
             for (int i = 0; i < newDict.valuesAsArray.Length; i++)
             {
                 if (CloneValue(newDict.valuesAsArray[i]) is {} newValue)
@@ -439,6 +459,12 @@ internal void CloneInto(ref DotvvmControlProperties newDict)
                     newDict.valuesAsArray[i] = newValue;
                 }
             }
+
+            if (newDict.values == this.values)
+            {
+                this.ownsValues = false;
+                newDict.ownsValues = false;
+            }
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]

diff --git a/src/Framework/Framework/DependencyInjection/DotVVMServiceCollectionExtensions.cs b/src/Framework/Framework/DependencyInjection/DotVVMServiceCollectionExtensions.cs
@@ -122,7 +122,7 @@ public static IServiceCollection RegisterDotVVMServices(IServiceCollection servi
                 var requiredResourceControl = controlResolver.ResolveControl(new ResolvedTypeDescriptor(typeof(RequiredResource)));
                 o.TreeVisitors.Add(() => new StyleTreeShufflingVisitor(controlResolver));
                 o.TreeVisitors.Add(() => new ControlPrecompilationVisitor(s));
-                // o.TreeVisitors.Add(() => new LiteralOptimizationVisitor());
+                o.TreeVisitors.Add(() => new LiteralOptimizationVisitor());
                 o.TreeVisitors.Add(() => new BindingRequiredResourceVisitor((ControlResolverMetadata)requiredResourceControl));
                 var requiredGlobalizeControl = controlResolver.ResolveControl(new ResolvedTypeDescriptor(typeof(GlobalizeResource)));
                 o.TreeVisitors.Add(() => new GlobalizeResourceVisitor((ControlResolverMetadata)requiredGlobalizeControl));

diff --git a/src/Tests/Runtime/DotvvmPropertyTests.cs b/src/Tests/Runtime/DotvvmPropertyTests.cs
@@ -303,6 +303,8 @@ public void DotvvmProperty_CorrectGetAndSet()
                     foreach (var example in GetExampleValues(p.PropertyType))
                     {
                         instance.SetValue(p, example);
+                        Assert.AreEqual(example, instance.GetValue(p), $"GetValue is behaving weird {p}");
+                        Assert.AreEqual(example, instance.GetValueRaw(p.Id), $"GetValue(id) is behaving weird {p}");
                         Assert.AreEqual(example, p.PropertyInfo.GetValue(instance), $"Getter is broken in {p}");
                     }
 
@@ -371,5 +373,58 @@ public void DotvvmProperty_ManyItemsSetter()
 
             Assert.AreEqual(1000, control1.Properties.Count);
         }
+
+        [TestMethod]
+        [DataRow(false, false)]
+        [DataRow(false, true)]
+        [DataRow(true, false)]
+        [DataRow(true, true)]
+        public void DotvvmProperty_ControlClone(bool manyAttributes, bool nestedControl)
+        {
+            var control = new HtmlGenericControl("div");
+            control.CssStyles.Set("color", "red");
+            control.Attributes.Set("something", "value");
+
+            if (manyAttributes)
+                for (int i = 0; i < 60; i++)
+                    control.Attributes.Set("data-" + i.ToString(), i.ToString());
+
+            if (nestedControl)
+            {
+                control.Properties.Add(Styles.ReplaceWithProperty, new HtmlGenericControl("span") { InnerText = "23" });
+            }
+
+            var clone = (HtmlGenericControl)control.CloneControl();
+
+            Assert.AreEqual(control.TagName, clone.TagName);
+            Assert.AreEqual(control.CssStyles["color"], "red");
+
+            // change original
+            Assert.IsFalse(clone.CssStyles.ContainsKey("abc"));
+            control.CssStyles.Set("color", "blue");
+            control.CssStyles.Set("abc", "1");
+            Assert.AreEqual("red", clone.CssStyles["color"]);
+            Assert.IsFalse(clone.CssStyles.ContainsKey("abc"));
+
+            if (nestedControl)
+            {
+                var nestedClone = (HtmlGenericControl)clone.Properties[Styles.ReplaceWithProperty]!;
+                var nestedOriginal = (HtmlGenericControl)control.Properties[Styles.ReplaceWithProperty]!;
+                Assert.AreEqual("23", nestedClone.InnerText);
+                // change clone this time
+                nestedClone.InnerText = "24";
+                Assert.AreEqual("23", nestedOriginal.InnerText);
+                Assert.AreEqual("24", nestedClone.InnerText);
+            }
+
+            if (manyAttributes)
+            {
+                for (int i = 0; i < 60; i++)
+                {
+                    Assert.AreEqual(i.ToString(), control.Attributes["data-" + i.ToString()]);
+                    Assert.AreEqual(i.ToString(), clone.Attributes["data-" + i.ToString()]);
+                }
+            }
+        }
     }
 }