potential listener LEAK detected in menubarControl

[alexdima]

• open settings.json
• add "editor.renameOnType": true
• hover over the warning and select some text
• click in the editor
• repeat...

Error
    at Stacktrace.create (event.js:444:35)
    at PersistedMenuHideState._event [as onDidChange] (event.js:542:44)
    at Object.startLazyListener [as onFirstListenerAdd] (menuService.js:167:48)
    at Menu._event [as onDidChange] (event.js:536:39)
    at menubarControl.js:81:49
    at Array.forEach (<anonymous>)
    at NativeMenubarControl.populateMenuItems (menubarControl.js:72:25)
    at NativeMenubarControl.getMenubarMenus (menubarControl.js:59:26)
    at NativeMenubarControl.doUpdateMenubar (menubarControl.js:46:22)
    at menubarControl.js:139:89
    at Listener.invoke (event.js:461:27)
    at PrivateEventDeliveryQueue.deliver (event.js:631:38)
    at DebounceEmitter.fire (event.js:596:37)
    at DebounceEmitter.resume (event.js:723:31)
    at event.js:757:26

[bpasero]

Seeing a similar stack but for custom menus when running browser integration tests:

[866] potential listener LEAK detected, having 306 listeners already. MOST frequent listener (89): [
  '[866] potential listener LEAK detected, having 306 listeners already. MOST frequent listener (89):'
]
Error
    at Stacktrace.create (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:444:35)
    at PersistedMenuHideState._event [as onDidChange] (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:542:44)
    at Object.startLazyListener [as onFirstListenerAdd] (http://localhost:9888/oss-dev/static/out/vs/platform/actions/common/menuService.js:167:48)
    at Menu._event [as onDidChange] (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:536:39)
    at updateActions (http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:538:56)
    at CustomMenubarControl.setupCustomMenubar (http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:596:21)
    at CustomMenubarControl.doUpdateMenubar (http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:387:18)
    at http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:139:89
    at Listener.invoke (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:461:27)
    at PrivateEventDeliveryQueue.deliver (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:631:38) [
  'Error\n' +
    '    at Stacktrace.create (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:444:35)\n' +
    '    at PersistedMenuHideState._event [as onDidChange] (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:542:44)\n' +
    '    at Object.startLazyListener [as onFirstListenerAdd] (http://localhost:9888/oss-dev/static/out/vs/platform/actions/common/menuService.js:167:48)\n' +
    '    at Menu._event [as onDidChange] (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:536:39)\n' +
    '    at updateActions (http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:538:56)\n' +
    '    at CustomMenubarControl.setupCustomMenubar (http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:596:21)\n' +
    '    at CustomMenubarControl.doUpdateMenubar (http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:387:18)\n' +
    '    at http://localhost:9888/oss-dev/static/out/vs/workbench/browser/parts/titlebar/menubarControl.js:139:89\n' +
    '    at Listener.invoke (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:461:27)\n' +
    '    at PrivateEventDeliveryQueue.deliver (http://localhost:9888/oss-dev/static/out/vs/base/common/event.js:631:38)'
]

[jrieken]

Things are better after #161465 but something still seems to be off. When skipping this if-statement and basically emitting a menu event per context key event the leak warning reappear and they are rooted at the native menu...

[jrieken]

So, this is the problem: there is abstract MenubarControl (left editor) which is extended by NativeMenubarControl (right editor). The super-type defines a this.menus which stores menus by their title and submenus by their MenuId.

Whenever a change happens for the global menu (MenubarMainMenu) the map is reset and all menus are re-created. Now the subtypes registers things via _register which means lifecycle of the main menu control itself - that doesn't get disposed and therefore menus pile up.

[jrieken]

This leak is definitely over-exposed by a previous bug of emitting way too many menu change events but it will also happen without that, just less dramatic.

[jrieken]

I pushed 4768357 which prevent the leak but there is some debt which can be simplified. I am thinking of the following items

• for each submenu a change-listener is registered which rebuild the whole global menu (not just that submenu). That isn't needed: the global menu can be created with the emitEventsForSubmenuChanges so that only a single listener is needed
• for each SubmenuItemAction the corresponding menu is created and disposed so that its actions can be extracted. This is a little wasteful and instead SubmenuItemAction#actions should be used (it is all actions of the corresponding submenu)

[sbatten]

@jrieken what is the recommended way to get the submenuitemaction actions since I need the group information that the menu getActions provides. Also, are context keys and such observed?

[jrieken]

what is the recommended way to get the submenuitemaction actions since I need the group information that the menu getActions provides.

That's implicit by the use of Separator-instances. So, instead the array of arrays you have a single array that already contains separators. It's less model-information but I believe it makes this logic simpler

Also, are context keys and such observed?

Not sure what you mean by that?

[sbatten]

I guess I'm just confused by the typings. I'm getting a list of IActions. In this context I'm using a lot of information from the menu typings like mnemonic labels and toggled states. I'll continue this in debt week as I don't want to change all the menubar creation on the last day of the iteration.

[jrieken]

Oh, now I get it... Yeah you can safely check for/cast to SubmenuItemAction and MenuItemAction

[sbatten]

@jrieken after fixing this, there is one minor nuisance that comes out of not manually listening to every submenu onDidChange event. The repro is this:

I tested on windows with custom menus:

1. Enable profiles and create a named profile
2. Use File>Preferences>Settings Profiles>Rename (profile name)
3. Accept the rename and navigate to the menu entry

🐛 it still shows the old name

Another context key update eventually gets the entry to update pretty quickly, but since the menu changes but not due to context keys, I don't think they are propagated up.

[jrieken]

but since the menu changes but not due to context keys, I don't think they are propagated up.

Actions (menu items) only react to context keys that they spell out, not to all changes. I am not familiar how the rename profiles command is implemented. Maybe there needs to be another special event for when profiles change?

[oskarkk]

Hi, I'm using monaco-editor in React, and when I've upgraded it from 0.33 (released ca. 8 months ago) to 0.34 (released a month ago), I'm constantly getting an error when the editor mounts, and the only place that showed up when i googled PersistedMenuHideState is here, so maybe you will know something about what went wrong or point me in some direction to fix it. I'll probably make a new issue in monaco-editor repo about this.

Here's the error:

 Uncaught Error: _storageService.onDidChangeValue is not a function

PersistedMenuHideState@http://c4.localhost:3025/web/static/js/0.chunk.js:244616:43
MenuService@http://c4.localhost:3025/web/static/js/0.chunk.js:244575:26
_createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251511:191
_createServiceInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251686:21
_createServiceInstanceWithOwner@http://c4.localhost:3025/web/static/js/0.chunk.js:251666:21
_createServiceInstanceWithOwner@http://c4.localhost:3025/web/static/js/0.chunk.js:251668:29
_createAndCacheServiceInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251641:35
_safeCreateAndCacheServiceInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251558:21
_getOrCreateServiceInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251541:21
_createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251483:30
createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251455:23
ContentHoverController@http://c4.localhost:3025/web/static/js/0.chunk.js:190899:62
_createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251511:191
createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251455:23
_getOrCreateContentWidget@http://c4.localhost:3025/web/static/js/0.chunk.js:192121:58
_onEditorMouseMove@http://c4.localhost:3025/web/static/js/0.chunk.js:192065:32
_hookEvents/<@http://c4.localhost:3025/web/static/js/0.chunk.js:191936:25
invoke@http://c4.localhost:3025/web/static/js/0.chunk.js:41145:21
deliver@http://c4.localhost:3025/web/static/js/0.chunk.js:41407:28
fire@http://c4.localhost:3025/web/static/js/0.chunk.js:41350:29
_createView/viewUserInputEvents.onMouseMove@http://c4.localhost:3025/web/static/js/0.chunk.js:94869:36
emitMouseMove@http://c4.localhost:3025/web/static/js/0.chunk.js:80010:71
emitMouseMove@http://c4.localhost:3025/web/static/js/0.chunk.js:78717:28
_onMouseMove@http://c4.localhost:3025/web/static/js/0.chunk.js:65612:27
MouseHandler/<@http://c4.localhost:3025/web/static/js/0.chunk.js:65409:13
onMouseMove/<@http://c4.localhost:3025/web/static/js/0.chunk.js:73654:16
EventListener.handleEvent*DomListener@http://c4.localhost:3025/web/static/js/0.chunk.js:2479:16
addDisposableListener@http://c4.localhost:3025/web/static/js/0.chunk.js:2502:10
onMouseMove@http://c4.localhost:3025/web/static/js/0.chunk.js:73653:64
MouseHandler@http://c4.localhost:3025/web/static/js/0.chunk.js:65408:33
PointerHandler@http://c4.localhost:3025/web/static/js/0.chunk.js:67850:41
View@http://c4.localhost:3025/web/static/js/0.chunk.js:77287:45
_createView@http://c4.localhost:3025/web/static/js/0.chunk.js:94900:18
_attachModel@http://c4.localhost:3025/web/static/js/0.chunk.js:94732:36
StandaloneEditor@http://c4.localhost:3025/web/static/js/0.chunk.js:236076:12
_createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251511:191
createInstance@http://c4.localhost:3025/web/static/js/0.chunk.js:251455:23
create@http://c4.localhost:3025/web/static/js/0.chunk.js:236599:31
initMonaco@http://c4.localhost:3025/web/static/js/0.chunk.js:264678:102
invokePassiveEffectCreate@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:286011:24
callCallback@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:266625:18
invokeGuardedCallbackDev@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:266674:20
invokeGuardedCallback@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:266734:35
flushPassiveEffectsImpl@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:286093:34
unstable_runWithPriority@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:306907:16
runWithPriority$1@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:274031:14
flushPassiveEffects@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:285970:18
commitBeforeMutationEffects/<@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:285851:15
workLoop@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:306857:46
flushWork@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:306831:18
performWorkUntilDeadline@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:306596:52
EventHandlerNonNull*./node_modules/scheduler/cjs/scheduler.development.js/<@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:306620:7
./node_modules/scheduler/cjs/scheduler.development.js@http://c4.localhost:3025/web/static/js/vendors~main.chunk.js:307079:5
__webpack_require__@http://c4.localhost:3025/web/static/js/bundle.js:857:31