Dangling text file models of deleted files hanging around in memory

[testforstephen]

• VSCode Version: 1.45.1
• OS Version: Windows 10

Steps to Reproduce: Screencast:

1. Open sample node project below in VS Code. dnd.zip
2. Set the setting "typescript.updateImportsOnFileMove.enabled": "always" or "never".
3. Click to open multiple files (e.g. src/utils/ma.ts, src/utils/mb.ts, src/utils/mc.ts, src/utils/md.ts) in VS Code.
4. In File Explorer, drag and drop these files back and forth several times, e.g.

   1st dnd:  utils/m*.ts -> funcs/m*.ts
   2st dnd:  funcs/m*.ts -> utils/m*.ts
   3rd dnd:  utils/m*.ts -> funcs/m*.ts

5. You would see the states for these opened editors are in chaos. Some showed as "deleted", see the screenshot below. But actually these files exist. My initial finding was that vscode didn't clear the opened text models for the old locations after the move operation, so that the next time you move back, vscode will try to reuse the previously cached text models.

Does this issue occur when all extensions are disabled?: Yes

[isidorn]

@bpasero sounds like this is on the file IO layer, and not in the UI representation. Can you investigate first? Let me know if you are busy and I can also investigate.

[bpasero]

@testforstephen I tried to reproduce but it seemed to work ok for me, maybe more settings are needed? For once, it is not clear to me if it breaks only in the one case where typescript.updateImportsOnFileMove.enabled is set to always or in both cases? It seems quite obvious to me that this issue only surfaces for you if the imports are updated, right?

Can you try the same thing in insiders, to rule out any particular extension or setting you might have that I don't. You can give our preview releases a try from: https://code.visualstudio.com/insiders/

[bpasero]

Hm, I still see a bit of weirdness: the title indicates the file is deleted, but the editor tab does not show that. I think there is no breakage of functionality or data loss (@testforstephen correct me if that is different for you) but there seems to be a bug in showing the decoration in that case.

[testforstephen]

At insider, i can reproduce it in both updateImports enabled and disabled.

I didn't observe data loss yet, but not sure whether some complex case will be.

At least, i observed there are some extra didChange events happening for those problematic editors. And if there is a job on applying the edit in the meantime, it probably throws errors saying there are files changed in the meantime. Especially if you choose to use Refactor Preview panel to preview the updates introduced by dnd, then apply the changes will most likely run into the error saying the file has been modified.

[bpasero]

So the fact that imports are still being rewritten when this setting is disabled smells like a bug that I filed as https://github.com/microsoft/vscode/issues/98081

Checking for the state of file models when doing this, I see some stale text file models lurking around, easy to identify:

• move those files back and forth as instructed
• run the command "Developer: Log Working Copies"
• notice how eventually there are working copies for the moved files both in the one folder as well as the other

Checking for callers of ITextModelResolverService#createReferencedObject() it looks like MainThreadDocuments#$tryCreateDocument() is responsible for creating a reference that is not being disposed when the file is moved.

@jrieken is that intentional? I see BoundModelReferenceCollection having some time associated with it, but this means in theory that invalid models are still hanging around when files get deleted or moved? I think that should be revisited and possibly the IWorkingCopyFileService#onDidRunWorkingCopyFileOperation be used to dispose these references.

[jrieken]

@jrieken is that intentional? I see BoundModelReferenceCollection having some time associated with it, but this means in theory that invalid models are still hanging around when files get deleted or moved?

Yes - we allow extensions to open text documents and there should be a little respecting of extensions when it comes to dispose. E.g. not dispose immediately but eventually. Also, dispose them when they never appear in an editor or as edited. So, today they remain for 3 mins or when a certain size constraint is reached.

This code is very ancient and maybe renaming/deleting wasn't an issue back then - if I remember correctly then the editor disposed models outside the ref-counted lifecycle for the longest time, right?

[bpasero]

if I remember correctly then the editor disposed models outside the ref-counted lifecycle for the longest time, right?

Maybe in the past, not sure. But for a while file editors use the same mechanisms and dispose the reference when the editor closes:

https://github.com/microsoft/vscode/blob/1fc148b25413ec45e4eb6ce13fa44c0eb813ea4a/src/vs/workbench/contrib/files/common/editors/fileEditorInput.ts#L322

I think just dipsosing the model under the hood without everyone that created a reference knowing about this is very dangerous because you may keep a reference to a model that is disposed without knowing.

I can make a pass over clients of the text model resolver that currently do not react on the move/delete and we can go from there. I will also check if my suggestion about participating in the working copy file event is possible, but I think it should be fine. This would then just be another condition under which we free up a reference, besides the timeout.

[jrieken]

Maybe in the past, not sure. But for a while file editors use the same mechanisms and dispose the reference when the editor closes:

https://github.com/microsoft/vscode/blob/e5dadf5e8a00c1b84c77ac4197804967bd393ab5/src/vs/workbench/common/editor/textEditorModel.ts#L96-L98

☝️ this is what I have meant. The text editor will re-use a model that it doesn't own so I wonder what happens in case the actual owner(s) let it go. E.g this

• extension acquires model reference via API
• editor opens and reuses that model (line 96)
• extension reference is released (e.g after timeout) and model is disposed
• editor uses a disposed model?

[bpasero]

That cannot happen, text editors go through the reference way as well, otherwise we would have seen a ton of issues by now. Text file editors always create a reference here:

https://github.com/microsoft/vscode/blob/686512400b9cfbe747aa6e94af99797003cb0515/src/vs/workbench/contrib/files/common/editors/fileEditorInput.ts#L253

Still, the fact that we have code like this is dangerous, and possibly not used anymore or at least needs some better understanding. I can look into this as part of looking into the lifecycle of references.

[bpasero]

I checked back on the fact that file models and untitled models have their own way of creating text models and it seems fine because ITextModelResolverService is actually going through that code:

• untitled: https://github.com/microsoft/vscode/blob/2aa9f3f243b148b6441dc7d51808027dce68d6d2/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts#L160
• file: https://github.com/microsoft/vscode/blob/2aa9f3f243b148b6441dc7d51808027dce68d6d2/src/vs/workbench/services/textmodelResolver/common/textModelResolverService.ts#L42

There should be no issue from that usage that I can think of as long as everyone is going through these methods which they should.

[testforstephen]

When i try the latest insider (2020-05-27), i still can reproduce the issue with the sample project above.

Version: 1.46.0-insider (user setup)
Commit: 876f2e70f9a2e1988887f8ca82294418afac15a2
Date: 2020-05-27T05:43:41.115Z
Electron: 7.2.4
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.18363

Is the fix on the latest insider?

[bpasero]

Should be fixed, reopening then.

[jrieken]

Using the sample and steps below, this does seems to work for me. Tho, I wonder if we fixed the right issue because the code that @bpasero added isn't actually hit. Well the event fires but there is removal happening because the documents do exist on the extension host but they haven't been requested by an extension and are therefore not kept in the BoundReferenceCollection. This was still a good change but I wonder if we have missed something...

For more insights, I have an extension that logs all onDidOpen and onDidClose events for text documents and this is what it printed. At one point you see a series of CLOSE messages for the files that have been moved, that's followed by OPEN messages for the new locations, and finally come OPEN messages for the files that get updated

OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/tsconfig.json
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
OPEN: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
CLOSE: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
OPEN: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
CLOSE: vscode-userdata:/Users/jrieken/Library/Application Support/code-oss-dev/User/settings.json
OPEN: output:tasks
CLOSE: output:tasks
OPEN: output:extension-output-%234
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mb.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/md.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/mc.ts
CLOSE: file:///Users/jrieken/Code/_samples/devfest/issues/98057/tsconfig.json
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/utils/ma.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts","ref":""}
OPEN: file:///Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/fb.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mb.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/mc.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts","ref":""}
CLOSE: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/md.ts","ref":""}
OPEN: git:/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts.git?{"path":"/Users/jrieken/Code/_samples/devfest/issues/98057/src/funcs/ma.ts","ref":""}

@testforstephen Did you try Insiders with this test case or is there a more complicated one in the Java world?

[testforstephen]

1. Open the sample nodejs project below in VS Code. dnd.zip
2. Set the setting "typescript.updateImportsOnFileMove.enabled": "always".
3. Click to open multiple files (e.g. src/utils/ma.ts, src/utils/mb.ts, src/utils/mc.ts, src/utils/md.ts) in VS Code.
4. In File Explorer, drag and drop these files back and forth several times, e.g.

   1st dnd:  utils/m*.ts -> funcs/m*.ts
   2st dnd:  funcs/m*.ts -> utils/m*.ts
   3rd dnd:  utils/m*.ts -> funcs/m*.ts

   I tried the steps above in the latest insider 2020-06-03 again, the issue still exists. In fact, i also tried in a new windows machine, it has same issue.

@jrieken Did you check the window title? After DnD back and froth several times, you would see a (deleted) text in the window title.

In Java extension, what i want to do is same as "Update Imports on Move" feature in typescript. A little difference is that we provide the option for users to preview the update. The current problem is that after dragging files back and forth a few times, then applying preview will fail and it says "files were modified in the meantime". At the same time, i noticed there are additional title bar text "(deleted)" appended to these affected editors. Given it can also be reproduced in typescript, that's why i confirm it's a vscode issue.

[jrieken]

Oh, my bad. Didn't move back and forth, moved only once

[jrieken]

I can now reproduce but from the logs and debugging it seems that the extension host side is looking correct - I see meaningful CLOSE/OPEN messages and I do see how the remove function is executing.

Moving this to June for further investigation but @bpasero I believe there is more to this than just extension referenced documents

[jrieken]

A little difference is that we provide the option for users to preview the update.

FYI - our current plan is to add API, e.g a workspace edit can say that it can be previewed and based on user-preference the preview pane will show. Similar to how rename (symbol) preview works

[bpasero]

This seems to be the same underlying reason as https://github.com/microsoft/vscode/issues/98653. During the steps we close and open editors, but some of them are inactive. When these get closed (e.g. by doing another DND operation), we do not dispose the model because an editor only disposes a model if it was opened actively, not inactive. Since the models are dirty doing the operation, we refuse to dispose it when the bulk edits are done.

I think there is no issue with disposal of references, but the sequence is rather this:

• bulk edit creates model ref and performs edits
• bulk edits is done and disposes the ref
• we do not dispose the model because it is dirty at this point
• at this time we have a model without associated reference that can potentially leak
• we open dirty models by default in the background
• usually editors participate in the lifecycle of models by creating a reference, but not when opened in background (we do not call resolve in that case)
• operation is done again, since files are moved, we close editors and reopen them
• closing an inactive editor does not dispose any reference because none was created

I am not sure yet about the proper fix but tested locally that a fix for https://github.com/microsoft/vscode/issues/98653 should also fix this issue. Closing as duplicate.