Fix handful of unit test issues and the intermittent IndexOutOfRangeE…

…xception failures (#10635) It's probably easiest to go through this commit-by-commit. The main fix here is the `IndexOutOfRangeException` in the `FilePathNormalizer`. The problem occurs when the array rented from the pool already contains a `\` or `/` immediately after the span that we care about. It was possible for `FilePathNormalizer.NormalizeAndDedupeSlashes(...)` to read past the end of the span, and if it found a `\` or `/` there, it would cause the incorrect length to be used later, causing an `IndexOutOfRangeException`. The reason the failure was intermittent is because it's dependent on the contents of the array pool.
dotnet · Jul 17, 2024 · 8e007c6 · 8e007c6
2 parents c0b9aa2 + 6d76c6d
commit 8e007c6
Show file tree

Hide file tree

Showing 9 changed files with 77 additions and 53 deletions.
diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/FilePathNormalizer.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/FilePathNormalizer.cs
@@ -18,7 +18,7 @@ internal static class FilePathNormalizer
 
     public static string NormalizeDirectory(string? directoryFilePath)
     {
-        if (directoryFilePath.IsNullOrEmpty())
+        if (directoryFilePath.IsNullOrEmpty() || directoryFilePath == "/")
         {
             return "/";
         }
@@ -76,7 +76,7 @@ public static string Normalize(string? filePath)
     /// </summary>
     public static string GetNormalizedDirectoryName(string? filePath)
     {
-        if (filePath.IsNullOrEmpty())
+        if (filePath.IsNullOrEmpty() || filePath == "/")
         {
             return "/";
         }
@@ -253,7 +253,8 @@ private static void NormalizeAndDedupeSlashes(Span<char> span, ref int charsWrit
             {
                 writeSlot = '/';
 
-                if (Unsafe.Add(ref readSlot, 1) is '/' or '\\')
+                // Be careful not to read past the end of the span.
+                if (read < charsWritten - 1 && Unsafe.Add(ref readSlot, 1) is '/' or '\\')
                 {
                     // We found two slashes in a row. If we are at the start of the path,
                     // we we are at '\\network' paths, so want to leave them alone. Otherwise

diff --git a/...rosoft.AspNetCore.Razor.LanguageServer.Test/Completion/TagHelperTooltipFactoryBaseTest.cs b/...rosoft.AspNetCore.Razor.LanguageServer.Test/Completion/TagHelperTooltipFactoryBaseTest.cs
@@ -10,6 +10,7 @@
 using Microsoft.AspNetCore.Razor.Language.Components;
 using Microsoft.AspNetCore.Razor.LanguageServer.Tooltip;
 using Microsoft.AspNetCore.Razor.ProjectSystem;
+using Microsoft.AspNetCore.Razor.Test.Common;
 using Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
 using Microsoft.CodeAnalysis.Razor.ProjectSystem;
 using Roslyn.Test.Utilities;
@@ -374,7 +375,7 @@ await projectManager.UpdateAsync(updater =>
         {
             updater.ProjectAdded(hostProject);
             updater.ProjectWorkspaceStateChanged(hostProject.Key, projectWorkspaceState);
-            updater.DocumentAdded(hostProject.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
         });
 
         var service = new TestTagHelperToolTipFactory(projectManager);
@@ -419,11 +420,11 @@ await projectManager.UpdateAsync(updater =>
         {
             updater.ProjectAdded(hostProject1);
             updater.ProjectWorkspaceStateChanged(hostProject1.Key, projectWorkspaceState);
-            updater.DocumentAdded(hostProject1.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject1.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
 
             updater.ProjectAdded(hostProject2);
             updater.ProjectWorkspaceStateChanged(hostProject2.Key, projectWorkspaceState);
-            updater.DocumentAdded(hostProject2.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject2.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
         });
 
         var service = new TestTagHelperToolTipFactory(projectManager);
@@ -468,10 +469,10 @@ await projectManager.UpdateAsync(updater =>
         {
             updater.ProjectAdded(hostProject1);
             updater.ProjectWorkspaceStateChanged(hostProject1.Key, projectWorkspaceState);
-            updater.DocumentAdded(hostProject1.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject1.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
 
             updater.ProjectAdded(hostProject2);
-            updater.DocumentAdded(hostProject2.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject2.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
         });
 
         var service = new TestTagHelperToolTipFactory(projectManager);
@@ -512,10 +513,10 @@ public async Task GetAvailableProjects_NotAvailableInAnyProject_ReturnsText()
         await projectManager.UpdateAsync(updater =>
         {
             updater.ProjectAdded(hostProject1);
-            updater.DocumentAdded(hostProject1.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject1.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
 
             updater.ProjectAdded(hostProject2);
-            updater.DocumentAdded(hostProject2.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
+            updater.DocumentAdded(hostProject2.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
         });
 
         var service = new TestTagHelperToolTipFactory(projectManager);

diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DocumentContextFactoryTest.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DocumentContextFactoryTest.cs
@@ -67,7 +67,7 @@ public async Task TryCreateForOpenDocumentAsync_CanNotResolveVersion_ReturnsNull
 
         await _projectManager.UpdateAsync(updater =>
         {
-            updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, CreateTextLoader(filePath, ""));
+            updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, TestMocks.CreateTextLoader(filePath, ""));
         });
 
         var factory = new DocumentContextFactory(_projectManager, _documentVersionCache, LoggerFactory);
@@ -87,7 +87,7 @@ public async Task TryCreateAsync_ResolvesContent()
 
         await _projectManager.UpdateAsync(updater =>
         {
-            updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, CreateTextLoader(filePath, ""));
+            updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, TestMocks.CreateTextLoader(filePath, ""));
         });
 
         var miscFilesProject = _projectManager.GetMiscellaneousProject();
@@ -142,7 +142,7 @@ public async Task TryCreateForOpenDocumentAsync_ResolvesContent()
 
         await _projectManager.UpdateAsync(updater =>
         {
-            updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, CreateTextLoader(filePath, ""));
+            updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, TestMocks.CreateTextLoader(filePath, ""));
         });
 
         var miscFilesProject = _projectManager.GetMiscellaneousProject();

diff --git a/...t/Microsoft.AspNetCore.Razor.Test.Common.Tooling/LanguageServer/LanguageServerTestBase.cs b/...t/Microsoft.AspNetCore.Razor.Test.Common.Tooling/LanguageServer/LanguageServerTestBase.cs
@@ -24,7 +24,6 @@
 using Microsoft.CodeAnalysis.Text;
 using Microsoft.CommonLanguageServerProtocol.Framework;
 using Microsoft.VisualStudio.LanguageServer.Protocol;
-using Moq;
 using Xunit.Abstractions;
 
 namespace Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
@@ -131,23 +130,6 @@ private protected static VersionedDocumentContext CreateDocumentContext(Uri uri,
         return new VersionedDocumentContext(uri, snapshot, projectContext: null, version: 0);
     }
 
-    protected static TextLoader CreateTextLoader(string filePath, string text)
-    {
-        return CreateTextLoader(filePath, SourceText.From(text));
-    }
-
-    protected static TextLoader CreateTextLoader(string filePath, SourceText text)
-    {
-        var mock = new StrictMock<TextLoader>();
-
-        var textAndVersion = TextAndVersion.Create(text, VersionStamp.Create(), filePath);
-
-        mock.Setup(x => x.LoadTextAndVersionAsync(It.IsAny<LoadTextOptions>(), It.IsAny<CancellationToken>()))
-            .ReturnsAsync(textAndVersion);
-
-        return mock.Object;
-    }
-
     private protected static RazorLSPOptionsMonitor GetOptionsMonitor(bool enableFormatting = true, bool autoShowCompletion = true, bool autoListParams = true, bool formatOnType = true, bool autoInsertAttributeQuotes = true, bool colorBackground = false, bool codeBlockBraceOnNextLine = false, bool commitElementsWithSpace = true)
     {
         var configService = StrictMock.Of<IConfigurationSyncService>();

diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/Logging/TestOutputLogger.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/Logging/TestOutputLogger.cs
@@ -73,10 +73,11 @@ public void Log(LogLevel logLevel, string message, Exception? exception)
         try
         {
             _provider.TestOutputHelper.WriteLine(finalMessage);
-        }
-        catch (InvalidOperationException iex) when (iex.Message == "There is no currently active test.")
-        {
-            // Ignore, something is logging a message outside of a test. Other loggers will capture it.
+
+            if (exception is not null)
+            {
+                _provider.TestOutputHelper.WriteLine(exception.ToString());
+            }
         }
         catch (Exception ex)
         {

diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/TestMocks.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/TestMocks.cs
@@ -0,0 +1,29 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT license. See License.txt in the project root for license information.
+
+using System.Threading;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Text;
+using Moq;
+
+namespace Microsoft.AspNetCore.Razor.Test.Common;
+
+internal static class TestMocks
+{
+    public static TextLoader CreateTextLoader(string filePath, string text)
+    {
+        return CreateTextLoader(filePath, SourceText.From(text));
+    }
+
+    public static TextLoader CreateTextLoader(string filePath, SourceText text)
+    {
+        var mock = new StrictMock<TextLoader>();
+
+        var textAndVersion = TextAndVersion.Create(text, VersionStamp.Create(), filePath);
+
+        mock.Setup(x => x.LoadTextAndVersionAsync(It.IsAny<LoadTextOptions>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(textAndVersion);
+
+        return mock.Object;
+    }
+}
diff --git a/...alStudio.LanguageServices.Razor.Test/DocumentGenerator/BackgroundDocumentGeneratorTest.cs b/...alStudio.LanguageServices.Razor.Test/DocumentGenerator/BackgroundDocumentGeneratorTest.cs
@@ -14,14 +14,17 @@
 using Microsoft.AspNetCore.Razor.ProjectSystem;
 using Microsoft.AspNetCore.Razor.Test.Common;
 using Microsoft.AspNetCore.Razor.Test.Common.VisualStudio;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Razor;
 using Microsoft.CodeAnalysis.Razor.Logging;
+using Microsoft.CodeAnalysis.Razor.ProjectSystem;
 using Microsoft.CodeAnalysis.Text;
 using Microsoft.VisualStudio.Razor.DynamicFiles;
 using Moq;
 using Xunit;
 using Xunit.Abstractions;
 
-namespace Microsoft.CodeAnalysis.Razor.ProjectSystem;
+namespace Microsoft.VisualStudio.Razor.ProjectSystem;
 
 // These tests are really integration tests. There isn't a good way to unit test this functionality since
 // the only thing in here is threading.
@@ -219,27 +222,32 @@ await projectManager.UpdateAsync(updater =>
     [UIFact]
     public async Task ProcessWorkAndRestart()
     {
+        var hostProject1 = TestProjectData.SomeProject;
+        var hostProject2 = TestProjectData.AnotherProject;
+        var hostDocument1 = TestProjectData.SomeProjectFile1;
+        var hostDocument2 = TestProjectData.SomeProjectFile2;
+
         // Arrange
         var projectManager = CreateProjectSnapshotManager();
 
         await projectManager.UpdateAsync(updater =>
         {
-            updater.ProjectAdded(s_hostProject1);
-            updater.ProjectAdded(s_hostProject2);
-            updater.DocumentAdded(s_hostProject1.Key, s_documents[0], null!);
-            updater.DocumentAdded(s_hostProject1.Key, s_documents[1], null!);
+            updater.ProjectAdded(hostProject1);
+            updater.ProjectAdded(hostProject2);
+            updater.DocumentAdded(hostProject1.Key, hostDocument1, null!);
+            updater.DocumentAdded(hostProject1.Key, hostDocument2, null!);
         });
 
-        var project = projectManager.GetLoadedProject(s_hostProject1.Key);
-        var documentKey1 = new DocumentKey(project.Key, s_documents[0].FilePath);
-        var documentKey2 = new DocumentKey(project.Key, s_documents[1].FilePath);
+        var project = projectManager.GetLoadedProject(hostProject1.Key);
+        var documentKey1 = new DocumentKey(project.Key, hostDocument1.FilePath);
+        var documentKey2 = new DocumentKey(project.Key, hostDocument2.FilePath);
 
         using var generator = new TestBackgroundDocumentGenerator(projectManager, _dynamicFileInfoProvider, LoggerFactory);
 
         // Act & Assert
 
         // First, enqueue some work.
-        generator.Enqueue(project, project.GetDocument(s_documents[0].FilePath).AssumeNotNull());
+        generator.Enqueue(project, project.GetDocument(hostDocument1.FilePath).AssumeNotNull());
 
         // Wait for the work to complete.
         await generator.WaitUntilCurrentBatchCompletesAsync();
@@ -248,14 +256,14 @@ await projectManager.UpdateAsync(updater =>
         Assert.Single(generator.CompletedWork, documentKey1);
 
         // Enqueue more work.
-        generator.Enqueue(project, project.GetDocument(s_documents[1].FilePath).AssumeNotNull());
+        generator.Enqueue(project, project.GetDocument(hostDocument2.FilePath).AssumeNotNull());
 
         // Wait for the work to complete.
         await generator.WaitUntilCurrentBatchCompletesAsync();
 
         Assert.Collection(generator.CompletedWork.OrderBy(key => key.DocumentFilePath),
-            key => Assert.Equal(documentKey2, key),
-            key => Assert.Equal(documentKey1, key));
+            key => Assert.Equal(documentKey1, key),
+            key => Assert.Equal(documentKey2, key));
     }
 
     [UIFact]

diff --git a/...t.VisualStudio.LanguageServices.Razor.Test/Documents/VisualStudioFileChangeTrackerTest.cs b/...t.VisualStudio.LanguageServices.Razor.Test/Documents/VisualStudioFileChangeTrackerTest.cs
@@ -79,6 +79,7 @@ public async Task StopListening_UnadvisesForFileChange()
             .Verifiable();
         fileChangeService
             .Setup(f => f.UnadviseFileChangeAsync(123, It.IsAny<CancellationToken>()))
+            .ReturnsAsync(TestProjectData.SomeProjectImportFile.FilePath)
             .Verifiable();
         var tracker = new VisualStudioFileChangeTracker(
             TestProjectData.SomeProjectImportFile.FilePath,

diff --git a/...io.LanguageServices.Razor.Test/LanguageClient/ProjectSystem/RazorProjectInfoDriverTest.cs b/...io.LanguageServices.Razor.Test/LanguageClient/ProjectSystem/RazorProjectInfoDriverTest.cs
@@ -8,6 +8,7 @@
 using Microsoft.AspNetCore.Razor.Language;
 using Microsoft.AspNetCore.Razor.LanguageServer.ProjectSystem;
 using Microsoft.AspNetCore.Razor.ProjectSystem;
+using Microsoft.AspNetCore.Razor.Test.Common;
 using Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
 using Microsoft.CodeAnalysis.Razor.ProjectSystem;
 using Xunit;
@@ -41,10 +42,10 @@ public async Task ProcessesExistingProjectsDuringInitialization()
         await projectManager.UpdateAsync(static updater =>
         {
             updater.ProjectAdded(s_hostProject1);
-            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
+            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
 
             updater.ProjectAdded(s_hostProject2);
-            updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
+            updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, TestMocks.CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
         });
 
         var (driver, testAccessor) = await CreateDriverAndInitializeAsync(projectManager);
@@ -93,10 +94,10 @@ public async Task ProcessesProjectsAddedAfterInitialization()
         await projectManager.UpdateAsync(static updater =>
         {
             updater.ProjectAdded(s_hostProject1);
-            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
+            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
 
             updater.ProjectAdded(s_hostProject2);
-            updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
+            updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, TestMocks.CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
         });
 
         await testAccessor.WaitUntilCurrentBatchCompletesAsync();
@@ -134,7 +135,7 @@ await projectManager.UpdateAsync(static updater =>
 
         await projectManager.UpdateAsync(static updater =>
         {
-            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
+            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
         });
 
         await testAccessor.WaitUntilCurrentBatchCompletesAsync();
@@ -204,7 +205,7 @@ await projectManager.UpdateAsync(static updater =>
 
         await projectManager.UpdateAsync(static updater =>
         {
-            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
+            updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
         });
 
         await testAccessor.WaitUntilCurrentBatchCompletesAsync();