Use ITextBufferCloneService if available in AsText#26339
Merged
sharwell merged 1 commit intodotnet:dev15.7.xfrom Apr 24, 2018
Merged
Use ITextBufferCloneService if available in AsText#26339sharwell merged 1 commit intodotnet:dev15.7.xfrom
sharwell merged 1 commit intodotnet:dev15.7.xfrom
Conversation
sharwell
commented
Apr 23, 2018
Contributor
Author
There was a problem hiding this comment.
💭 This seems to have been an oversight in 4924d70
CyrusNajmabadi
approved these changes
Apr 23, 2018
jasonmalinowski
approved these changes
Apr 23, 2018
This change also ensures the projection buffers created in Visual Studio have the ITextBufferCloneService added to their property bag. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/604150
15047ce to
ab34327
Compare
heejaechang
reviewed
Apr 23, 2018
| if (!finalize) | ||
| { | ||
| _textBufferFactoryService.TextBufferCreated -= AddTextBufferCloneServiceToBuffer; | ||
| _projectionBufferFactoryService.ProjectionBufferCreated -= AddTextBufferCloneServiceToBuffer; |
Contributor
There was a problem hiding this comment.
so we do this for every buffer or only roslyn buffer?
Contributor
|
approved to merge via link |
Member
Not for this change but in general we should really avoid the use of Type instances as key in the property bags. Invites collisions with other services. Refers to: src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs:229 in ab34327. [](commit_id = ab34327, deletion_comment = False) |
agocke
pushed a commit
that referenced
this pull request
May 16, 2018
See #26339 for the primary RCA. This change corrects an additional edge case for ITextBuffer instances created before VisualStudioWorkspace attaches event handlers to ITextBufferFactoryService and IProjectionBufferFactoryService. This case is only believed to occur in IDE initialization scenarios where the workspace load is triggered by opening a document.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/604150
/cc @minestarks
Customer scenario
A customer attempts to reformat an HTML document, and Visual Studio crashes. Pasting the following into an HTML document will reproduce the issue.
Bugs this fixes
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/604150
Workarounds, if any
None.
Risk
Low. Of multiple possible places to fix the sequence of events leading to a product crash, this one provides the strongest assurances that 15.7 release will continue to behave like 15.6 w.r.t. text differencing.
Performance impact
Aside from fixing a possible crash, performance is improved in the impacted scenarios by allowing the use of
ITextBufferCloneServicerather than falling back to the slower path.Is this a regression from a previous update?
Yes, introduced by #25558.
Root cause analysis
Two changes in the causal PR resulted in a behavior change:
AsText(ITextSnapshot)extension method failed to use theITextBufferCloneServiceassigned to the source text buffer for the snapshot.ITextBufferFactoryService. The TypeScript implementation, on the other hand, uses projection buffers created byIProjectionBufferFactoryService. The events notifying extension code that a buffer is created differs for these two services.While the fallback behavior of Roslyn was intended to be slower but functionally equivalent, the text differencing algorithm differed between the two approaches - the one used when
ITextBufferCloneServicewas not available threw an exception if the set of changes made to a buffer were not ordered.How was the bug found?
Dogfooding / reported by TypeScript team
Test documentation updated?
N/A