Memory leaks fix, part I: create/destroy

[jodator]

Suggested merge commit message (convention)

Fix: Fix memory leaks. Closes ckeditor/ckeditor5#1341.

---

Additional information

• This PR introduces a test util for testing memory leaks using mocha and requires changes from ckeditor5-dev repo: Add --expose-gc to Chrome's flags in Karma launcher configuration. ckeditor5-dev#473.
• Alternatively the test can be run on manually spawned Chrome instance and attaching to to a karma runner. To run Google Chrome with required flags execute in command line (other Chrome instances must be closed):

google-chrome \
    -js-flags="--expose-gc" \
    --enable-precise-memory-info \
    --disable-extensions \
    --disable-plugins  \
    --incognito \
    http://localhost:8125

• There's a bunch of repos touched, gathered under ckeditor5 branch:
  • editor-balloon: PR
  • editor-classic: PR
  • editor-decoupled: PR
  • editor-inline: PR
  • engine: PR
  • image: PR
  • link: PR
  • typing: PR (redundant)
  • ui: PR
  • utils: PR
  • widget: PR

WIP - test are failing.

[coveralls]

Coverage increased (+4.1%) to 100.0% when pulling a9da39c on t/ckeditor5/1341 into bf774c6 on master.

[Reinmar]

Frankly speaking, this entire code should not be necessary. If we really need all that dereferencing, then it means that something still retains the editor reference, which means that there's some issue that those PRs don't fix.

Also, like I said on the TM, I think that manually destroying UI components is also unnecessary. They should be destroyed by their parents or by the plugin which defined the given root UI component.

I may be wrong, but I think we should look into this more closely because I think this is just hiding some lower-level issue that we have.

[jodator]

Also, like I said on the TM, I think that manually destroying UI components is also unnecessary. They should be destroyed by their parents or by the plugin which defined the given root UI component.

Yes I they weren't destroyed by the parent Plugin - that's the part of the fix.
The other code was needed mostly because of the clickOutsideHandler that attached some listeners to the DOM that no one cleared and some references were still there.

I will review the code to what parts are really needed to fix "static" memory leaks (mostly the FocusHandler or similar dereferences but almost all of those changes were needed to free up memory after properly destroying the editor - which involves properly dereferencing it in the test (ie manual test).

ps.: also the PR is not ready as I need to fix some tests thay fails. I've added the note as I removed it by mistake.

[oleq]

I noticed certain instability when running tests. Locally:

  BalloonEditor build
    memory usage
      ✖ should not grow on multiple create/destroy
        Chrome 71.0.3578 (Mac OS X 10.14.2)
      AssertionError: used heap size should not grow: expected 6141640 to be at most 0

      + expected - actual

      -6141640
      +0

    at Promise.resolve.then.then.then.then.then.then.then.then.then.then.then.then.then.then (webpack:///packages/ckeditor5-core/tests/_utils/memory.js:67 <- build/.automated-tests/entry-point.js:43065:74)


  DecoupledEditor build
    memory usage
      ✖ should not grow on multiple create/destroy
        Chrome 71.0.3578 (Mac OS X 10.14.2)
      Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

and on CI

Chrome 71.0.3578 (Linux 0.0.0) ClassicEditor build memory usage should not grow on multiple create/destroy FAILED
	AssertionError: used heap size should not grow: expected 2766784 to be at most 0
	    at Promise.resolve.then.then.then.then.then.then.then.then.then.then.then.then.then.then (webpack:///./packages/ckeditor5-core/tests/_utils/memory.js?:70:74)
..........17 01 2019 09:29:27.839:WARN [web-server]: 404: /manual/sample.jpg
..............17 01 2019 09:29:30.514:WARN [web-server]: 404: /manual/sample.jpg
........
Chrome 71.0.3578 (Linux 0.0.0) DecoupledEditor build memory usage should not grow on multiple create/destroy FAILED
	Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

note that a different editor type got memory bloated.

[jodator]

It is stable-ish AFAICT:

[jodator]

ps.: It's only the builds which fails so it might be that some plugin which I didn't test in memory manual tests is causing this. I'll try to debug it though.

Other solution is to close an eye for builds and remove those tests ;)

[jodator]

@oleq: after some debugging and also talking with @mlewand it seems that it might beneficial to add some timeouts after window.gc() calls. I've also updated the timeout for whole test as it looks like one of the tests is taking longer to complete (DecoupledEditor tests).

Could you test current state of branch constellation (sorry for messy commits - I needed to trigger CI couple of times) on you machine? Current configuration will run every test 10 times so it could take a while to complete. I'll remove unnecessary it.only() and for loop from the final PR. Right not I'd like to check current setup for stability.

yarn test -cws --files=editor-*,build-*

[jodator]

@oleq: It looks like the CI is stable enough. The only one fails is test timeout which might be due to "heavy" usage of window.gc() during tests (the same tests is repeated 10 times in that run) and IMHO we could live with it. If it the test pass on your local machine I will cleanup git history and we could merge it finally :)

[oleq]

LGTM

[jodator]

@oleq I'm confident enough that current setup will not blow up after merging it.

The CI build is 💚.