Verify destruction chain

[Reinmar]

Moved from ckeditor/ckeditor5-core#186

[Reinmar]

Another thing I noticed is that models are not unbound from each other.

[Reinmar]

A possible blocker: https://github.com/ckeditor/ckeditor5-ui/issues/83.

[pjasiun]

ckeditor/ckeditor5-core#186 is invalid link now. I will miss your essay.

[Reinmar]

There was no essay. I've been writing down places in the code we need to take into consideration :/

[Reinmar]

There's a big chance that we'll have to work on destroying the engine's view correctly now, because it's causing many issues once we have all tests run in a single scope. The observers are not disabled so there are hard to track issues with relations between tests. I'm not tracking such things in ckeditor/ckeditor5-engine#671.

[Reinmar]

BTW. FYI – DomEmmiterMixin will need to be moved to utils, cause the observers should use it. Otherwise, we're not able to remove listeners easily.

[scofalik]

I did some work on this subject and it's done in PR #671 however I am not sure whether it completes this issue because I only focused on view observers.

[Reinmar]

When reviewing ckeditor/ckeditor5-core#87 I realised that UI's destruction doesn't work in real life. We implemented it too early, without considering what needs to be done and, as a result of that, it does too much. This leads to conflicts when the editor and plugins try to destroy the pieces of UI introduced by them.

But I also realised that it's not only unclear what needs to be done but also how the process should look in general.

Goals

So, first of all, goals:

• The minimal goal is to revert all changes done by the editor to the app/website. Notice that we're talking here only about things which affect the editor surrounding. So, if a listener was added to window#scroll event, it needs to be removed. If the editor was created based on an existing element (so that that element became editor's editable root), all listeners, attributes, whatever needs to be cleared. If editor injected some elements into the page, they need to be removed. Etc.

  We're not talking here about listeners between editor components because both of those things will be soon gone so they will not retain memory and they won't be triggered by anything.

• However, the editor has many dynamic parts which one should be able to create and destroy on runtime (while one editor instance works). This is what CKEditor 4 does not support so if you create any component it just stays with you until the editor is destroyed.

  So which editor parts are dynamic? Of course, the model/view entities from the engine, but that's kind of obvious – you create and remove them all the time. Then, UI is definitely a strong candidate here. But, while a bit theoretical right now, we tend to implement controllers (such as plugins or engine's controllers) in a dynamic way too. However, the real focus on plugin destruction is that it works flawlessly during editor destruction – unloading plugin is just a side effect of that.

To sum up – destruction should be about detaching components from the world (their surrounding). Additional work, like reverting DOM changes may also be needed in a limited number of cases (such as removing outermost editor UI elements). Removing references between objects is not needed! If you destroy a component, you (the world) remove a reference to that component, so there's no need for that component to remove references to its children.

Process

Theoretically, it's simple – if you initialise some components in A, B, C order, then they need to be destroyed in C, B, A order.

So, e.g. if the editor first loads plugins and then inits the UI, the UI needs to be destroyed before the plugins are destroyed.

However, what if one of the plugins created some UI and added it to editor UI's collection? On editor destroy, first the UI will be destroyed (which is recursive and will destroy that plugin's component too) and then the plugins will be destroyed and one of these plugins may want to somehow destroy the component which it created (it might've even initialised it, but usually plugins don't do that).

We've got a collision of responsibilities here. editor.ui.destroy() is recursive so it destroys things which the editor creator didn't create. And then plugins come and try to destroy those things again.

I can see two options here:

• Either we strictly define that only a component which created some other component should destroy it (also, if it added it to some collection, only it should remove it?). While well organised, I feel that this will be inconvenient. We'd need to remove recursive destruction of UI. We'd need to figure out how components created by the component factory are destroyed (probably by their creator, so e.g. the toolbar). Finally, controllers will need to store all components they created so they are able to destroy them later on.

  BTW, this approach would be the way to go if we would like to really allow unloading plugins. Since the UI destruction won't be recursive, a plugin will need to destroy everything it created.

• We go the easy way and just make sure that a component can be destroyed a couple of times (which should be easy to achieve if we'll stop dereferencing stuff on destroy). This will ensure that on UI destroy all reachable UI components will be destroyed, and then, upon plugins destruction, if any of them would like to destroy its own component, it will be able to do so (it won't have any effect).

The second option is definitely a bit messy. We basically give up and avoid the problem by making destruction more forgiving.

The first approach is cleaner but there are technical problems that we'd need to tackle. One of them is that UI initialisation is recursive (so it's symmetrical to destruction). If we'll make destruction non-recursive it means that initialisation needs to become non-recursive as well. I'm not sure what implications it brings.

The second of them is that in at least two places (plugins, UI) we divided initialisation of a class into two parts – constructor and init() (so the latter can be async). In both cases it may happen that between the construction and initialisation of some component, some other code was executed. However, destruction is just a single action so an initialisation process like: A1 (constructor), B, A2 (init) cannot be reverted to A2, B, A1. It will be either B, A2, A1 or A2, A1, B. I can't tell now but it might cause some issues.

[Reinmar]

PS. I think that we'll need to touch a bit of code here, especially in the UI lib. We also plan there to work on https://github.com/ckeditor/ckeditor5-ui/issues/225 and there's a high chance that it will make sense to work on both topics at the same time. Trying to do them separately may be a waste of time.

Fortunately, AFAIK the destruction process is not a blocker, so we can move it to after alpha.

[Reinmar]

I have a very similar case in the CommandCollection. Should CommandCollection#destroy() destroy all the commands?

If it won't do that – plugins which introduced them will need to do that manually.

If it will – you won't be able to unload a plugin or you might destroy a command twice (if a plugin would destroy it too).

The first solution is really inconvenient. Just like with the UI, there's a high chance of bugs caused by plugins which forgot to destroy components which they created.

So, since CommandCollection is yet another example where it'd be convenient if the collection destroyed its children, I'm leaning towards this solution. For now, we can save time and do not have plugins trying to destroy or remove their children. But that doesn't block us from writing plugins which clean up after themselves in the future.

[Reinmar]

We discussed this further internally. We made the decision to go the easy way.

1. On init plugins add components to various collections in the editor – UI, commands and maybe more.
2. On destroy, it's the editor responsibility to destroy items in these collections. Destroying means removing interaction between components, not dereferencing properties or removing subcomponents from parent's collections. Features should also take care of reverting other changes that they made to the environment (the DOM outside of the editor DOM).
3. If we'll ever need to destroy a single plugin fully, we'll review its destruction() method to cover more cases than normally – i.e. removing and destroying all its child components.
4. But in order for 4. to be ever doable and to avoid issues like the one which we had in T/86: Plugin#destroy() should remove listeners ckeditor5-core#87 we need to think about destroy() as about an action which can be triggered more than once. Since, in most cases, we're just calling stopListening() it's a no brainer but in some, we'll need to think a bit. However, this is not a major requirement at this moment so we should not spend time on testing this.

[oleq]

I read all your comments, @Reinmar and there are things which seem a little bit confusing to me:

1. This leads to conflicts when the editor and plugins try to destroy the pieces of UI introduced by them.

   Correct me if I'm wrong, but we've never considered such possibility that a plugin contributes to the destruction process in the UI. UI has always been designed in such way that it is destroyed recursively from editor.ui.view.destroy() up to the farthest node in the UI tree.
   If inside of any plugin (or any kind of controller), there's a code which does anything with the destruction of the UI, most certainly it's a bug. Any UI component belonging to the application must be registered in one of the collections in the tree (one way or another, either by the Template or manually by the developer) and it's totally up to the collections to destroy children recursively.

2. Does it simply boil down to "Allow double destruction of the UI tree/create a counter–flag to View#ready to prevent double destruction"? It feels so. But even if we allow it, I'm still worried about the quote in 1. which clearly indicates there are bugs in the code and we're simply masking them.

[Reinmar]

Correct me if I'm wrong, but we've never considered such possibility that a plugin contributes to the destruction process in the UI. UI has always been designed in such way that it is destroyed recursively from editor.ui.view.destroy() up to the farthest node in the UI tree.
If inside of any plugin (or any kind of controller), there's a code which does anything with the destruction of the UI, most certainly it's a bug. Any UI component belonging to the application must be registered in one of the collections in the tree (one way or another, either by the Template or manually by the developer) and it's totally up to the collections to destroy children recursively.

You're right – we never considered it (openly) and this is a bug – ContextualBalloon should not destroy anything on its own. This needs to be fixed and I reported that in https://github.com/ckeditor/ckeditor5-ui/issues/248#issuecomment-307731282.

However, it doesn't mean that we will never think about destroying single plugins and thinking how they can destroy the UI they introduced. This is theoretical but worth keeping in the back of our heads.

Does it simply boil down to "Allow double destruction of the UI tree/create a counter–flag to View#ready to prevent double destruction"? It feels so.

There should be no need for such a flag at the moment. At least, I don't see any. The biggest issue right now is that UI framework has too aggressive destroy() methods.

Let's fix both issues that I described in https://github.com/ckeditor/ckeditor5-ui/issues/248#issuecomment-307731282 and see where it gets us.

[Reinmar]

We performed a quick tests with @oleq and it seems that there are no significant issues. The only ones I could see were related to retaining some deopt data and minor stuff – nothing big.

[Reinmar]

I think that we can close this ticket. New, more detailed ones should be opened for issues that we'll find (and if we'll find them :P).