Removed memory leak caused by CKEDITOR.filter.instances #1770

jacekbogdanski · 2018-03-09T08:57:03Z

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?

Added additional option to CKEDITOR.filter constructor to pass an editor owning filter instance. Changed editor.destroy function to remove correct editors and refactored plugins using CKEDITOR.filter. With this change we are now able to remove orphaned editor filters and prevent memory leak with CKEDITOR.filter.instances.

Additional parameter doesn't look most beautiful because we should probably go with something like new CKEDITOR.filter( editor, rules) but it would require to provide breaking change for our API.

Based on #1690

Closes #1722, closes #2466

Comandeer

The fix does not cover filters for nested editables, which should be destroyed after destroying the widget, not the whole editor.

Comandeer · 2018-04-20T10:02:27Z

CHANGES.md

@@ -1,6 +1,12 @@
 CKEditor 4 Changelog
 ====================

+## CKEDITOR 4.9.1


Please update version tag.

Comandeer · 2018-04-20T10:32:08Z

core/filter.js

 	 *
 	 * @since 4.1
 	 * @class
 	 * @constructor Creates a filter class instance.
 	 * @param {CKEDITOR.editor/CKEDITOR.filter.allowedContentRules} editorOrRules
+	 * @param {CKEDITOR.editor} owner Editor owning the filter instance.


I don't like it. We already have editor as a first parameter. We could make that change compatible with older versions by allowing to optionally pass rules as a second parameter. It seems like a more elegant solution:

CKEDITOR.filter = function( editorOrRules, rules )

Please also note that optional parameters should be marked with [] (so [owner], not owner).

The only issue with such approach is the fact that new CKEDITOR.filter( editor ) creates new filter based on editor's features. However I still think that something like new CKEDITOR.filter( editor, {} ) or new CKEDITOR.filter( editor, null ) is more readable than new CKEDITOR.filter( null, editor ).

I agree that it's not beautiful. But let's say you would like to create empty filter and setup it by CKEDITOR.filter#allow function e.g

var filter = new CKEDITOR.filter(); filter.allow( {...} );

In that case, you won't be able to create a filter with owning editor - if you try to pass editor as an argument you will create a filter based on editor (https://github.com/ckeditor/ckeditor-dev/blob/major/core/filter.js#L168).

We have that case in our code (https://github.com/ckeditor/ckeditor-dev/blob/major/plugins/clipboard/plugin.js#L1347) and it's possible that people could use it in that way also.

Actually you would be still able to create such filter:

var filter = new CKEDITOR.filter( editor, {} ); filter.allow( {} );

I agree it can be done but with that change, we will provide breaking change for our API. If we are ok with that I also prefer new CKEDITOR.filter( editorOrRules, rules ) signature. WDYT @Comandeer?

Comandeer · 2018-04-20T11:40:24Z

tests/_benderjs/ckeditor/static/tools.js

+		 * @param  {Object} obj
+		 * @return {Array} object values.
+		 */
+		objToArray: function( obj ) {


This change does not seem to be directly connected with the fix. Why is it present here?

~~I changed it because this PR is based on #1690. Now I think maybe it shouldn't be changed here as it's a separate issue.~~

I'm using this function inside unit tests for proposed changes.

jacekbogdanski · 2018-04-27T09:53:12Z

Rebased into the latest major.

jacekbogdanski · 2018-05-07T10:31:50Z

I left CKEDITOR.filter signature unchanged - please refer to #1770 (comment) and if you think we can provide breaking change for API I will refactor the code.
About destroying nested editables inside widget plugin - a problem with this approach is that we are sharing the same filter for each widget instance:
https://github.com/ckeditor/ckeditor-dev/blob/045ae8e15cbfc85872748d737e8e54fb414acbcd/plugins/widget/plugin.js#L2202-L2224
So if we destroy nested editable filter on widget destroy it will become unavailable for other widgets of the same type. It's not a problem when destroying editor because in that case widgets are already / will be destroyed so filters are not needed anymore. I could implement some workaround like destroying filter only if it's not used by widgets but not sure if it's correct way.

Comandeer · 2018-05-11T15:40:25Z

Hmm, the main ticket is about destroying filters in nested editables, so it would be nice to actually implement it. I think that workaround to detect any nested editable still using this filter should be enough.

Comandeer

Please rebase onto latest major.

Comandeer · 2018-09-19T10:18:30Z

core/filter.js

 	 *
 	 * @since 4.1
 	 * @class
 	 * @constructor Creates a filter class instance.
 	 * @param {CKEDITOR.editor/CKEDITOR.filter.allowedContentRules} editorOrRules
+	 * @param {CKEDITOR.editor} rules This parameter is available since 4.10.0.


Please update version tag.

Also this parameter is not of CKEDITOR.editor type.

Comandeer · 2018-09-19T10:27:38Z

core/filter.js

@@ -165,11 +175,12 @@
 		// Register filter instance.
 		CKEDITOR.filter.instances[ this.id ] = this;

-		if ( editorOrRules instanceof CKEDITOR.editor ) {
-			var editor = this.editor = editorOrRules;
+		this.editor = editorOrRules instanceof CKEDITOR.editor ? editorOrRules : null;


Why not:

var editor = this.editor = editorOrRules instanceof CKEDITOR.editor ? editorOrRules : null;

? With such version there won't be need to change all references to the editor.

Comandeer · 2018-09-19T10:40:42Z

tests/core/editor/destroy.js

+
+	function countFilters( editor ) {
+		var filters = bender.tools.objToArray( CKEDITOR.filter.instances );
+		return editor ? filters.filter( function( filter ) {


This function won't work in IE8, as there is no Array.prototype.filter. Our own CKEDITOR.tools.array.filter should be used here.

Comandeer · 2018-09-19T10:41:14Z

tests/core/editor/manual/filterdestroy.md

@@ -0,0 +1,14 @@
+@bender-tags: bug, 4.9.1, 1722


Please update version tag.

Comandeer · 2018-09-19T10:41:49Z

tests/plugins/widget/manual/nestededitablefilters.md

@@ -0,0 +1,14 @@
+@bender-tags: widget, bug, 4.10.0, 1722


Please update version tag.

Comandeer · 2018-10-08T10:47:38Z

Rebased onto latest major and changed base.

Comandeer

Fix looks good, only some bureaucracy left. However it seems that filters are still leaking, due to #1452…

Comandeer · 2018-10-08T11:48:47Z

CHANGES.md

@@ -32,6 +32,7 @@ Fixed Issues:
 * [#421](https://github.com/ckeditor/ckeditor-dev/issues/421) Fixed: Expandable [Button](https://ckeditor.com/cke4/addon/button) puts `(Selected)` text at the end of the label when clicked.
 * [#1454](https://github.com/ckeditor/ckeditor-dev/issues/1454): Fixed: [Upload Widget](https://ckeditor.com/cke4/addon/uploadwidget) [`onAbort`](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.fileTools.uploadWidgetDefinition-property-onAbort) method is not called when loader is aborted.
 * [#1451](https://github.com/ckeditor/ckeditor-dev/issues/1451): Fixed: Context menu is incorrectly positioned when opened with `Shift-F10`.
+* [#1722](https://github.com/ckeditor/ckeditor-dev/issues/1722): [`CKEDITOR.filter.instances`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_filter.html#static-property-instances) is causing memory leaks.


Please create followup ticket that will describe changes in API and reference it in "API Changes" section.

Comandeer · 2018-10-08T12:12:30Z

core/filter.js

 	 *
 	 * @since 4.1
 	 * @class
 	 * @constructor Creates a filter class instance.
 	 * @param {CKEDITOR.editor/CKEDITOR.filter.allowedContentRules} editorOrRules
+	 * @param {CKEDITOR.filter.allowedContentRules} rules This parameter is available since 4.10.2


Please update version and add period at the end.

Please also mark this parameter as optional.

Comandeer · 2018-10-08T12:14:23Z

core/filter.js

+	 * ```
+	 *
+	 * If filter is only used by a single editor instance, you should pass editor instance alongside with the rules
+	 * so the filter could be removed with {@link CKEDITOR.editor#destroy} method.


There should be additional info about what will happen if editor is passed without rules as it changes the behaviour of the constructor in not so obvious way.

Isn't enough described by the whole https://github.com/ckeditor/ckeditor-dev/blob/013af363d892680f9c0a2f11765aee45ec09ca2b/core/filter.js#L33-L56 docs? 🤔 However, I added additional info that passing only editor ( without rules ) will also prevent memory leaks.

Comandeer

LGTM!

Implements most of ckeditor#1770

Comandeer requested changes Apr 20, 2018

View reviewed changes

jacekbogdanski force-pushed the t/1722 branch from f94b5c9 to 9925301 Compare April 27, 2018 09:00

jacekbogdanski force-pushed the t/1722 branch 2 times, most recently from c1de513 to 045ae8e Compare May 7, 2018 10:18

jacekbogdanski requested a review from Comandeer May 7, 2018 10:31

Comandeer requested changes May 11, 2018

View reviewed changes

jacekbogdanski force-pushed the t/1722 branch from 045ae8e to 3eb74d0 Compare June 6, 2018 07:20

jacekbogdanski requested a review from Comandeer June 6, 2018 11:04

mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jul 10, 2018

jacekbogdanski self-assigned this Aug 29, 2018

jacekbogdanski force-pushed the t/1722 branch from da9f76c to 8b05322 Compare August 29, 2018 10:52

jacekbogdanski changed the base branch from major to master August 29, 2018 10:54

jacekbogdanski removed their assignment Aug 29, 2018

Comandeer self-assigned this Sep 18, 2018

Comandeer requested changes Sep 19, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Sep 19, 2018

jacekbogdanski self-assigned this Sep 19, 2018

jacekbogdanski force-pushed the t/1722 branch from 8b05322 to b4755f7 Compare September 19, 2018 11:33

jacekbogdanski requested a review from Comandeer September 19, 2018 11:34

CKEditorBot unassigned jacekbogdanski Sep 19, 2018

Comandeer self-assigned this Oct 8, 2018

jacekbogdanski and others added 6 commits October 8, 2018 12:32

Added filter owning editor option.

f7e709d

Binded editor destroy with plugin filters.

72f1e87

Added tests.

5caa79b

Changelog entry.

b0c0431

Added and updated unit tests.

314f23c

Updated docs.

94213c6

jacekbogdanski and others added 8 commits October 8, 2018 12:37

Updated tests.

e99654a

Changed CKEDITOR.filter signature.

5b84602

Handed filter destroy for widget editables.

bfa71c5

Added tests and fixed filter deletion.

96c7d3b

Updated filter docs param.

5342902

Refactored editor reference.

1546d91

Updated filter function to support IE8.

0c42d3b

Updated version tags.

013af36

Comandeer force-pushed the t/1722 branch from b4755f7 to 013af36 Compare October 8, 2018 10:46

Comandeer changed the base branch from master to major October 8, 2018 10:47

Comandeer requested changes Oct 8, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Oct 8, 2018

jacekbogdanski self-assigned this Oct 9, 2018

jacekbogdanski added 4 commits October 9, 2018 11:41

Updated docs.

50681f0

Changelog entry.

c5f2244

Changelog entry rewrite.

87d5473

Updated version tags and simple refactoring.

7c837b7

jacekbogdanski requested a review from Comandeer October 9, 2018 10:09

CKEditorBot unassigned jacekbogdanski Oct 9, 2018

Comandeer self-assigned this Oct 9, 2018

Simplify entry in API Changes section.

e42fe2d

Comandeer approved these changes Oct 9, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Oct 9, 2018

Comandeer merged commit 1115dbb into major Oct 9, 2018

CKEditorBot deleted the t/1722 branch October 9, 2018 16:00

mixonic added a commit to Addepar/ckeditor-dev that referenced this pull request May 20, 2019

Clean up further memory leaks around filters

4d9d9f4

Implements most of ckeditor#1770

mixonic mentioned this pull request May 20, 2019

Clean up further memory leaks around filters Addepar/ckeditor-dev#15

Closed

RubyGeorge 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

Removed memory leak caused by CKEDITOR.filter.instances #1770

Removed memory leak caused by CKEDITOR.filter.instances #1770

jacekbogdanski commented Mar 9, 2018 •

edited

Loading

Comandeer left a comment

Comandeer Apr 20, 2018

Comandeer Apr 20, 2018

Comandeer Apr 20, 2018

jacekbogdanski Apr 26, 2018

Comandeer Apr 26, 2018

jacekbogdanski Apr 27, 2018

Comandeer Apr 20, 2018

jacekbogdanski Apr 26, 2018 •

edited

Loading

jacekbogdanski commented Apr 27, 2018

jacekbogdanski commented May 7, 2018

Comandeer commented May 11, 2018

Comandeer left a comment

Comandeer Sep 19, 2018

Comandeer Sep 19, 2018

Comandeer Sep 19, 2018

Comandeer Sep 19, 2018

Comandeer Sep 19, 2018

Comandeer Sep 19, 2018

Comandeer commented Oct 8, 2018

Comandeer left a comment

Comandeer Oct 8, 2018

Comandeer Oct 8, 2018

Comandeer Oct 8, 2018

Comandeer Oct 8, 2018

jacekbogdanski Oct 9, 2018

Comandeer left a comment

Removed memory leak caused by CKEDITOR.filter.instances #1770

Removed memory leak caused by CKEDITOR.filter.instances #1770

Conversation

jacekbogdanski commented Mar 9, 2018 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

jacekbogdanski commented Apr 27, 2018

jacekbogdanski commented May 7, 2018

Comandeer commented May 11, 2018

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented Oct 8, 2018

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Mar 9, 2018 •

edited

Loading

jacekbogdanski Apr 26, 2018 •

edited

Loading