"RangeError: Maximum call stack size exceeded" when DocumentSymbolProvider is used

[DaanVandenBosch]

monaco-editor version: 0.26.1
Browser: Chrome, Firefox
OS: Windows 10
Playground code that reproduces the issue:

class DSP {
    constructor() {
        this.selfRef = this;
    }

    provideDocumentSymbols() {
        return [];
    }
}

monaco.languages.registerDocumentSymbolProvider("javascript", new DSP)

monaco.editor.create(document.getElementById("container"), {
	value: "function hello() {\n\talert('Hello world!');\n}",
	language: "javascript"
});

Then hit Ctrl-Shift-O in the editor view on the right with the console open. The problem is that LanguageFeatureRequestDelays.update calculates a hash of the provider. The hash function used produces a stack overflow if the provider has circular references.

Stacktrace:

hash.ts:37 Uncaught (in promise) RangeError: Maximum call stack size exceeded
    at L (hash.ts:37)
    at m (hash.ts:46)
    at hash.ts:61
    at Array.reduce (<anonymous>)
    at i (hash.ts:60)
    at M (hash.ts:23)
    at hash.ts:62
    at Array.reduce (<anonymous>)
    at i (hash.ts:60)
    at M (hash.ts:23)
Promise.then (async) |   |  
-- | -- | --
  | create | @ | outlineModel.ts:127
  | (anonymous) | @ | gotoSymbolQuickAccess.ts:424
  | (anonymous) | @ | cancellation.ts:97
  | Ie | @ | cancellation.ts:97
  | getDocumentSymbols | @ | gotoSymbolQuickAccess.ts:423
  | doProvideWithEditorSymbols | @ | gotoSymbolQuickAccess.ts:149
  | provideWithTextEditor | @ | gotoSymbolQuickAccess.ts:62
  | doProvide | @ | editorNavigationQuickAccess.ts:115
  | provide | @ | editorNavigationQuickAccess.ts:65
  | doShowOrPick | @ | quickAccess.ts:128
  | show | @ | quickAccess.ts:35
  | run | @ | standaloneGotoSymbolQuickAccess.ts:65
  | runEditorCommand | @ | editorExtensions.ts:333
  | (anonymous) | @ | editorExtensions.ts:265
  | invokeFunction | @ | instantiationService.ts:61
  | invokeWithinContext | @ | codeEditorWidget.ts:379
  | runCommand | @ | editorExtensions.ts:258
  | handler | @ | editorExtensions.ts:120
  | invokeFunction | @ | instantiationService.ts:61
  | executeCommand | @ | simpleServices.ts:227
  | _doDispatch | @ | abstractKeybindingService.ts:226
  | _dispatch | @ | abstractKeybindingService.ts:143
  | (anonymous) | @ | simpleServices.ts:257

Edit: It also seems like a bad idea to repeatedly hash an entire object graph in code meant to improve performance.

[hediet]

@jrieken wouldn't it make more sense to use the object identity as key for the cache in LanguageFeatureRequestDelays? I have to agree that hashing like this is dangerous and it is unclear to me how functions (with captured variables) are hashed.

[aeschli]

@jrieken Can you move away from using hash on provider objects?
'hash' was implemented for JSON like data, currently not suited for objects with functions and circular references.
I will update the spec of hash for now and we can discuss improvements to hash in a separate feature request.

[jrieken]

I will update the spec of hash for now and we can discuss improvements to hash in a separate feature request.

Fair enough... Tho, a try-catch-return-const could also do it. This isn't a real issue, reported against monaco, since in vscode it is all free of cycles, and since the error seems to be reported against an old version. There is no more use of hash from outline.

[rcjsuen]

This isn't a real issue, reported against monaco, since in vscode it is all free of cycles, and since the error seems to be reported against an old version. There is no more use of hash from outline.

@jrieken If I try running the code from the playground now the RangeError is still thrown. The next version of Monaco will pick up the correct fix then?

[aeschli]

Things work when removing this.selfRef = this; from the sample.

[aeschli]

That said the use of hash in https://github.com/microsoft/vscode/blob/23fa383f8b44f9491871cf1a3e2dfb088cfc3eb1/src/vs/editor/common/modes/languageFeatureRegistry.ts#L197 is still fishy. I believe providers of the same kind (e.g all DocumentSynbolProviders) without any fields will have the same hash.

[jrieken]

You are right...