search

microsoft / vs-editor-api

Microsoft Visual Studio Editor API definitions
MIT License
120 stars 32 forks source link

Leak of ITextUndoHistory #8

Closed Therzok closed 4 years ago

Therzok commented 6 years ago

In a few profiling analysis sessions done in VSMac, I have observed that ITextUndoHistory is being leaked in UndoHistoryRegistryImpl.histories Dictionary.

From a few quick source greps, I observe that:

Thus, all docs will leak an UndoHistoryRegistryImpl.

Therzok commented 6 years ago

Tentative fix in MD's source copy: https://github.com/mono/monodevelop/commit/a37e74a10335cecbbdea5630a635d3bc5d7896a3 Haven't tested it yet.

Therzok commented 6 years ago

If the leak is coming in from EditorOperations instantiation, via ITextBufferUndoManagerProvider. GetTextBufferUndoManager, something needs to also call .ITextBufferUndoManagerProvider. RemoveTextBufferUndoManager, along the above tentative fix.

olegtk commented 6 years ago

this probably doesn't leak in VS because VS uses its own legacy undo implementation, which is cleared on VS-specific file close code path. Can you please come up with VS for Mac repro steps? Also +@dpugh

Therzok commented 6 years ago

Repro steps on VS4Mac are opening a doc, editing a bit, closing a doc. UndoHistoryRegistryImpl.histories will contain as many instances of ITextUndoHistory as docs were opened, even if all of them were closed. This causes leaks of undo stacks.

KirillOsenkov commented 6 years ago

@Therzok if you want, you can submit a PR against this fork/branch: https://github.com/KirillOsenkov/vs-editor-api/tree/FirstRelease

and I can rebuild and publish a new NuGet and insert it into MonoDevelop.

olegtk commented 6 years ago

I'm fixing this (same way @Therzok proposed) as part of bigger fix for a leak in LightBulb.

Therzok commented 6 years ago

@olegtk https://github.com/KirillOsenkov/vs-editor-api/pull/1 You can just cherry-pick this if you want. We put it in Kirill's fork so we get it in VSMac asap.

Therzok commented 4 years ago

Should this be closed?

KirillOsenkov commented 4 years ago

@DavidKarlas and I will check next week

KirillOsenkov commented 4 years ago

Hmm, neither of the two fixes here are applied in VS-Platform: https://github.com/KirillOsenkov/vs-editor-api/pull/1/files

@olegtk could you please take a look?

DavidKarlas commented 4 years ago

@KirillOsenkov I opened upstream PR just now... Closing. This slipped, because its in Impl and I wasn't paying much attention in that area yet :(