Fix event leak on reuse of touchbar item #12527
Conversation
lib/browser/api/touch-bar.js
Outdated
| @@ -118,6 +118,9 @@ class TouchBar extends EventEmitter { | |||
| window.removeListener('closed', removeListeners) | |||
| window._touchBar = null | |||
| delete this.windowListeners[id] | |||
| for (const item of this.ordereredItems) { | |||
| item.removeListener('change', this.changeListener) | |||
There was a problem hiding this comment.
I think this is undoing the item.on('change', this.changeListener) in registerItem(), but the next line there is to also register each of item.child.ordereredItems iff item.child is a TouchBar. Do we need to undo that here too?
If we don't also need that, then I'm 👍 on this PR
There was a problem hiding this comment.
edit: and walking recursively down all TouchBar subtrees
There was a problem hiding this comment.
edit 2: do we also need to remove the escapeItem changeListener too?
There was a problem hiding this comment.
I now clean up recursively and deal with the escapeItem listener
lib/browser/api/touch-bar.js
Outdated
| @@ -118,6 +118,9 @@ class TouchBar extends EventEmitter { | |||
| window.removeListener('closed', removeListeners) | |||
| window._touchBar = null | |||
| delete this.windowListeners[id] | |||
| for (const item of this.ordereredItems) { | |||
There was a problem hiding this comment.
'ordereredItems'? not new to this PR, but... 🙄 😄
There was a problem hiding this comment.
items is a map of id --> item
orderedItems is an array giving you the left->right order of those items containing references to the objects in the items map
There was a problem hiding this comment.
Right, I get it. Just amused by the spelling ordereredItems 🥈
There was a problem hiding this comment.
Oh, wow, ok. That's a typo I never noticed 😆
|
We have automatically backported this PR to "1-8-x", please check out #12623 |
|
We have automatically backported this PR to "2-0-x", please check out #12624 |
Fixes #12508