Fixed cases where 1:1 mapping between Roslyn and editor text buffer/s… #24849

heejaechang · 2018-02-14T21:38:33Z

…napshot are broken.

First case was where the code reused same Roslyn text snapshot to 2 different editor snapshots.
that can cause getting editor snapshot from roslyn snapshot to fail.

Second case was where same editor snapshot can give 2 different roslyn text snapshot breaking
invariant.

didn't do much cleanup except fixing those above two.

Customer scenario

Customer doing rename and in rare case, some identifier doesn't get renamed correctly.

Bugs this fixes

#7364

Workarounds, if any

No

Risk

I don't see any risk

Performance impact

I believe this code that broke 1:1 mapping was added long time ago hoping improve typing perf number by reusing same buffer when possible. but we are seeing issue due to it. and not sure whether this optimization still matter now since the optimization was to reduce allocations and current allocation behavior is completely different than when this optimization was added. also we now reuse ITextImage rather than cloning. can't say for sure until we see RPS result, but I bet is we won't see much difference since the optimization was targeting very specific case and we noticed that case only because the perf test we used (not RPS test) at the moment happen to have a case where this gets hit.

Is this a regression from a previous update?

No

Root cause analysis

internally we have a bi-directional map between roslyn text buffer and editor text buffer. this map must be 1:1 map each other. otherwise, we can't reliably move between roslyn's world and editor's world. we use this map to move from editor's snapshot to roslyn's snapshot, and move back from roslyn to editor.

if this 1:1 map got broken, we might move to wrong snapshot from one to the other or failed to move at all which cause issue like (#7364)

How was the bug found?

Watson, feedback

heejaechang · 2018-02-14T21:38:55Z

@jinujoseph @dotnet/roslyn-ide can you take a look?

…napshot are broken. First case was where the code reused same Roslyn text snapshot to 2 different editor snapshots. that can cause getting editor snapshot from roslyn snapshot to fail. Second case was where same editor snapshot can give 2 different roslyn text snapshot breaking invariant. didn't do much cleanup except fixing those above two.

heejaechang · 2018-02-15T18:51:42Z

ping?

heejaechang · 2018-02-15T19:03:31Z

retest windows_coreclr_test_prtest please

heejaechang · 2018-02-15T21:38:51Z

if this solves the issue where FindCorrespondingEditorTextSnapshot returns null even if the snapshot is currently strongly held in another place, we should remove all the workaround we have put so far.

heejaechang · 2018-02-16T21:55:30Z

can I get a code review?

heejaechang · 2018-02-26T19:05:26Z

ping?

heejaechang · 2018-03-01T19:00:21Z

@jinujoseph @jasonmalinowski can I get some code review? @mavasani @sharwell

heejaechang · 2018-03-01T19:01:05Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

+            /// Reverse map of roslyn text to editor snapshot. unlike forward map, this doesn't strongly hold onto editor snapshot so that 
+            /// we don't leak editor snapshot which should go away once editor is closed. roslyn source's lifetime is not usually tied to view.
+            /// </summary>
+            private static readonly ConditionalWeakTable<ITextImage, WeakReference<ITextSnapshot>> s_roslynToEditorSnapshotMap = new ConditionalWeakTable<ITextImage, WeakReference<ITextSnapshot>>();


this just moved from deleted file

heejaechang · 2018-03-01T19:01:32Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

            {
-                var strongBox = s_textBufferLatestSnapshotMap.GetOrCreateValue(editorSnapshot.TextBuffer);
-                var text = strongBox.Value;
-                if (text != null && text._reiteratedVersion == editorSnapshot.Version.ReiteratedVersionNumber)


this is actual bug where 1:1 mapping got broken.

@heejaechang For clarity, what was the problem? If we reiterated a version, we'd use the same SourceText. That meant that s_roslynToEditorSnapshotMap was pointing to the older snapshot rather than the newer one, which could have gotten collected?

we need to guarantee this. ITextSnapshot(#1) -> SourceText (#2) => ITextSnapshot (#1)

the code above can cause ITextSnapshot(#1) -> SourceText(#2) -> ITextSnapshot (#3)

but if ITextSnapshot (#3) is released, one will one up get null.

"the older snapshot"

it could be older or newer, which ever called AsSourceText first.

jinujoseph · 2018-03-01T19:11:11Z

cc @chborl

heejaechang · 2018-03-01T19:13:30Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

@@ -262,6 +239,53 @@ public override SourceText WithChanges(IEnumerable<TextChange> changes)
                    currentSnapshot: ((ITextSnapshot2)buffer.CurrentSnapshot).TextImage);
            }

+            private static ITextImage RecordReverseMapAndGetImage(ITextSnapshot editorSnapshot)
+            {


these are also just moved from deleted file.

heejaechang · 2018-03-01T19:13:48Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

@@ -23,17 +22,6 @@ public static partial class Extensions
        /// </summary>
        private class SnapshotSourceText : SourceText
        {
-            /// <summary>
-            /// Use a separate class for closed files to simplify memory leak investigations


this just moved down

heejaechang · 2018-03-01T19:14:28Z

src/EditorFeatures/Text/Extensions.TextBufferContainer.cs


                _weakEditorBuffer = new WeakReference<ITextBuffer>(editorBuffer);
-                _currentText = new SnapshotSourceText(TextBufferMapper.RecordTextSnapshotAndGetImage(editorBuffer.CurrentSnapshot), encoding, this);


this another 1:1 mapping bug.

heejaechang · 2018-03-02T22:23:50Z

thank you! @jinujoseph @Pilchie can I get approval for 15.7?

jinujoseph · 2018-03-02T22:45:33Z

we need one more review @jasonmalinowski can you take a look

jasonmalinowski · 2018-03-02T23:36:13Z

if this solves the issue where FindCorrespondingEditorTextSnapshot returns null even if the snapshot is currently strongly held in another place, we should remove all the workaround we have put so far.

How do you propose we tackle this? Can we identify a set of non-fatal Watsons or some other telemetry event to watch?

heejaechang · 2018-03-02T23:56:37Z

I was thinking doing this (#24875) which will let me to audit all TryFindCorrespondingSnapshot/Buffer usages. while doing it I can remove workaround if one is added there.

jasonmalinowski

Many questions, but critically I believe there's an assert with an incorrect condition that would have caught this bug long ago.

I also admit, I do wish our SourceTexts for open files just held onto ITextSnapshots. I understand the concern about "if we leak a snapshot with open files, we're holding open ITextBuffers" but I reject that concern. "If we leak something bad, we might leak something bad" isn't something I think warrants additional complexity here.

jasonmalinowski · 2018-03-02T23:32:03Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs


-            private SnapshotSourceText(ITextSnapshot editorSnapshot, Encoding encodingOpt)
+            private SnapshotSourceText(ITextSnapshot editorSnapshot)


Can this just do : this(editorSnapshot, TextBufferContainer.From(editorSnapshot.TextBuffer) to avoid the duplicate constructor logic?

(or for that matter, why do we have both?...)

yep. looks like we can merge these two

jasonmalinowski · 2018-03-02T23:34:15Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

            {
-                var strongBox = s_textBufferLatestSnapshotMap.GetOrCreateValue(editorSnapshot.TextBuffer);
-                var text = strongBox.Value;
-                if (text != null && text._reiteratedVersion == editorSnapshot.Version.ReiteratedVersionNumber)


@heejaechang For clarity, what was the problem? If we reiterated a version, we'd use the same SourceText. That meant that s_roslynToEditorSnapshotMap was pointing to the older snapshot rather than the newer one, which could have gotten collected?

jasonmalinowski · 2018-03-03T00:13:03Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

+
+                // If we're already in the map, there's nothing to update.  Do a quick check
+                // to avoid two allocations per call to RecordTextSnapshotAndGetImage.
+                if (!s_roslynToEditorSnapshotMap.TryGetValue(textImage, out var weakReference))


I recognize this got moved but it's still confusing: should this be called s_textImageToEditorSnapshotMap or something more precise?

jasonmalinowski · 2018-03-03T00:15:58Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

+                var textImage = ((ITextSnapshot2)editorSnapshot).TextImage;
+                Contract.ThrowIfNull(textImage);
+
+                // If we're already in the map, there's nothing to update.  Do a quick check


Are there edits that don't change the ITextImage, such as content type changes? Because it seems we'll skip this and once again be holding onto a weak reference of an old version.

I dont believe there is such case.
http://index/?leftProject=Microsoft.VisualStudio.Platform.VSEditor&leftSymbol=tkm9ha9jkucm&file=BaseSnapshot.cs&line=32

if there is new Snapshot there will be new Image

Thanks for confirming.

jasonmalinowski · 2018-03-03T00:16:49Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

+                // forward and reversed map is 1:1 map. as long as snapshot is not null, it can't be
+                // different
+                var snapshot = weakReference.GetTarget();
+                Contract.ThrowIfFalse(snapshot == null || snapshot == editorSnapshot);


The snapshot == null part seems bad here, as I suspect had this been enabled we would have caught this bug long ago: if that is ever true, it means s_roslynToEditorSnapshotMap should have been updated and we've retriggered this bug. Should we remove this condition?

snapshot can be null. if someone did var sourceText = ITextSnapshot(#1).AsSourceText(); and let ITextSnapshot(#1) go. then, sourceText.TryGetCorresponding... will return null (snapshot == null)

Yes, but this function isn't called there. :-) The weak reference can obviously be lost (otherwise what's the point), but it should never disappear in this function.

Imagine we took the case where the if branch was taken, and we did create a weak reference for the text image that was passed. In debug, we have GC roots for editorSnapshot (because of this line) and textImage (because it's going to be returned), therefore, the weak reference can't possibly have been collected yet. Thus, the latter assert will be true, and snapshot can never be null.

Now consider the case where the branch wasn't taken; that means that the textImage -> textSnapshot weak entry has already been created. By your statement to my earlier question, there is a 1:1 mapping here, so if the textImage is alive, the it must be the same snapshot as editorSnapshot, which is also alive (because it was handed to this function). Therefore, it can't be null. If it was null, that means there are two different snapshots with the same TextImage, which invalidates your assertion when you linked to BaseSnapshot.cs earlier, and if so, this entire approach is broken.

Follow me?

ah. yes. it can't be null :)

heejaechang · 2018-03-03T00:53:42Z

"I also admit, I do wish our SourceTexts for open files just held onto ITextSnapshots. I understand the concern about "if we leak a snapshot with open files, we're holding open ITextBuffers" but I reject that concern. "If we leak something bad, we might leak something bad" isn't something I think warrants additional complexity here."

well, editor and roslyn holding each other strongly will certainly make things more convenient. but once we allowed anyone to be able to access VSWorkspace.CurrentSolution, I am not sure whether that is okay. I added this once I got bunch of issues from customer dump showing us holding to a lot of things that are inside of ITextBuffer property bags. snapshot itself or buffer itself wasn't the biggest issue. issue was property bags. and people put all kinds of things there including refernece back to WpfViews.

anyway, without the bug, only thing roslyn side make sure is that holding onto ITextSnapshot somewhere while working on Roslyn snapshot created off that ITextSnapshot. it doesn't need to pass along. it could be just a local variable.

jasonmalinowski · 2018-03-03T01:15:59Z

I added this once I got bunch of issues from customer dump showing us holding to a lot of things that are inside of ITextBuffer property bags. snapshot itself or buffer itself wasn't the biggest issue. issue was property bags. and people put all kinds of things there including refernece back to WpfViews.

I guess I would have pushed those bugs all upstream. If a SourceText is causing a leak of an ITextBuffer, that means somebody is leaking the SourceText, probably because they're holding onto a Solution. I would have just pushed those bugs to whoever leaked that since leaking Solutions is still expensive. I don't see the reason to be resilient to misuse bugs when the misuse is still plenty bad, and it means we create really complicated lifetime bugs like this. 😄 The cure seems worse than the disease.

heejaechang · 2018-03-03T02:26:23Z

well. once we let public to hold onto Solution, I think it is up to them to decide when is appropriate for them to release solution. and I think that is different than what they expect from holding onto Solution (roslyn objects). In reality, they are not only holding onto roslyn objects, but unbound number of objects (since it has property bag, it is not just editor objects either. it is truely everything there.)

heejaechang · 2018-03-03T18:30:05Z

retest windows_debug_vs-integration_prtest please

heejaechang · 2018-03-05T19:43:25Z

@jasonmalinowski ping. addressed all your feedback.

jasonmalinowski · 2018-03-05T19:49:20Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

+                }
+
+#if DEBUG
+                // forward and reversed map is 1:1 map. as long as snapshot is not null, it can't be


"as long as snapshot is not null" comment is now stale.

will update

heejaechang · 2018-03-05T22:07:35Z

@jinujoseph @Pilchie can I get approval for 15.7?

Pilchie · 2018-03-06T00:06:27Z

Approved - thanks.

…t change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - dotnet#24849

* made sure we raise workspace changed events even when there is no text change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - #24849 * added comments on why we need to process all text change events

* made sure we raise workspace changed events even when there is no text change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - dotnet#24849 * added comments on why we need to process all text change events

…apshot without changes (#25576) * Fixed DDRIT failure (#25551) * made sure we raise workspace changed events even when there is no text change. this is needed to bring workspace up to date with latest text snapshot after we don't merge 2 text snapshot to 1 source text if content is same. see this for more detail - #24849 * added comments on why we need to process all text change events * removed changes empty check. now it is possible to have empty changes. * remove non-empty from comment since set now can be empty.

heejaechang requested a review from a team as a code owner February 14, 2018 21:38

heejaechang added 4 commits February 14, 2018 16:00

null check. weakReference can be null.

a595eaa

make sure weakReference always exist

97f9a1c

fixed test failures

8700a69

heejaechang force-pushed the cleanupBuffer branch from bb657fa to 8700a69 Compare February 15, 2018 00:03

heejaechang mentioned this pull request Feb 15, 2018

take advantage of Trackspan of ITextImage that are used as underlying implementation of SnapshotSourceText #24875

Closed

heejaechang commented Mar 1, 2018

View reviewed changes

chborl approved these changes Mar 2, 2018

View reviewed changes

jinujoseph added this to the 15.7 milestone Mar 2, 2018

jasonmalinowski requested changes Mar 3, 2018

View reviewed changes

addressed PR feedbacks

2fa75b1

jasonmalinowski approved these changes Mar 5, 2018

View reviewed changes

updated comment

518f45d

Pilchie added the Approved to merge label Mar 6, 2018

heejaechang merged commit 82ae6b7 into dotnet:dev15.7.x Mar 6, 2018

heejaechang mentioned this pull request Mar 17, 2018

Fixed DDRIT failure #25551

Merged

heejaechang mentioned this pull request Mar 30, 2018

Pass SemanticModel as an argument where it is required #25839

Merged


		_weakEditorBuffer = new WeakReference<ITextBuffer>(editorBuffer);
		_currentText = new SnapshotSourceText(TextBufferMapper.RecordTextSnapshotAndGetImage(editorBuffer.CurrentSnapshot), encoding, this);


		private SnapshotSourceText(ITextSnapshot editorSnapshot, Encoding encodingOpt)
		private SnapshotSourceText(ITextSnapshot editorSnapshot)

Fixed cases where 1:1 mapping between Roslyn and editor text buffer/s… #24849

Fixed cases where 1:1 mapping between Roslyn and editor text buffer/s… #24849

Conversation

heejaechang commented Feb 14, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

heejaechang commented Feb 14, 2018

heejaechang commented Feb 15, 2018

heejaechang commented Feb 15, 2018

heejaechang commented Feb 15, 2018

heejaechang commented Feb 16, 2018

heejaechang commented Feb 26, 2018

heejaechang commented Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 2, 2018

jinujoseph commented Mar 2, 2018

jasonmalinowski commented Mar 2, 2018

heejaechang commented Mar 2, 2018

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 3, 2018

jasonmalinowski commented Mar 3, 2018

heejaechang commented Mar 3, 2018 • edited Loading

heejaechang commented Mar 3, 2018

heejaechang commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 5, 2018

Pilchie commented Mar 6, 2018

heejaechang commented Mar 1, 2018 •

edited

Loading

heejaechang Mar 3, 2018 •

edited

Loading

jasonmalinowski Mar 3, 2018 •

edited

Loading

heejaechang commented Mar 3, 2018 •

edited

Loading