From e4a5b4809268d000bf6e2e769153d5001ddb8735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 10 Jan 2019 18:34:32 +0100 Subject: [PATCH 01/10] Introduce test utils for testing memory leaks with Karma. --- tests/_utils/memory.js | 144 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 tests/_utils/memory.js diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js new file mode 100644 index 00000000..1413eb6b --- /dev/null +++ b/tests/_utils/memory.js @@ -0,0 +1,144 @@ +/** + * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md. + */ + +/* global window, document, setTimeout */ + +const MiB = 1024 * 1024; + +/** + * @param {Function} callback Callback with test suit body + */ +export function describeMemoryUsage( callback ) { + describe( 'memory usage', () => { + skipIfNoGarbageCollector(); + + beforeEach( () => collectGarbageAndWait() ); + + beforeEach( createEditorElement ); + + afterEach( destroyEditorElement ); + + callback(); + } ); +} + +/** + * @param {String} testName + * @param {Function} editorCreator Callback which creates editor and returns it's `.create()` promise. + */ +export function testMemoryUsage( testName, editorCreator ) { + it( testName, function() { + // This is a long-running test unfortunately. + this.timeout( 8000 ); + + return runTest( editorCreator ); + } ); +} + +// Runs a single test case. This method will properly setup memory-leak test: +// - create editor +// - run garbage collector +// - record memory allocations +// - destroy the editor +// - create & destroy editor multiple times (6) - after each editor creation the test runner will be paused for ~200ms +function runTest( editorCreator ) { + const createEditor = createAndDestroy( editorCreator ); + + let memoryAfterFirstStart; + + return Promise.resolve() // Promise.resolve just to align below code. + // First editor creation needed to load all editor code,css into the memory (it is reported as used heap memory) + .then( createEditor ) + // Wait for any delayed actions (after editor creation) + .then( wait( 200 ) ) + .then( collectGarbageAndWait ) + .then( editor => { + memoryAfterFirstStart = snapMemInfo(); + + return editor; + } ) + .then( destroy ) + // Run create-wait-destroy multiple times. Multiple runs to grow memory significantly even on smaller leaks. + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + // Finally enforce garbage collection to ensure memory is freed before measuring heap size. + .then( collectGarbageAndWait ) + .then( () => { + const memory = snapMemInfo(); + + const memoryDifference = memory.usedJSHeapSize - memoryAfterFirstStart.usedJSHeapSize; + + expect( memoryDifference, 'used heap size should not grow above 1 MB' ).to.be.below( MiB ); + } ); + + function testWaitAndDestroy() { + return Promise.resolve() + .then( createEditor ) + .then( wait( 200 ) ) + .then( destroy ); + } +} + +function createEditorElement() { + const editorElement = document.createElement( 'div' ); + editorElement.id = 'mem-editor'; + + editorElement.innerHTML = + '

Editor 1

\n' + + '

This is an editor instance. And there\'s some link.

'; + + document.body.appendChild( editorElement ); +} + +function destroyEditorElement() { + document.getElementById( 'mem-editor' ).remove(); +} + +function createAndDestroy( editorCreator ) { + return () => editorCreator(); +} + +function destroy( editor ) { + return editor.destroy(); +} + +function snapMemInfo() { + const memeInfo = window.performance.memory; + + return { + totalJSHeapSize: memeInfo.totalJSHeapSize, + usedJSHeapSize: memeInfo.usedJSHeapSize, + jsHeapSizeLimit: memeInfo.jsHeapSizeLimit + }; +} + +// Will skip test suite if not in compatible browser. +// Currently on Google Chrome supports this method and must be run with proper flags: +// +// google-chrome -js-flags="--expose-gc" +// +function skipIfNoGarbageCollector() { + before( function() { + if ( !window.gc ) { + this.skip(); + } + } ); +} + +function collectGarbageAndWait( pass ) { + window.gc(); + + return Promise.resolve( pass ).then( wait( 500 ) ); +} + +// Simple method that returns a helper method that returns a promise which resolves after given timeout. +// The returned promise will pass the value from previus call (usually and editor instance). +function wait( ms ) { + return editor => new Promise( resolve => setTimeout( () => resolve( editor ), ms ) ); +} From a50586195370d10168c9f5fa49c4819d17d7d2e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 15 Jan 2019 10:44:50 +0100 Subject: [PATCH 02/10] Improve editor destructuring process. --- src/editor/editor.js | 9 +++++++++ src/editor/editorui.js | 1 + 2 files changed, 10 insertions(+) diff --git a/src/editor/editor.js b/src/editor/editor.js index ccafa12a..82cf7ec3 100644 --- a/src/editor/editor.js +++ b/src/editor/editor.js @@ -278,6 +278,15 @@ export default class Editor { this.data.destroy(); this.editing.destroy(); this.keystrokes.destroy(); + + // Fail-safe dereference of entities that may hold back reference to the editor - prevent memory leaks #1341 + this.plugins = null; + this.commands = null; + this.model = null; + this.data = null; + this.editing = null; + this.keystrokes = null; + this.conversion = null; } ); } diff --git a/src/editor/editorui.js b/src/editor/editorui.js index 55dabdc3..0b594aae 100644 --- a/src/editor/editorui.js +++ b/src/editor/editorui.js @@ -80,6 +80,7 @@ export default class EditorUI { destroy() { this.stopListening(); this.view.destroy(); + this.focusTracker.destroy(); } /** From 80f51d4b3498f1d450d5079bf4106a74e4ff0ddf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 15 Jan 2019 11:05:26 +0100 Subject: [PATCH 03/10] Add memory leak test retry run if it fails. --- tests/_utils/memory.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index 1413eb6b..42db25fc 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -33,6 +33,10 @@ export function testMemoryUsage( testName, editorCreator ) { // This is a long-running test unfortunately. this.timeout( 8000 ); + // It happens from time to time that Browser will allocate additional resources and the test will fail slightly by ~100-200kB. + // In such scenarios another run of test should pass. + this.retries( 2 ); + return runTest( editorCreator ); } ); } From 815b2ae50eee6339841020d7746e4d392d87202d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 15 Jan 2019 14:37:57 +0100 Subject: [PATCH 04/10] Make memory test fail if memory grows over 0 bytes. --- tests/_utils/memory.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index 42db25fc..493e07f6 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -5,8 +5,6 @@ /* global window, document, setTimeout */ -const MiB = 1024 * 1024; - /** * @param {Function} callback Callback with test suit body */ @@ -33,11 +31,7 @@ export function testMemoryUsage( testName, editorCreator ) { // This is a long-running test unfortunately. this.timeout( 8000 ); - // It happens from time to time that Browser will allocate additional resources and the test will fail slightly by ~100-200kB. - // In such scenarios another run of test should pass. - this.retries( 2 ); - - return runTest( editorCreator ); + return runTest( editorCreator, this ); } ); } @@ -47,7 +41,7 @@ export function testMemoryUsage( testName, editorCreator ) { // - record memory allocations // - destroy the editor // - create & destroy editor multiple times (6) - after each editor creation the test runner will be paused for ~200ms -function runTest( editorCreator ) { +function runTest( editorCreator, test ) { const createEditor = createAndDestroy( editorCreator ); let memoryAfterFirstStart; @@ -71,14 +65,18 @@ function runTest( editorCreator ) { .then( testWaitAndDestroy ) .then( testWaitAndDestroy ) .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) + .then( testWaitAndDestroy ) // Finally enforce garbage collection to ensure memory is freed before measuring heap size. .then( collectGarbageAndWait ) .then( () => { const memory = snapMemInfo(); const memoryDifference = memory.usedJSHeapSize - memoryAfterFirstStart.usedJSHeapSize; + test._memoryDiff = memoryDifference; - expect( memoryDifference, 'used heap size should not grow above 1 MB' ).to.be.below( MiB ); + expect( memoryDifference, 'used heap size should not grow' ).to.be.at.most( 0 ); } ); function testWaitAndDestroy() { From 889482bfe4f38fe15193fa9c98c284098efba6ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 15 Jan 2019 14:52:27 +0100 Subject: [PATCH 05/10] Remove redundant timeouts and garbage collector calls. --- tests/_utils/memory.js | 62 ++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 42 deletions(-) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index 493e07f6..083c2809 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* global window, document, setTimeout */ +/* global window, document */ /** * @param {Function} callback Callback with test suit body @@ -12,8 +12,6 @@ export function describeMemoryUsage( callback ) { describe( 'memory usage', () => { skipIfNoGarbageCollector(); - beforeEach( () => collectGarbageAndWait() ); - beforeEach( createEditorElement ); afterEach( destroyEditorElement ); @@ -27,12 +25,7 @@ export function describeMemoryUsage( callback ) { * @param {Function} editorCreator Callback which creates editor and returns it's `.create()` promise. */ export function testMemoryUsage( testName, editorCreator ) { - it( testName, function() { - // This is a long-running test unfortunately. - this.timeout( 8000 ); - - return runTest( editorCreator, this ); - } ); + it( testName, () => runTest( editorCreator ) ); } // Runs a single test case. This method will properly setup memory-leak test: @@ -41,7 +34,7 @@ export function testMemoryUsage( testName, editorCreator ) { // - record memory allocations // - destroy the editor // - create & destroy editor multiple times (6) - after each editor creation the test runner will be paused for ~200ms -function runTest( editorCreator, test ) { +function runTest( editorCreator ) { const createEditor = createAndDestroy( editorCreator ); let memoryAfterFirstStart; @@ -49,40 +42,34 @@ function runTest( editorCreator, test ) { return Promise.resolve() // Promise.resolve just to align below code. // First editor creation needed to load all editor code,css into the memory (it is reported as used heap memory) .then( createEditor ) - // Wait for any delayed actions (after editor creation) - .then( wait( 200 ) ) - .then( collectGarbageAndWait ) .then( editor => { - memoryAfterFirstStart = snapMemInfo(); + memoryAfterFirstStart = collectMemoryStats(); return editor; } ) .then( destroy ) // Run create-wait-destroy multiple times. Multiple runs to grow memory significantly even on smaller leaks. - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - .then( testWaitAndDestroy ) - // Finally enforce garbage collection to ensure memory is freed before measuring heap size. - .then( collectGarbageAndWait ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) + .then( testAndDestroy ) .then( () => { - const memory = snapMemInfo(); + const memory = collectMemoryStats(); const memoryDifference = memory.usedJSHeapSize - memoryAfterFirstStart.usedJSHeapSize; - test._memoryDiff = memoryDifference; expect( memoryDifference, 'used heap size should not grow' ).to.be.at.most( 0 ); } ); - function testWaitAndDestroy() { + function testAndDestroy() { return Promise.resolve() .then( createEditor ) - .then( wait( 200 ) ) .then( destroy ); } } @@ -110,7 +97,10 @@ function destroy( editor ) { return editor.destroy(); } -function snapMemInfo() { +function collectMemoryStats() { + // Enforce garbage collection before recording memory stats. + window.gc(); + const memeInfo = window.performance.memory; return { @@ -132,15 +122,3 @@ function skipIfNoGarbageCollector() { } } ); } - -function collectGarbageAndWait( pass ) { - window.gc(); - - return Promise.resolve( pass ).then( wait( 500 ) ); -} - -// Simple method that returns a helper method that returns a promise which resolves after given timeout. -// The returned promise will pass the value from previus call (usually and editor instance). -function wait( ms ) { - return editor => new Promise( resolve => setTimeout( () => resolve( editor ), ms ) ); -} From 591b9470a22093b802661ba9f3e7c667c924f053 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 16 Jan 2019 14:16:22 +0100 Subject: [PATCH 06/10] Remove redundant variables de-referencing. --- src/editor/editor.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/editor/editor.js b/src/editor/editor.js index 82cf7ec3..ccafa12a 100644 --- a/src/editor/editor.js +++ b/src/editor/editor.js @@ -278,15 +278,6 @@ export default class Editor { this.data.destroy(); this.editing.destroy(); this.keystrokes.destroy(); - - // Fail-safe dereference of entities that may hold back reference to the editor - prevent memory leaks #1341 - this.plugins = null; - this.commands = null; - this.model = null; - this.data = null; - this.editing = null; - this.keystrokes = null; - this.conversion = null; } ); } From 782f26acbcdadc35b56eaece29f700619e8d6564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Thu, 17 Jan 2019 12:12:44 +0100 Subject: [PATCH 07/10] Increase timeout in memory tests. --- tests/_utils/memory.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index 083c2809..8ff170c1 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -25,7 +25,11 @@ export function describeMemoryUsage( callback ) { * @param {Function} editorCreator Callback which creates editor and returns it's `.create()` promise. */ export function testMemoryUsage( testName, editorCreator ) { - it( testName, () => runTest( editorCreator ) ); + it( testName, function() { + this.timeout( 6000 ); + + return runTest( editorCreator ); + } ); } // Runs a single test case. This method will properly setup memory-leak test: From f8230e977820d0d29de9515d49ad85eb7940c8df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Mon, 21 Jan 2019 18:55:52 +0100 Subject: [PATCH 08/10] Make collecting memory statistic a promise. --- tests/_utils/memory.js | 45 +++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index 8ff170c1..fcf63f04 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* global window, document */ +/* global window, document, setTimeout */ /** * @param {Function} callback Callback with test suit body @@ -47,9 +47,11 @@ function runTest( editorCreator ) { // First editor creation needed to load all editor code,css into the memory (it is reported as used heap memory) .then( createEditor ) .then( editor => { - memoryAfterFirstStart = collectMemoryStats(); + return collectMemoryStats().then( mem => { + memoryAfterFirstStart = mem; - return editor; + return editor; + } ); } ) .then( destroy ) // Run create-wait-destroy multiple times. Multiple runs to grow memory significantly even on smaller leaks. @@ -64,11 +66,14 @@ function runTest( editorCreator ) { .then( testAndDestroy ) .then( testAndDestroy ) .then( () => { - const memory = collectMemoryStats(); - - const memoryDifference = memory.usedJSHeapSize - memoryAfterFirstStart.usedJSHeapSize; - - expect( memoryDifference, 'used heap size should not grow' ).to.be.at.most( 0 ); + return new Promise( resolve => { + collectMemoryStats().then( memory => { + const memoryDifference = memory.usedJSHeapSize - memoryAfterFirstStart.usedJSHeapSize; + + expect( memoryDifference, 'used heap size should not grow' ).to.be.at.most( 0 ); + resolve(); + } ); + } ); } ); function testAndDestroy() { @@ -102,16 +107,20 @@ function destroy( editor ) { } function collectMemoryStats() { - // Enforce garbage collection before recording memory stats. - window.gc(); - - const memeInfo = window.performance.memory; - - return { - totalJSHeapSize: memeInfo.totalJSHeapSize, - usedJSHeapSize: memeInfo.usedJSHeapSize, - jsHeapSizeLimit: memeInfo.jsHeapSizeLimit - }; + return new Promise( resolve => { + // Enforce garbage collection before recording memory stats. + window.gc(); + + setTimeout( () => { + const memeInfo = window.performance.memory; + + resolve( { + totalJSHeapSize: memeInfo.totalJSHeapSize, + usedJSHeapSize: memeInfo.usedJSHeapSize, + jsHeapSizeLimit: memeInfo.jsHeapSizeLimit + } ); + }, 500 ); + } ); } // Will skip test suite if not in compatible browser. From 44d1a44129ee09156b63aae0d208cade568a051b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 22 Jan 2019 10:07:42 +0100 Subject: [PATCH 09/10] Extract magic numbers to constants. --- tests/_utils/memory.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index fcf63f04..bd79abd7 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -5,6 +5,9 @@ /* global window, document, setTimeout */ +const TEST_TIMEOUT = 6000; +const GARBAGE_COLLECTOR_TIMEOUT = 500; + /** * @param {Function} callback Callback with test suit body */ @@ -26,7 +29,7 @@ export function describeMemoryUsage( callback ) { */ export function testMemoryUsage( testName, editorCreator ) { it( testName, function() { - this.timeout( 6000 ); + this.timeout( TEST_TIMEOUT ); return runTest( editorCreator ); } ); @@ -119,7 +122,7 @@ function collectMemoryStats() { usedJSHeapSize: memeInfo.usedJSHeapSize, jsHeapSizeLimit: memeInfo.jsHeapSizeLimit } ); - }, 500 ); + }, GARBAGE_COLLECTOR_TIMEOUT ); } ); } From 2825fed7abd0c04867981d91f7e4bdc7f4c6e841 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 22 Jan 2019 10:28:30 +0100 Subject: [PATCH 10/10] Update docs for memory usage test functions. --- tests/_utils/memory.js | 48 +++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/tests/_utils/memory.js b/tests/_utils/memory.js index bd79abd7..77b75f6d 100644 --- a/tests/_utils/memory.js +++ b/tests/_utils/memory.js @@ -9,6 +9,22 @@ const TEST_TIMEOUT = 6000; const GARBAGE_COLLECTOR_TIMEOUT = 500; /** + * Memory tests suite definition that: + * - skips tests when garbage collector is not available. + * - creates/destroys editor element (id = 'mem-editor'). + * + * This method should be used with dedicated memory usage test case functions: + * + * describe( 'editor', () => { + * // Other tests. + * + * describeMemoryUsage( () => { + * testMemoryUsage( 'and editor', () => { + * return ClassicEditor.create( document.querySelector( '#mem-editor' ) ); + * } ); + * } ); + * } ); + * * @param {Function} callback Callback with test suit body */ export function describeMemoryUsage( callback ) { @@ -24,7 +40,16 @@ export function describeMemoryUsage( callback ) { } /** - * @param {String} testName + * Single test case for memory usage test. This method will handle memory usage test procedure: + * - creating editor instance + * - recording its memory usage (after garbage collector) + * - create and destroy editor 10 times + * - record memory usage after final editor destroy (after garbage collector) + * - tests if memory grew + * + * See {@link describeMemoryUsage} function for usage details. + * + * @param {String} testName Name of a test case. * @param {Function} editorCreator Callback which creates editor and returns it's `.create()` promise. */ export function testMemoryUsage( testName, editorCreator ) { @@ -40,7 +65,7 @@ export function testMemoryUsage( testName, editorCreator ) { // - run garbage collector // - record memory allocations // - destroy the editor -// - create & destroy editor multiple times (6) - after each editor creation the test runner will be paused for ~200ms +// - create & destroy editor multiple times (9) - after each editor creation the test runner will be paused for ~200ms function runTest( editorCreator ) { const createEditor = createAndDestroy( editorCreator ); @@ -58,16 +83,15 @@ function runTest( editorCreator ) { } ) .then( destroy ) // Run create-wait-destroy multiple times. Multiple runs to grow memory significantly even on smaller leaks. - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) - .then( testAndDestroy ) + .then( testAndDestroy ) // #2 + .then( testAndDestroy ) // #3 + .then( testAndDestroy ) // #4 + .then( testAndDestroy ) // #5 + .then( testAndDestroy ) // #6 + .then( testAndDestroy ) // #7 + .then( testAndDestroy ) // #8 + .then( testAndDestroy ) // #9 + .then( testAndDestroy ) // #10 .then( () => { return new Promise( resolve => { collectMemoryStats().then( memory => {