Skip to content

Commit

Permalink
Merge pull request #8480 from fix-event-emitter-memory-leak
Browse files Browse the repository at this point in the history 
Fix (utils): Fix memory leak in EventEmitterMixin. See #8480.
  • Loading branch information
@jodator
jodator committed Nov 20, 2020
2 parents 7f9cebb + 672094f commit 6a7f381
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions packages/ckeditor5-utils/src/emittermixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,18 @@ const EmitterMixin = {
// All params provided. off() that single callback.
if ( callback ) {
removeCallback( emitter, event, callback );

// We must remove callbacks as well in order to prevent memory leaks.
// See https://github.com/ckeditor/ckeditor5/pull/8480
const index = eventCallbacks.indexOf( callback );

if ( index !== -1 ) {
if ( eventCallbacks.length === 1 ) {
delete emitterInfo.callbacks[ event ];
} else {
removeCallback( emitter, event, callback );

This comment has been minimized.

Sign in to view

Copy link
@bendemboski

bendemboski Mar 9, 2021

Contributor

@jodator this line is incorrect. This is doing the same thing that line 158 did. This needs to be emitterInfo.callbacks[ event ].splice( index, 1 ); or eventCallbacks.splice( index, 1 );

The memory leak still exists when removing the non-last callback.

This comment has been minimized.

Sign in to view

Copy link
@jodator

jodator Mar 10, 2021

Author Contributor

OH, I see that now :D I'm not sure why I've done that this way, but yes the intention was (probably) to remove the callback at the given index.

CC @Reinmar This looks like a straightforward fix. However, I do not recall ATM if we have or even could have a test for that change ;)

This comment has been minimized.

Sign in to view

Copy link
@bendemboski

bendemboski Mar 10, 2021

Contributor

Yeah, I'm sure that was the intention -- it's how it was in the original PR but somehow got messed up in the process of merging. And I guess y'all tried to test it but, like my when I looked at it, couldn't really figure out a good way to without a bigger refactor to make more things public outside the module.

This comment has been minimized.

Sign in to view

Copy link
@bendemboski

bendemboski Oct 21, 2021

Contributor

@jodator @Reinmar are you planning to fix this? The memory leak is still present...
}
}
}
// Only `emitter` and `event` provided. off() all callbacks for that event.
else if ( eventCallbacks ) {
Expand Down

0 comments on commit 6a7f381

Please sign in to comment.