Fix for: memory leaks #2969

engineering-this · 2019-03-21T12:21:39Z

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

List of found and fixed memory leaks/ potential memory leaks:

Magicline

Magicline plugin exposes backdoor object property for debugging purposes in CKEDITOR.plugins.magicline, this object is replaced with each editor creation.
To fix memory leaks I've moved this property into editor._.magicLineBackdoor, so all references are removed along with editor instance.

Widget styles

Widget plugin keeps registered styles in styleGroups local variable. With every editor creation new styles are added. I've added check that prevents adding same styles again. At first I've used styleDefinition object for comparisement, but easyimage creates them dynamically, so I've changed solution to deeply compare object, to make sure only new styles are stored.

Script `onload` and `onerror`

CKEDITOR.scriptLoader and CKEDITOR.loader weren't removing onload/onerror listeners once loading is finished. Listeners keeps references to things that otherwise might be cleaned by Garbage Collector.

Image2 dialog

Image2 plugin adds global listener via CKEDITOR.on( 'dialogDefinition' ) for each editor. Listener was never removed, thus editor reference was leaking. I've removed this listener on editor destroy.

Codesnippet highlighter

Codesnippet plugin on beforeInit registers method CKEDITOR.plugins.registered.setHighlighter which is bound to editor instance, hence editor reference is leaking. According to docs.

This method can only be called while initialising plugins (in one of the three callbacks).

To fix leak I've forced removal of this method on pluginsLoaded.

Autocomplete panel

Element cke_autocomplete_panel was leaking after each editor creation. It is fixed by calling autocomplete.destroy on editor.destroy.

CKEDITOR.currentInstance

CKEDITOR.currentInstance points at editor even after it was destroyed. Added on destroy listener, to clean that.

skin.js

In skin.jsthere is stored reference to style element for each editor. I've added destroy listener which removes the reference.

Dialog cover

Dialog cover reference is stored in currentCover variable. I've forced removing reference when dialog is hidden.

Testing

For manual testing enable test on your current branch by:
git checkout memory-test tests/_assets/memorytest.html

Then read:
https://github.com/ckeditor/ckeditor-dev/blob/memory-test/tests/core/memory/memory.md

master branch

After applying fix for magicline

Snapshots

After page load.
After creating and destroying first editor.
After creating and destroying second editor.
After 100 next cycles of create-destroy editor.

Notes

First editor takes plenty of memory which is not released, but most of it is reused by next editors.
Second editor takes more memory than any next editors. I assume that Chrome after second creating of editor performs some optimizations which trade some memory for performance.

Other info

Branch is targeted on major because it uses CKEDITOR.tools.array.find.

Closes #589

f1ames

I have tested the optimizations both in our tool and with CHrome dev console and the improvement is easily visible:

Create - destroy cycles

Before (current `master`)

After (this PR)

First of all the overall memory used after destroying editor instance is lower (~7 vs ~9 MB). Also the difference between consecutive create/destroy cycles is much lower (~0.2 vs >1 MB). Also the overall delta (so the number of elements/resources left after cycles) decreased significantly 👍

Create - edit - destroy cycles

This test included some editing (changing text colors, using widgets, dialogs, dropdowns) between create and destroy. The results are similar:

Before (current `master`)

After (this PR)

Of course the memory usage is bigger than it the create/destroy case but the optimization is clearly visible 👍

I think the overall result is very satisfying for us. As for:

First editor takes plenty of memory which is not released, but most of it is reused by next editors.

I think it is caused by some shared resources and additional scripts which are loaded on editor creation. This resources are reused so we shouldn't try to remove them on destroying editors (because they will need to be loaded again which slows down significantly editor creation).

Second editor takes more memory than any next editors. I assume that Chrome after second creating of editor performs some optimizations which trade some memory for performance.

I'm not 100% sure about this, but it was my assumption too. And the fact that later create/destroy cycles don't increase memory usage almost at all is a good think.

I have added two small comment about test coverage, so you could take a look if we could do something about it. Also it would be good to include this fix in the next minor release (to have it released as fast as possible) so we should do something about CKEDITOR.tools.array.find:

You can port it from major to master.
You can try to do the same without using this method (maybe our array.forEach or array.filter could be useful here)?

The second options should be simpler IMHO.

Last thing, two of the newly added tests are failing on the built version of the editor:

Looks like some issue with async code:

f1ames · 2019-03-21T16:34:08Z

core/skin.js


 				// Add stylesheet if missing.
 				if ( !iframe.getById( 'cke_ui_color' ) ) {
 					var node = getStylesheet( iframe );
 					uiColorMenus.push( node );

+					// Cleanup after destroying editor (#589).
+					editor.on( 'destroy', function() {
+						uiColorMenus = CKEDITOR.tools.array.filter( uiColorMenus, function( storedNode ) {


There is no unit test for this change. I am not sure how hard it would be to test it (since there are not many skin.js test), but maybe you could give it a try?

☝️ Now I have realised that uiColorMenus is just some local var so probably can't be accessed from tests code directly. If that's the case we may skip unit test here.

f1ames · 2019-03-21T17:09:33Z

plugins/widget/plugin.js

+				group = styleGroups[ widgetName ][ groupName ];
+
+				// Don't push style if it's already stored (#589).
+				if ( !CKEDITOR.tools.array.find( group, getCompareFn( style ) ) ) {


I would be for covering this code with some tests. I know it's local function but maybe saveStyleGroup() can be somehow exposed for testing purposes?

Even if we made saveStyleGroup public we still don't have access to styleGroups local variable, so we would have to expose also it.
We could expose them both under CKEDITOR.style.customHandlers.widget._. But I wonder is it really worth make them public only for testing purposes?

😭 I don't think it is worth the effort, plus I have checked where this styleGroups are used and it's only in removeStylesFromSameGroup() method there which is covered with unit test so if saveStyleGroup() breaks the test should fail too. I think we are covered here 👍

engineering-this · 2019-03-22T11:44:10Z

I have added two small comment about test coverage, so you could take a look if we could do something about it. Also it would be good to include this fix in the next minor release (to have it released as fast as possible) so we should do something about CKEDITOR.tools.array.find:

You can port it from major to master.
You can try to do the same without using this method (maybe our array.forEach or array.filter could be useful here)?
The second options should be simpler IMHO.

I'd just copy array.tools.find into private method, we can refactor it as soon as this branch is merged into master, and master into major. Probably reverting the commit on major would be sufficient.

Edit:

Rebased branch on master

Once this is merged into major again as a refactoring just revert commit 145b12b9a.

f1ames · 2019-03-25T11:03:03Z

Rebased on latest master.

f1ames

The core/loader test fails in IE8:

I was also thinking about improving the changelog entry but haven't come up with anything better I am afraid. Maybe something like:

Fixed: Editor causes memory leaks in create / destroy cycles.

or

Fixed: Editor does not clean up its resources after destroy causing memory leaks.

engineering-this · 2019-03-25T15:39:43Z

Fixed: Editor causes memory leaks in create / destroy cycles.

I like this one more. I didn't want to be too general as it might sound as we fixed all the possible memory leaks, but didn't have any better idea.

Fixed IE failing test by choosing correct event name in same manner as callback function is added.

f1ames · 2019-03-26T13:28:15Z

Rebased on latest master.

f1ames

LGTM 🎉

From ckeditor#2969 Excludes changes to: * plugins/widget/plugin.js - change can't apply cleanly. * plugins/autocomplete/plugin.js - file doesn't exist on this version.

f1ames self-requested a review March 21, 2019 12:24

engineering-this mentioned this pull request Mar 21, 2019

Fix for: magicline plugin leaks memory #2963

Closed

2 tasks

f1ames self-assigned this Mar 21, 2019

f1ames suggested changes Mar 22, 2019

View reviewed changes

CKEditorBot unassigned f1ames Mar 22, 2019

engineering-this self-assigned this Mar 22, 2019

engineering-this changed the base branch from major to master March 22, 2019 13:34

engineering-this force-pushed the t/589 branch from 22e61f6 to 145b12b Compare March 22, 2019 13:36

engineering-this requested a review from f1ames March 22, 2019 16:02

CKEditorBot unassigned engineering-this Mar 22, 2019

f1ames force-pushed the t/589 branch from c42f139 to 8d61948 Compare March 25, 2019 11:02

f1ames mentioned this pull request Mar 25, 2019

Add unit tests for memory leaks #2977

Open

f1ames suggested changes Mar 25, 2019

View reviewed changes

engineering-this self-assigned this Mar 25, 2019

engineering-this requested a review from f1ames March 25, 2019 15:42

CKEditorBot unassigned engineering-this Mar 25, 2019

f1ames self-assigned this Mar 26, 2019

engineering-this added 10 commits March 26, 2019 14:25

Prevent adding same styles.

9e2cfbc

Remove script listeners after load/error.

4343124

Remove listener on editor destroy.

06af05b

Improved comparing stored styles with new ones.

0b47c15

Remove setHighlighter once it can't be used.

7e2af54

Call autocomplete.destroy on editor.destroy.

b64b633

Remove currentInstance if it's destroyed.

954eaee

Remove stored node when editor is destroyed.

e87a2c0

Remove element reference once it's removed.

3af7dcb

Tests: added test for script load listeners.

d5bcd3d

engineering-this added 15 commits March 26, 2019 14:25

Tests: added unit test.

0b8ba72

Change event type to pluginsLoaded.

ca059f7

Tests: added test case and updated tests after changes.

62a422a

Tests: added test cast.

d5102be

Fixed typeerror when destroy is called when view never was attached.

41a5ee3

Tests: added unit test.

d31edb4

Moved backdoor property to private editor properties.

091a0e9

Tests: updated after changes.

5959863

Tests: added unit test.

1426865

Changelog: added new entry.

9200c0b

Copied absent CKEDITOR.tools.array.find into local function.

80865ba

Tests: ignore on built version.

18a8fbe

Tests: fixed on built version.

b0e5d10

Tests: fixed on IE.

214722c

Changelog: rephrased entry.

8e0dd65

f1ames force-pushed the t/589 branch from c647e31 to 8e0dd65 Compare March 26, 2019 13:27

f1ames approved these changes Mar 26, 2019

View reviewed changes

CKEditorBot unassigned f1ames Mar 26, 2019

f1ames merged commit 20caee9 into master Mar 26, 2019

CKEditorBot deleted the t/589 branch March 26, 2019 14:44

mixonic mentioned this pull request May 17, 2019

Backport memory leak fixes. Addepar/ckeditor-dev#13

Merged

engineering-this mentioned this pull request Jun 26, 2019

Fix for: Errors thrown when Editor is detached and destroyed during initialization #3128

Merged

2 tasks

f1ames mentioned this pull request Nov 28, 2019

Memory leak after having multiple inline editor instances of ckeditor #3690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for: memory leaks #2969

Fix for: memory leaks #2969

engineering-this commented Mar 21, 2019

f1ames left a comment

f1ames Mar 21, 2019

f1ames Mar 21, 2019

f1ames Mar 21, 2019

engineering-this Mar 22, 2019 •

edited

Loading

f1ames Mar 22, 2019

engineering-this commented Mar 22, 2019 •

edited

Loading

f1ames commented Mar 25, 2019

f1ames left a comment

engineering-this commented Mar 25, 2019 •

edited

Loading

f1ames commented Mar 26, 2019

f1ames left a comment

Fix for: memory leaks #2969

Fix for: memory leaks #2969

Conversation

engineering-this commented Mar 21, 2019

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Magicline

Widget styles

Script onload and onerror

Image2 dialog

Codesnippet highlighter

Autocomplete panel

CKEDITOR.currentInstance

skin.js

Dialog cover

Testing

Other info

f1ames left a comment

Choose a reason for hiding this comment

Create - destroy cycles

Before (current master)

After (this PR)

Create - edit - destroy cycles

Before (current master)

After (this PR)

f1ames Mar 21, 2019

Choose a reason for hiding this comment

f1ames Mar 21, 2019

Choose a reason for hiding this comment

f1ames Mar 21, 2019

Choose a reason for hiding this comment

engineering-this Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

f1ames Mar 22, 2019

Choose a reason for hiding this comment

engineering-this commented Mar 22, 2019 • edited Loading

f1ames commented Mar 25, 2019

f1ames left a comment

Choose a reason for hiding this comment

engineering-this commented Mar 25, 2019 • edited Loading

f1ames commented Mar 26, 2019

f1ames left a comment

Choose a reason for hiding this comment

Script `onload` and `onerror`

Before (current `master`)

Before (current `master`)

engineering-this Mar 22, 2019 •

edited

Loading

engineering-this commented Mar 22, 2019 •

edited

Loading

engineering-this commented Mar 25, 2019 •

edited

Loading