Fix memory leak in ProjectRootElement.Reload

src/Build.OM.UnitTests/Construction/ProjectRootElement_Tests.cs

@@ -9,6 +9,7 @@
 using System.Security.AccessControl;
 using System.Security.Principal;
 #endif
+using System.Reflection;
 using System.Text;
 using System.Threading;
 using System.Xml;
@@ -18,6 +19,7 @@
 
 using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException;
 using ProjectCollection = Microsoft.Build.Evaluation.ProjectCollection;
+using Shouldly;
 using Xunit;
 
 namespace Microsoft.Build.UnitTests.OM.Construction
@@ -1854,6 +1856,31 @@ public void ReloadCanOverwriteUnsavedChanges()
             AssertReload(SimpleProject, ComplexProject, true, true, true, act);
         }
 
+        [Fact]
+        public void ReloadDoesNotLeakCachedXmlDocuments()
+        {
+            using var env = TestEnvironment.Create();
+            var testFiles = env.CreateTestProjectWithFiles("", new[] { "build.proj" });
+            var projectFile = testFiles.CreatedFiles.First();
+
+            var projectElement = ObjectModelHelpers.CreateInMemoryProjectRootElement(SimpleProject);
+            projectElement.Save(projectFile);
+
+            int originalDocumentCount = GetNumberOfDocumentsInProjectStringCache(projectElement);
+
+            // Test successful reload.
+            projectElement.Reload(false);
+            GetNumberOfDocumentsInProjectStringCache(projectElement).ShouldBe(originalDocumentCount);
+
+            // Test failed reload.
+            using (StreamWriter sw = new StreamWriter(projectFile))
+            {
+                sw.WriteLine("<XXX />"); // Invalid root element
+            }
+            Should.Throw<InvalidProjectFileException>(() => projectElement.Reload(false));
+            GetNumberOfDocumentsInProjectStringCache(projectElement).ShouldBe(originalDocumentCount);
+        }
+
         private void AssertReload(
             string initialContents,
             string changedContents,
@@ -1986,5 +2013,17 @@ private void VerifyAssertLineByLine(string expected, string actual)
         {
             Helpers.VerifyAssertLineByLine(expected, actual, false);
         }
+
+        /// <summary>
+        /// Returns the number of documents retained by the project string cache.
+        /// Peeks at it via reflection since internals are not visible to these tests.
+        /// </summary>
+        private int GetNumberOfDocumentsInProjectStringCache(ProjectRootElement project)
+        {
+            var bindingFlags = BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.GetProperty;
+            object document = typeof(ProjectRootElement).InvokeMember("XmlDocument", bindingFlags, null, project, Array.Empty<object>());
+            object cache = document.GetType().InvokeMember("StringCache", bindingFlags, null, document, Array.Empty<object>());
+            return (int)cache.GetType().InvokeMember("DocumentCount", bindingFlags, null, cache, Array.Empty<object>());
+        }
     }
 }

src/Build/Construction/ProjectRootElement.cs

@@ -1677,19 +1677,33 @@ private void ReloadFrom(Func<bool, XmlDocumentWithLocation> documentProducer, bo
         {
             ThrowIfUnsavedChanges(throwIfUnsavedChanges);
 
-            XmlDocumentWithLocation document = documentProducer(preserveFormatting ?? PreserveFormatting);
-
-            // Reload should only mutate the state if there are no parse errors.
-            ThrowIfDocumentHasParsingErrors(document);
-
-            // Do not clear the string cache.
-            // Based on the assumption that Projects are reloaded repeatedly from their file with small increments,
-            // and thus most strings would get reused
-            //this.XmlDocument.ClearAnyCachedStrings();
+            var oldDocument = XmlDocument;
+            XmlDocumentWithLocation newDocument = documentProducer(preserveFormatting ?? PreserveFormatting);
+            try
+            {
+                // Reload should only mutate the state if there are no parse errors.
+                ThrowIfDocumentHasParsingErrors(newDocument);
 
-            RemoveAllChildren();
+                RemoveAllChildren();
 
-            ProjectParser.Parse(document, this);
+                ProjectParser.Parse(newDocument, this);
+            }
+            finally
+            {
+                // Whichever document didn't become this element's document must be removed from the string cache.
+                // We do it after the fact based on the assumption that Projects are reloaded repeatedly from their
+                // file with small increments, and thus most strings would get reused avoiding unnecessary churn in
+                // the string cache.
+                var currentDocument = XmlDocument;
+                if (!object.ReferenceEquals(currentDocument, oldDocument))
+                {
+                    oldDocument.ClearAnyCachedStrings();
+                }
+                if (!object.ReferenceEquals(currentDocument, newDocument))
+                {
+                    newDocument.ClearAnyCachedStrings();
+                }
+            }
 
             MarkDirty("Project reloaded", null);
         }

src/Build/Evaluation/ProjectStringCache.cs

@@ -61,6 +61,20 @@ internal int Count
             }
         }
 
+        /// <summary>
+        /// Obtain the number of documents contained in the cache.
+        /// </summary>
+        internal int DocumentCount
+        {
+            get
+            {
+                lock (_locker)
+                {
+                    return _documents.Count;
+                }
+            }
+        }
+
         /// <summary>
         /// Add the given string to the cache or return the existing string if it is already
         /// in the cache.