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

engineering-this · 2019-05-23T11:36:27Z

What is the purpose of this pull request?

A bit of fix a bit of feature

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

Proposed changelog entry

Fixed Issues:

* [#3115](https://github.com/ckeditor/ckeditor-dev/issues/3115): Fixed: Destroying editor during initialization throws an error.

API Changes:

* [#3124](https://github.com/ckeditor/ckeditor-dev/issues/3124): Added the [`CKEDITOR.dom.element#isDetached`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_dom_element.html#method-isDetached) method.

What changes did you make?

Fix

Within various places in code I've added checks if editor is destroyed or detached then execution of following code is prevented. When possible editor.container is used to check, otherwise editor.element.

Edge && IE

MS browsers sometimes throws Permission Denied, however browser won't break on such error, so it's nearly impossible to fix it. I've spend some time checking timeouts and onload listeners with no success.
Note: This PR shouldn't break anything for Edge or IE, because when editor is detached than it will throws errors regardless of having this fix or not.

Other stuff

https://github.com/ckeditor/ckeditor-dev/compare/t/3115?expand=1#diff-162a1b36d2efd62081522822a026e853R195
☝️ I think about removing above message, as now editor can be destroyed without any issues.
this PR introduces new method CKEDITOR.dom.element.isDetached (element.isDetached check if element is detached from DOM #3124)

Closes #718.
Closes #2257.
Closes #3115.
Closes #3124.

jacekbogdanski · 2019-06-05T09:03:33Z

@alinaciuysal the issue has been added to the 4.12.0 milestone (see #3115) so if everything goes smooth it will be available since the 4.12.0 release. Bear in mind that the time may be delayed if we will struggle with some unexpected problems, but hopefully we would be able to close it with 4.12.0.

Please, subscribe #3115 for updates.

jacekbogdanski

Rebased to the latest major.

I'm missing a lot of test coverage which makes reviewing hard as it's a bit unknown why some changes have been introduced.

Also, you refactored a lot of tests introducing editors.element stub object like:

		editor.element = {
			isDetached: function() {},
			getAttribute: function() {}
		};

It hides a real issue where according to our documentation, editor's element is optional:
https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_editor.html#method-constructor
but your changes makes it obligatory.

CHANGES.md

jacekbogdanski · 2019-06-13T08:52:15Z

tests/core/creators/inline-textarea.js

@@ -109,6 +109,27 @@
 					assert.isNull( editor.container.getCustomData( 'x' ), 'Custom data purged' );
 				} );
 			} );
+		},
+
+		'test destroy when editor.container is absent': function() {


Missing tag reference. Please, update the rest of the tests also.

Also, there are some console errors for this test file which didn't occur before your changes:

☝️ I have same errors on major branch. If you check on createConcurrentEditorTest helper you will see that it is intentional to test this warning.

jacekbogdanski · 2019-06-13T08:57:26Z

core/creators/themedui.js

@@ -239,6 +239,9 @@ CKEDITOR.replaceClass = 'ckeditor';

 			// Delay to avoid race conditions (setMode inside setMode).
 			setTimeout( function() {
+				if ( editor.status === 'destroyed' || editor.container.isDetached() ) {


You are using this condition multiple times, so it seems like a good candidate for method extraction.

Missing unit tests.

Actually this case is supposed to be covered by
test editor set mode when editor is detached'
and
test editor set mode when editor is destroyed'

But these tests are false positive, because they use fictional mode :sadfrog:.

jacekbogdanski · 2019-06-13T09:37:04Z

core/editor.js

@@ -189,7 +189,7 @@

 		// Return the editor instance immediately to enable early stage event registrations.
 		CKEDITOR.tools.setTimeout( function() {
-			if ( this.status !== 'destroyed' ) {
+			if ( this.status !== 'destroyed' && !this.element.isDetached() ) {


This change doesn't seem to be covered by unit tests.

jacekbogdanski · 2019-06-13T09:40:18Z

core/editor.js

+							return;
+						}
+
+						if ( editor.container && editor.container.isDetached() ) {


No unit test coverage for this condition.

jacekbogdanski · 2019-06-13T09:54:09Z

plugins/clipboard/plugin.js

@@ -1203,8 +1203,10 @@
 		}

 		function setToolbarStates() {
-			if ( editor.mode != 'wysiwyg' )
+			if ( editor.mode != 'wysiwyg' || editor.status === 'destroyed' || editor.container.isDetached() ) {


No test coverage. Also, why this change has been introduced?

It produced an error for me, but I see after other changes it doesn't occur anymore, so probably now it can be reverted.

jacekbogdanski · 2019-06-13T09:57:16Z

plugins/wysiwygarea/plugin.js

@@ -89,6 +89,11 @@

 				function onLoad( evt ) {
 					evt && evt.removeListener();
+
+					if ( editor.status === 'destroyed' || editor.container.isDetached() ) {


No tests coverage.

plugins/wysiwygarea/plugin.js

jacekbogdanski · 2019-06-13T10:15:06Z

plugins/wysiwygarea/plugin.js

-					iframe.remove();
+					if ( iframe.getParent() ) {
+						iframe.remove();
+					}
 				} else {
 					CKEDITOR.warn( 'editor-destroy-iframe' );


This warning probably doesn't seem sense anymore after changes.

jacekbogdanski · 2019-06-13T10:19:38Z

tests/core/creators/inline-textarea.js

+				var editor = bot.editor,
+					container = editor.container;
+
+				delete editor.container;


I'm missing verification when the container is alive but in detached mode.

engineering-this · 2019-06-13T10:54:30Z

The general problem with unit test is that editor has to be detached at the right moment, so that given change in code is executed, if it's too early than it might be not reached. With that amount of async code it's quite tricky. I'll try to add some more cases, but I can't guarantee to cover them all.

engineering-this · 2019-06-14T10:59:45Z

With fine amount of dirty hack replacing methods and stealing provided arguments I managed to cover all the cases with tests.

jacekbogdanski

Failing test at /core/editor/destroy.

I can see that there is some memory increase when destroying editors between snapshots:

Also, allocation timeline shows some increase, around 150-200 KB for each create/destroy pair. However, I'm getting similar results with the editor without your changes (just skipping issue with container removed before destroy), so it seems more like the general issue than introduced by your changes. Nevertheless, I'm wondering if this test could be simplified. Memory leaks testing requires a bit of patience and depends highly on the browser dev tools, i.e. in the manual test you assumed that the tester uses Chrome, but it's available also for Safari and Edge. Tools also often change.

Also, there is no IE test coverage (and Edge is covered partially). I know that the tests may be hard to write for this case, but we shouldn't leave core change untested correctly.

If it won't be possible to add unit tests for these changes, we should have at least some really well written manual tests.

core/creators/inline.js

jacekbogdanski · 2019-06-17T08:20:03Z

core/editor.js

+		 * @private
+		 * @return {boolean}
+		 */
+		_shouldPreventInit: function() {


The method name doesn't tell much. Maybe something like isDestroyed? Wondering if we can assume that missing/detached editor's container means that it's destroyed 🤔

Also, as the function is used among multiple modules, it should be rather a public API.

isDestroyed would be very misleading. Even when editor is detached it still should be destroyed for proper cleanup, such name would suggest that if editor isDestroyed you might not need to call editor.destroy anymore.

Sometimes I have this dilemma which name is better:

what it does

what's the purpose

In this case second option feels better for me. Eventually condition might be updated, but with such a name it wouldn't be a problem. If we went for first option it'd be named like isDestroyedOrDetached, and in that case updating condition might be not possible without changing a name. Unless we want to have outdated name which doesn't tell what it does any more.

jacekbogdanski · 2019-06-17T08:22:36Z

core/editor.js

+				return true;
+			}
+
+			return this.status === 'destroyed';


Maybe simplify a bit into oneliner?

return this.status === 'destroyed' || this.container && this.container.isDetached()

jacekbogdanski · 2019-06-17T08:43:38Z

plugins/wysiwygarea/plugin.js

-					iframe.remove();
-				} else {
-					CKEDITOR.warn( 'editor-destroy-iframe' );
+					if ( iframe.getParent() ) {


Can't be used isDetached method here?

jacekbogdanski · 2019-06-17T09:27:29Z

tests/core/skin/skin.js

@@ -55,6 +55,12 @@ bender.test( {
 		var skin = CKEDITOR.skin;

 		var editor = new CKEDITOR.editor();
+
+		editor.element = {


Seems like you already fixed the issue with missing editor element. If so, do we still require this dirty stub among unit tests?

jacekbogdanski · 2019-06-17T11:07:56Z

I see that I incorrectly tested browser against memory leaks, and the leaks are much smaller than I thought:

It's also starting to be stable at some point so I would say it's probably fine enough to leave this issue for now.

engineering-this · 2019-06-26T12:21:00Z

i.e. in the manual test you assumed that the tester uses Chrome, but it's available also for Safari and Edge.

When initially working on memory leaks #2969 we decided to test only Chrome. We reduced amount of leaks there, so we can compare if given branch produces any new leaks or not. For other browsers we didn't test or fix leaks, doing so is out of scope of this ticket.

Other browser have memory profiling tools they aren't nearly as good as Chromes. Even if they allow to see how much memory is leaking they aren't really helpful to find out what is causing leaks.

New changes

Enabled unit tests for IE and Edge.
Added test cases for all editor types.
Reworked manual test, so that it is more precise at destroying editor where it caused issues.
Fixed permission denied thrown by Edge. It looks like an upstream issue, as it randomly throws when trying to access editable.$ of detached editor. It is fixed by wrapping some parts of code with try/catch, also caching cke-data-expando for proper custom data cleanup.

jacekbogdanski · 2019-07-09T12:49:09Z

Rebased into the latest major.

jacekbogdanski

Implementation starts to be a bit complicated.

please, update version tags into 4.13.0
some tests are failing, see CI
manual test gives no feedback when clicking create editor button. Looks like the test is broken
the only manual test is still written for Chrome (however, not ignored for other browsers this time). The most visible issue reproduction is error thrown in dev console. Can't it determine the test result for other browsers without more advanced dev tools?

I'm not sure about your latest changes. Does the whole expandoNumber workaround is created only for upstream Edge issue? If so, isn't better to report an upstream issue than creating workarounds? Silencing errors isn't the prettiest way to hide the issue. Especially, that at some point MS may decide to fix the issue (when moving to chromium ) and we will end up with dead code. Are you able to verify which accessor throws an error?

Of course, if the editor blows because of this issue, a workaround may make sense.

jacekbogdanski · 2019-07-09T12:56:51Z

core/editable.js

-				this.removeClass( 'cke_editable' );
+				catchEdgePermissionDenied( function() {
+					this.removeClass( 'cke_editable' );
+				}.bind( this ) );


Use CKEDITOR.tools.bind instead.

jacekbogdanski · 2019-07-12T10:42:59Z

core/editable.js

@@ -1264,6 +1270,17 @@
 		}
 	} );

+	function catchEdgePermissionDenied( callback ) {


Using a try/catch clause for an upstream issue is the last resort. Does this issue make editor unusable/unstable? It seems a more obvious choice to report upstream issue than providing such workaround.

Replied here: #3128 (comment)

jacekbogdanski · 2019-07-12T10:45:31Z

core/editable.js

+		try {
+			callback();
+		} catch ( error ) {
+			if ( !CKEDITOR.env.edge || error.number !== -2146828218 ) {


I bet error code may change during Edge update. Do you think it's safe to use it?

I think that it's possible but unlikely that error code will change. Yet it is safe, worst thing that could happen is that same error with different code will be thrown. However without using this check we might silent different error.

Previously we did it this way:
https://github.com/ckeditor/ckeditor-dev/blob/c4d250ea5fa0da09781a3ee84c99ca03f09c144b/core/editable.js#L100-L104

But IMO it's not optimal, as it might silent other errors. If error code changes tho, we might repeat logic from above.

jacekbogdanski · 2019-07-12T10:47:57Z

core/editor.js

+		 * @return {boolean}
+		 */
+		shouldPreventInitialization: function() {
+			return this.status === 'destroyed' || this.container && this.container.isDetached();


It still says nothing about function purpose. You need to read implementation to understand what's going on. Even isDetachedOrDestroyed seems like a better name.

The purpose is to tell when editor initialization should be prevented, so this name says everything you need to know. Condition under we prevent initialization might change in the future, so binding function name to the implementation doesn't feel right to me.

This function has too low-level implementation for such high abstraction name. But let's leave it for now and back later during final refactoring.

There is a big mismatch between API docs for this method and its name. isDetachedOrDestroyed seems a better choice.

I agree, especially that docs says the same:

Returns boolean whether editor is destroyed or detached.

👍

I've come with an idea to replace this method with simple isDetached() and implement or statement in places where we actually checking if editor is detached or destroyed.
This removes this additional abstraction here and remains much cleaner API.

jacekbogdanski · 2019-07-12T11:00:26Z

tests/core/editor/manual/detaching.html

+			<li><input type="radio" name="destroy" value="1"><label>async with 0 timeout</label></li>
+			<li><input type="radio" name="destroy" value="2"><label>before 'loaded' event</label></li>
+			<li><input type="radio" name="destroy" value="3"><label>before 'scriptLoader.load' callback called</label></li>
+			<li><input type="radio" name="destroy" value="4"><label>before iframe#onload</label></li>


It looks like this button is disabled all the time despite using different options. Couldn't spot a way to enable it.

It is enabled only when browser is Firefox or IE (not Edge), and editor type is classic. Reason for that is only in this case iframe#onload is used.

https://github.com/ckeditor/ckeditor-dev/blob/9cb74b35db7082189088f47326eb05b02c947f8d/plugins/wysiwygarea/plugin.js#L49-L53

Do you see it readable in a current shape? It looks like there is something broken when testing not listed browsers, not like it has been disabled on purpose. I see 2 options:

enable it despite browser sniffing, so it will prevent against regressions as browser sniffing should be treated as an internal mechanism

write some note at this radio so it's known that it should only work for FF and IE

I'm rather for unifying test case among browsers (enabling it).

I'll add an message in test. This condition is the same as one deciding if iframe#onload should be used. So it doesn't matter if browser sniffing is working or not. This case is enabled only in environment which uses this listener for creating editor. Also this case could break test in other browsers, so I'm for having it disabled when it shouldn't be tested.

https://github.com/ckeditor/ckeditor-dev/blob/334fef5a0da86a853b788a0d1e8201eb18624ec2/plugins/wysiwygarea/plugin.js#L51-L51

engineering-this · 2019-07-23T09:12:53Z

The problem with Edge permission denied is that it's occurring randomly, I couldn't reproduce it outside of CKEditor. I doubt Edges team would confirm issue without easy reproduction steps.

By fixing it with try/catch on our side, we are not just silenting it, but also allowing execution of code that otherwise wouldn't be reached because of error. This allows to finish the cleanup, otherwise some data of removed elements won't be ever released. What's more that error is very visible in our framework integrations.

Especially, that at some point MS may decide to fix the issue (when moving to chromium ) and we will end up with dead code.

When Chromium based MS Edge will be released we will have to investigate carefully how whole editor works on it. We have so many conditional code for Edge, that it all should be revised. I don't think that we should worry as much about how browsers might work in the future, instead we should focus on providing Editor that works right now. Even if it means sometimes we need to fix browser issue on our side.

the only manual test is still written for Chrome (however, not ignored for other browsers this time). The most visible issue reproduction is error thrown in dev console. Can't it determine the test result for other browsers without more advanced dev tools?

Test is written for all of the browsers. It has EXTRA steps for Chrome to make sure no memory leaks are introduced with changes. When initially working on memory leaks we decided to work only with Chrome. There is no reason to put our effort to test memory leaks for every browser now if we didn't do it in dedicated ticket.

…Data.

…variations. Improve checks in algorithm.

f1ames · 2019-09-17T12:08:47Z

Taking over this review from @Comandeer. Rebased onto latest major.

f1ames

2 unit tests fails on Safari:

msamsel · 2019-09-17T14:13:01Z

I extracted those failing tests to separate issue #3426.
I tried to rewrite the original prototype of the method to attach the callback before the editor is created. The error still occurs in such a case.

f1ames

LGTM 👍 🎉

Good job @engineering-this, @jacekbogdanski, @Comandeer and @msamsel 😄

Unit tests

Chrome (CI) ✅
Firefox (CI) ✅
Safari ✅
IE8 ✅
IE11 ✅
Edge ✅

Taken over review of this PR.

f1ames mentioned this pull request May 30, 2019

Enable classic editor ckeditor/ckeditor4-angular#6

Closed

jacekbogdanski assigned jacekbogdanski and unassigned jacekbogdanski May 31, 2019

engineering-this self-assigned this Jun 5, 2019

engineering-this removed their assignment Jun 5, 2019

jacekbogdanski self-requested a review June 12, 2019 15:24

jacekbogdanski requested changes Jun 13, 2019

View reviewed changes

jacekbogdanski force-pushed the t/3115 branch from 308a6fe to cc94e03 Compare June 13, 2019 10:42

engineering-this self-assigned this Jun 13, 2019

engineering-this force-pushed the t/3115 branch from 53072d4 to a0441d4 Compare June 14, 2019 10:56

engineering-this requested a review from jacekbogdanski June 14, 2019 10:59

CKEditorBot unassigned engineering-this Jun 14, 2019

jacekbogdanski self-assigned this Jun 17, 2019

jacekbogdanski requested changes Jun 17, 2019

View reviewed changes

engineering-this assigned engineering-this and unassigned jacekbogdanski Jun 17, 2019

engineering-this requested a review from jacekbogdanski June 26, 2019 12:21

CKEditorBot unassigned engineering-this Jun 26, 2019

jacekbogdanski self-assigned this Jul 9, 2019

jacekbogdanski force-pushed the t/3115 branch from 7e689fc to 0f127d8 Compare July 9, 2019 12:48

jacekbogdanski requested changes Jul 12, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski Jul 12, 2019

engineering-this self-assigned this Jul 22, 2019

Mateusz Samsel added 8 commits September 17, 2019 14:08

Add isDestroyed method and unit test. Correct typo in docs.

d8e5f0c

Replace direct status check with isDestroyed method.

b1419e3

Remove reduntant check and add the issue reference in code comments.

0ee9df7

Refactor code checking a detached state.

87b2912

Mark getData as internal. Replace numbers with object confign for set…

ec395cc

…Data.

Add unit test for checking detached document element and some of its …

472b5b6

…variations. Improve checks in algorithm.

Revert getData. Using internal data breaks tests.

5363c6b

Correct docs typo.

a68a5d9

f1ames force-pushed the t/3115 branch from 36c14a5 to a68a5d9 Compare September 17, 2019 12:08

f1ames added 2 commits September 17, 2019 14:19

Tests: Typo fixed.

50f0471

Tests: Refactoring - removed code duplication from test utils.

35bb480

f1ames suggested changes Sep 17, 2019

View reviewed changes

msamsel mentioned this pull request Sep 17, 2019

Find a way to destroy editor when iframe is loaded on Safari #3426

Closed

Extract failing test on Safari to spearate issue.

a41ad5a

f1ames self-assigned this Sep 17, 2019

f1ames self-requested a review September 17, 2019 15:42

f1ames approved these changes Sep 17, 2019

View reviewed changes

CKEditorBot unassigned f1ames Sep 17, 2019

Changelog entry.

f95d2f9

f1ames merged commit 0f47333 into major Sep 17, 2019

CKEditorBot deleted the t/3115 branch September 17, 2019 15:47

trevdor mentioned this pull request Sep 17, 2019

Fix for: [Edge]: opening dialogs results in error permission denied #3030

Closed

jacekbogdanski mentioned this pull request Jan 16, 2020

Vue CKEditor 4 Component implementation ckeditor/ckeditor4-vue#1

Merged

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

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

Conversation

engineering-this commented May 23, 2019 • edited by Comandeer Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Proposed changelog entry

What changes did you make?

Fix

Edge && IE

Other stuff

jacekbogdanski commented Jun 5, 2019

jacekbogdanski left a comment • edited Loading

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

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

engineering-this commented Jun 13, 2019

engineering-this commented Jun 14, 2019

jacekbogdanski 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

jacekbogdanski commented Jun 17, 2019 • edited Loading

engineering-this commented Jun 26, 2019

New changes

jacekbogdanski commented Jul 9, 2019

jacekbogdanski 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

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

engineering-this commented Jul 23, 2019

f1ames commented Sep 17, 2019

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Sep 17, 2019

f1ames left a comment

Choose a reason for hiding this comment

Unit tests

engineering-this commented May 23, 2019 •

edited by Comandeer

Loading

jacekbogdanski left a comment •

edited

Loading

jacekbogdanski commented Jun 17, 2019 •

edited

Loading