Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak after creating and destroying the editor #5954

Closed
jodator opened this issue Dec 10, 2019 · 23 comments
Closed

Memory leak after creating and destroying the editor #5954

jodator opened this issue Dec 10, 2019 · 23 comments
Labels
resolution:resolved This issue was already resolved (e.g. by another ticket). squad:core Issue to be handled by the Core team. support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@jodator
Copy link
Contributor

jodator commented Dec 10, 2019

πŸ“ Provide detailed reproduction steps (if any)

  1. Create / destroy editor

βœ”οΈ Expected result

No editor in the memory.

❌ Actual result

There is an editor left in the memory

πŸ“ƒ Other details

Performance time line:

Selection_089

There's ClassicEditor in the memory:
Selection_091

And some detached event listeners:
Selection_090


If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

@jodator jodator added type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases. labels Dec 10, 2019
@Reinmar Reinmar added this to the next milestone Dec 16, 2019
@andycrulez
Copy link

πŸ‘
this one is quite crucial to me. I'm using the CKEditor5 with Angular on the production (inline editor) with multiple instances on multiple pages. I can easy reach 2GB of memory :)

@martijnpieters
Copy link

Same problem over here. Next week we're going to try to make a dynamic Multi-root Editor, to see if we get an acceptable performance.

@martijnpieters
Copy link

Been there, done that. Under 75MB for a tab with tens of editable fields.

@QuentinLeGoff
Copy link

QuentinLeGoff commented Mar 18, 2020

Hello,
I'm facing the same problem with DecoupledEditor and collaboration feature.

Without collaboration feature, GC seems to remove instances but when I use the RealTimeCollaborativeEditing, objects accumulates on JS VM.

And each time I load new document (destroy then create to load new document in CKEditor Cloud Services), it take more time than before to create editor.

@jodator
Copy link
Contributor Author

jodator commented May 20, 2020

I did a quick scan today of the state of things and it looks like some of the memory leaks sources were removed from the time I've reported that.

As for the integrations I didn't check but I would advise anyone to check if memory leaks comes from the editor itself or not. Any non-destroyed listener which touches editor instance could prevent freeing memory.

edit: Also logging editor to the console and not clearing the console will also hold that editor instance in memory after the editor was destroyed.

@meddiecap
Copy link

I don't know if I need to drop this here, but I used the (new?) builder at https://ckeditor.com/ckeditor-5/online-builder/
If it's of any use: "Build id: 32ppao8vorw3-2dtu63mfs6mp"
(34 plugins selected)

I've attached a super simple html page that managed to go to 40mb in JS memory on the first load and each page refresh easily adds 20mb, and it never goes down.

testing.txt

Is there any way in JS to manually empty the memory or something like that?

@jodator
Copy link
Contributor Author

jodator commented Nov 3, 2020

Hi @meddiecap! How did you test the memory consumption?Β 

I did a quick test with a similar setup and the results for page refreshing are as follow:

  1. The "JavaScript VM Instance" memory utilization goes up and down, randomly. In my case in the 50 - 250 MB range.
  2. The "Heap Snapshot" (used memory per page/app) is in the 57 - 63 MB range.Β 

If the memory goes up/down on the page refresh it's up to the browser what it keeps in the memory and we can't control that. A full running editor will consume this much memory unfortunately.

So this is how Chrome utilizes memory on page refresh and we can't control that. A memory leak would occur if you'd

  1. Crate an editor.
  2. Use the editor.
  3. Destroy the editor (editor.destroy())

And the memory after step 3 would be the same or significantly bigger than the memory after step 2. You can observe this below:

@meddiecap
Copy link

Hey @jodator , thank you for your extensive explanation.

I test the memory consumption by simply refreshing the page.

Your first GIF shows me that you refresh the page a few times and then take a heap snapshot.
The results differ in that your snapshots seem to grow in a much slower pace than mine.
(Apologies for the low quality. What do you use for screen capturing?)

screen-capture

Your second GIF gives me the impression that it is best to destroy all instances of ckeditor when, and just before, a user wants to leave or reload the page. (Or when new content is loaded via AJAX which will replace the ckeditor with a new one, which I also intend to do.)

I tried just that and the snapshots do grow less, but still around 15mb per page reload:

    let allEditors = document.querySelectorAll('.ckeditor');
    let editorsLength = allEditors.length;

    window.editors = [];

    for (let i = 0; i < editorsLength; ++i) {
        ClassicEditor.create(allEditors[i], defaultConfig ).then(editor => {
            window.editors.push(editor);
        });
    }

    window.onbeforeunload = function() {

        for (let i = 0; i < window.editors.length; ++i) {
            window.editors[i].destroy();
        }
    };

@jodator
Copy link
Contributor Author

jodator commented Nov 3, 2020

window.editors = [];

this is preventing memory from being collected by the Garbage Collector during a page life cycle.

Your second GIF gives me the impression that it is best to destroy all instances of ckeditor when, and just before, a user wants to leave or reload the page.

The results differ in that your snapshots seem to grow in a much slower pace than mine.

If you refresh the web page you get a clean state (so the previous JavaScript objects are removed). There is no need, at least to my knowledge, to clean the editors if a user simply leaves a page. If you plan to reload the editor on AJAX calls then using editor.destroy() is a way to go. But remember to check if the editor instance is not held somewhere (like in window.editors[]).

The +20 MB on every page refresh is very strange. On quick research, I've found one bug in Chrome (which might be unrelated to your case) so it could be possible that some other code is still retaining objects.

However, the results you present in the GIF are curious - it looks like something might retain objects. The best way to verify memory leaks is to run the browser without any addons enabled:

google-chrome \
	--disable-extensions \
	--disable-plugins \
	--incognito

What do you use for screen capturing?)

You can compare snapshots between each other and inspect what happens there. But it might require some longer digging and staring into the Reatiners list :/ Β More on this here (and here πŸ˜‰)

I've used peek - it's Linux only tool.

@meddiecap
Copy link

I watched the youtube video, very interesting.

For now, when I leave the page, I loop over the editors and .destroy() them.
When I load new content using AJAX that needs to initialize an editor, I destroy the old ones first.

Although I still get the memory spikes I mentioned before, it also seems to be related to the DevTools. I don't know well how to read all the data but with every page refresh it looked like the snapshot tool stored and extra 8 "ck" object / classes.

Ckeditor was giving me console warning about missing toolbar stuff and fixing that also had some impact.

When I don't use DevTools, the memory will go back down to much more acceptable levels after around 10 seconds.
When I keep it open, the memory stays up.

I learned a lot today
Many thanks @jodator

@pomek pomek modified the milestones: next, nice-to-have Nov 10, 2020
@Mgsy Mgsy changed the title Memory leaks - they're back Memory leak after creating and destroying the editor multiple times Oct 13, 2021
@Mgsy Mgsy added the squad:core Issue to be handled by the Core team. label Oct 13, 2021
@Mgsy Mgsy changed the title Memory leak after creating and destroying the editor multiple times Memory leak after creating and destroying the editor Oct 13, 2021
@Mgsy
Copy link
Member

Mgsy commented Oct 13, 2021

It's possible to freeze the browser's tab after creating and destroying the editor multiple times (>100, it depends on the machine ).

To reproduce the freeze, you can use our all features manual test. Simply try to create and destroy the editor ~200x.

@Mgsy Mgsy added the support:1 An issue reported by a commercially licensed client. label Oct 13, 2021
@Reinmar
Copy link
Member

Reinmar commented Oct 13, 2021

I wanted to do a quick check and run into Google Dev toools crashing :O Anyone having similar issues now?

@Reinmar
Copy link
Member

Reinmar commented Oct 13, 2021

Apparently this is some very fresh bug because it doesn't seem to be reported yet. We need to wait now...

@Reinmar
Copy link
Member

Reinmar commented Oct 20, 2021

OK, dev tools are still working.

And I can confirm that there's some memory leak. Creating and destroying the editor, leaves 5MB of memory taken.

The first snapshot before the editor creation, second when the editor is created, 3rd after destroying it and removing the reference from window (I recorded this in manual tests where we expose the editor instance).

@oleq
Copy link
Member

oleq commented Oct 20, 2021

  1. We have a dedicated test http://localhost:8125/ckeditor5-editor-balloon/tests/manual/memory.html with some hints in the description.
  2. There are leaks
    image
  • I briefly checked and I saw some DOM elements and listeners retained after destruction.

@esezer
Copy link

esezer commented Oct 26, 2021

Hi. We are in touch with support team for a couple of weeks. They said they saw the leak 2 weeks ago. I leave a comment here.
This is an automated test url. And it generally freezes or stops working before 200 times. Tests done with 4-5 years of average computers.
https://merlon-test.firebaseapp.com/

I am looking forward to see the solution really

Thanks

@oleq
Copy link
Member

oleq commented Nov 4, 2021

Hi @esezer! Thanks for your input. We've been investigating this problem and it looks like the memory retention by the MathType plugin that you use is the source of your problems.Β 

MathType is not developed by the CKEditor team, though, so we cannot fix this right away. The issue has been brought to the attention of the company developing MathType.Β 

Can you run your test without the MathType and confirm the memory leaks are gone? Thanks!

@esezer
Copy link

esezer commented Nov 8, 2021

Hi @oleq! I created a new custom build editor without including MathType. But the problem is the same on my side.

Details:

  • I edit "src/ckeditor.js" under "packages/ckeditor5-build-classic"

  • Removed "import MathType from '@wiris/mathtype-ckeditor5';" and removed from builtinplugins and toolbar.

  • yarn run build

  • copy the files under "build" to my application.

As result I don't see any toolbar icon for MathType but nothing changed for me. Should I try a different method?

@oleq
Copy link
Member

oleq commented Nov 8, 2021

@esezer Can you share your sandbox online?

@esezer
Copy link

esezer commented Nov 8, 2021

@oleq I am sending you github url which I created CK team last month:
https://github.com/esezer/react-ckeditor5-test

This is the test url: https://merlon-test.firebaseapp.com/


I made tests all the day and found out an important memory leak on one of the CKEditor5 original plugins.

Wiris plugin makes a memory leak really but it is not that much than the leak I found today.

Tests show me that the most important leak is on "Table" plugins.

  • Table,
  • TableCaption,
  • TableCellProperties,
  • TableProperties,
  • TableToolbar,

I tried several times with and without those plugins above and results were dramatically different.

You can check out the repo on github to see my test project.

@Reinmar
Copy link
Member

Reinmar commented Nov 10, 2021

I can confirm it on this sample:

The spikes are new editors created. Pretty much none of them is cleared out.

But there's still MathType used somewhere there:

Please remove MathType and check then. The Table plugins may contribute to the size of the leak. But the most important question is – what causes the leak? From what we saw, it starts in MathType. Which means that it's the thing that needs to be fixed.

But of course we may be wrong, so it'd be awesome if you could modify this sample to remove MathType and check again.

@esezer
Copy link

esezer commented Nov 10, 2021

I had already tried sample with and without MathType.

But this time inside "ckeditor5-build-classic";

  • I upgraded CKEditor to v31,
  • deleted npm modules
  • removed @wiris/mathtype-ckeditor5 from package.json
  • npm install
  • npm run build

I copied the build folder into my test application and built it. Then deployed the test app to the same url. I also pushed the changes to my test repo.

When I start the test it still freezes and even sometimes crashes before finish. You can test the url and check the repo.

But if I also remove table plugins together with MathType, then it works much better.

That's why I thought it could be a leak because of table plugins. Because the only difference is that.

Thank you for your support.

@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@lslowikowska lslowikowska added support:3 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. labels Feb 23, 2022
@oleq
Copy link
Member

oleq commented Apr 11, 2022

Cross-positing the update from Wiris (the company behind MathType) https://github.com/wiris/html-integrations/issues/459#issuecomment-1094703678:

Hi @oleq,

We are happy to inform you that this issue has been fixed in the 7.28.0 version.

We are proceding to close the issue.

@Reinmar Reinmar closed this as completed Apr 13, 2022
@Reinmar Reinmar added the resolution:resolved This issue was already resolved (e.g. by another ticket). label Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution:resolved This issue was already resolved (e.g. by another ticket). squad:core Issue to be handled by the Core team. support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

No branches or pull requests