Skip to content

Commit eedc1a9

Browse files
Fix memory leaks and race conditions in word highlighter
--- - 🛡️ Prevent duplicate event registration with HashSet<ITextView> - 🧵 Add versioning to highlight requests to avoid race conditions - 🔒 Ensure thread safety in event registration/unregistration - ✅ Only process latest caret movement for highlighting - 🧹 Improve null checks and result filtering for robustness
1 parent 69483a6 commit eedc1a9

1 file changed

Lines changed: 45 additions & 13 deletions

File tree

src/toolkit/Community.VisualStudio.Toolkit.Shared/MEF/SameWordHighlighterBase.cs

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Collections.Generic;
33
using System.ComponentModel.Composition;
44
using System.Linq;
5+
using System.Threading;
56
using Microsoft.VisualStudio.Shell;
67
using Microsoft.VisualStudio.Text;
78
using Microsoft.VisualStudio.Text.Editor;
@@ -69,9 +70,11 @@ internal class SameWordHighlighterTagger : ITagger<HighlightWordTag>
6970
private readonly ITextSearchService? _textSearchService;
7071
private readonly ITextStructureNavigator? _textStructureNavigator;
7172
private readonly SameWordHighlighterBase _tagger;
73+
private readonly HashSet<ITextView> _registeredViews = new();
7274
private NormalizedSnapshotSpanCollection _wordSpans;
7375
private SnapshotSpan? _currentWord;
7476
private SnapshotPoint _requestedPoint;
77+
private int _requestVersion;
7578
private readonly object _syncLock = new();
7679

7780
public SameWordHighlighterTagger(ITextView view, ITextSearchService? textSearchService,
@@ -88,19 +91,35 @@ public SameWordHighlighterTagger(ITextView view, ITextSearchService? textSearchS
8891

8992
internal void RegisterEvents(ITextView textView)
9093
{
94+
lock (_syncLock)
95+
{
96+
// Prevent duplicate event registration for the same view (memory leak fix)
97+
if (!_registeredViews.Add(textView))
98+
{
99+
return;
100+
}
101+
}
102+
91103
textView.Caret.PositionChanged += CaretPositionChanged;
92104
textView.LayoutChanged += ViewLayoutChanged;
93105
textView.Closed += TextView_Closed;
94106
Counter += 1;
95-
//System.Diagnostics.Debug.WriteLine($"RegisterEvents {_fileName}: #{Counter} ");
96107
}
108+
97109
internal void UnRegisterEvents(ITextView textView)
98110
{
111+
lock (_syncLock)
112+
{
113+
if (!_registeredViews.Remove(textView))
114+
{
115+
return;
116+
}
117+
}
118+
99119
textView.Caret.PositionChanged -= CaretPositionChanged;
100120
textView.LayoutChanged -= ViewLayoutChanged;
101121
textView.Closed -= TextView_Closed;
102122
Counter -= 1;
103-
//System.Diagnostics.Debug.WriteLine($"UnRegisterEvents {_fileName}: #{Counter} ");
104123
}
105124
private void ViewLayoutChanged(object sender, TextViewLayoutChangedEventArgs e)
106125
{
@@ -139,14 +158,16 @@ private void UpdateAtCaretPosition(CaretPosition caretPosition)
139158
return;
140159
}
141160

161+
// Increment version atomically to track request changes (race condition fix)
162+
int currentVersion = Interlocked.Increment(ref _requestVersion);
142163
_requestedPoint = point.Value;
143164
TextExtent? word = _textStructureNavigator?.GetExtentOfWord(_requestedPoint);
144165

145166
if (word.HasValue && word.Value.IsSignificant && word.Value.Span.Length > 0)
146167
{
147168
ThreadHelper.JoinableTaskFactory.StartOnIdleShim(() =>
148169
{
149-
UpdateWordAdornments(word.Value);
170+
UpdateWordAdornments(word.Value, currentVersion);
150171
}, VsTaskRunContext.UIThreadIdlePriority).FireAndForget();
151172
}
152173
else
@@ -155,42 +176,53 @@ private void UpdateAtCaretPosition(CaretPosition caretPosition)
155176
// removed when we move the caret to whitespace
156177
ClearSpans();
157178
}
158-
159179
}
160180

161-
private void UpdateWordAdornments(TextExtent word)
181+
private void UpdateWordAdornments(TextExtent word, int requestVersion)
162182
{
183+
// Early exit if a newer request has been made (race condition fix)
184+
if (requestVersion != Volatile.Read(ref _requestVersion))
185+
{
186+
return;
187+
}
188+
163189
SnapshotPoint currentRequest = _requestedPoint;
164190
List<SnapshotSpan>? wordSpans = new();
165191

166192
string? text = word.Span.GetText();
167-
if (_tagger.ShouldHighlight(text))
193+
if (_tagger.ShouldHighlight(text) && _textSearchService != null)
168194
{
169195
FindData findData = new(text, word.Span.Snapshot)
170196
{
171197
FindOptions = _tagger.FindOptions
172198
};
173199

174-
System.Collections.ObjectModel.Collection<SnapshotSpan>? found = _textSearchService!.FindAll(findData);
175-
wordSpans.AddRange(_tagger.FilterResults(found));
200+
System.Collections.ObjectModel.Collection<SnapshotSpan>? found = _textSearchService.FindAll(findData);
201+
IEnumerable<SnapshotSpan>? filtered = _tagger.FilterResults(found);
202+
if (filtered != null)
203+
{
204+
wordSpans.AddRange(filtered);
205+
}
176206

177207
if (wordSpans.Count == 1)
178208
{
179209
wordSpans.Clear();
180210
}
181211
}
182-
//If another change hasn't happened, do a real update
183-
if (currentRequest == _requestedPoint)
212+
213+
// If another change hasn't happened, do a real update (use version check)
214+
if (requestVersion == Volatile.Read(ref _requestVersion))
184215
{
185-
SynchronousUpdate(currentRequest, new NormalizedSnapshotSpanCollection(wordSpans), word.Span);
216+
SynchronousUpdate(currentRequest, new NormalizedSnapshotSpanCollection(wordSpans), word.Span, requestVersion);
186217
}
187218
}
188219

189-
private void SynchronousUpdate(SnapshotPoint currentRequest, NormalizedSnapshotSpanCollection newSpans, SnapshotSpan? newCurrentWord)
220+
private void SynchronousUpdate(SnapshotPoint currentRequest, NormalizedSnapshotSpanCollection newSpans, SnapshotSpan? newCurrentWord, int requestVersion)
190221
{
191222
lock (_syncLock)
192223
{
193-
if (currentRequest != _requestedPoint)
224+
// Use version check instead of SnapshotPoint comparison (race condition fix)
225+
if (requestVersion != _requestVersion)
194226
{
195227
return;
196228
}

0 commit comments

Comments
 (0)