Memory leak when creating many decoration types and disposing them

[ArtemGovorov]

• VSCode Version: 1.2.0
• OS Version: OSX

Now when it's possible to set the contentText for a decoration with this change, one may need to create quite a few transient decoration types to display/update the side content for certain code lines.

I tried to simulate my scenario and it looks like some resources are not cleared when disposing a decoration type.

Steps to Reproduce:

Here is the test code:

  let getRandomInt = (min, max) => Math.floor(Math.random() * (max - min)) + min;
  let decorationTypes = [];
  let editor = window.visibleTextEditors[0];
  setInterval(function () {
    decorationTypes.forEach(dt => {
      editor.setDecorations(dt, []);
      dt.dispose();
    });
    decorationTypes.length = 0;
    for (let i = 0; i < 1000; i++) {
      let dt = window.createTextEditorDecorationType({after: {contentText: getRandomInt(0, Number.MAX_SAFE_INTEGER)}});
      editor.setDecorations(dt, [new vscode.Range(i, 0, i, 1000)]);
      decorationTypes.push(dt);
    }
  }, 1);

The memory consumption grows rapidly and after a couple of minutes I get the process crash:

Note that the issue can be reproduced with the current release version on VS Code (not only the insiders build), so it wasn't introduced by the change.

[aeschli]

Note that decoration types are kept until you call dispose on them. I will have a look, but it's clear you can't create too many of them.
With the latest change you don't need to create a decoration type for every attachment text, but can refine the type by defining before & after attachments on the decoration itself. These decoration are more expensive to be build (css rules need to be added) but are cleaned up as soon as not shown anymore.

[ArtemGovorov]

Note that decoration types are kept until you call dispose on them.

I call dispose in the provided test code for all decoration types. There still must be something somewhere that is not cleaned up.

With the latest change you don't need to create a decoration type for every attachment text, but can refine the type by defining before & after attachments on the decoration itself.

Oh, it's awesome, I didn't realise that, thanks for noting it. It fits my scenario much better than creating a lot of decoration types.

[alexdima]

The refCount property is not initialized on DecorationTypeOptionsProvider, making provider.refCount++ turn it into NaN, making this code in removeDecorationType not succeed:

            provider.refCount--;
            if (provider.refCount <= 0) {
                delete this._decorationOptionProviders[key];
                provider.dispose();
                this.listCodeEditors().forEach((ed) => ed.removeDecorations(key));
            }

Spotted and fixed while merging #6553

[alexdima]

Fixed while merging #6553 with 9e7b462

[aeschli]

Thanks @alexandrudima !

[isidorn]

Using the snippet user provided I can still crash VScode after 1 minute, not sure if that is expected (there is a timeout of 1ms, so it is really adding a bunch of decorations). Though I do not get an error in the console, just after running the extension whole vscode crashes.

Reopening so @alexandrudima can decide if this is expected or not

[alexdima]

I have a fix in a09ee36

[alexdima]

@jrieken Pushed fix to release/1.3.0. The fix is to clear all data associated with a decoration type in the editor. Please verify that those members are always accessed in a safe way. The contract is that the decoration type will always be unique and once removed, it will never come back (the change has no other side effects except fixing the memory leak).

[jrieken]

Change looks good, works in scripts/code.sh. I will verify with the build