From c123ebc948a48e4bf3cec4c8baf3c78dad293e32 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Thu, 7 Jul 2016 01:10:30 +0200 Subject: [PATCH 1/4] Memory leak fix candidates: 1) purge must destroy scene2d otherwise rAF never stops; 2) scene2d.destroy should destroy the traces (cherry picked from commit 8f6f2dd) --- src/plots/gl2d/scene2d.js | 8 ++++++++ src/plots/plots.js | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/plots/gl2d/scene2d.js b/src/plots/gl2d/scene2d.js index 68d04622779..ceceba6a225 100644 --- a/src/plots/gl2d/scene2d.js +++ b/src/plots/gl2d/scene2d.js @@ -318,6 +318,14 @@ proto.destroy = function() { this.glplot = null; this.stopped = true; + + var traces = this.traces + if(traces) { + Object.keys(traces).map(function(key) { + traces[key].dispose(); + traces[key] = null; + }) + } }; proto.plot = function(fullData, calcData, fullLayout) { diff --git a/src/plots/plots.js b/src/plots/plots.js index 4b814bfd85c..7b16626ec05 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -809,6 +809,16 @@ plots.purge = function(gd) { // remove modebar if(fullLayout._modeBar) fullLayout._modeBar.destroy(); + if(fullLayout._plots) { + Object.keys(fullLayout._plots).map(function(key) { + var plot = fullLayout._plots[key]; + if(plot._scene2d) { + plot._scene2d.destroy(); + plot._scene2d = null; + } + }); + } + // data and layout delete gd.data; delete gd.layout; From 6c92920d8c11fe73ca2199b124e2d5c7afb30e19 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Thu, 7 Jul 2016 01:28:57 +0200 Subject: [PATCH 2/4] Lint --- src/plots/gl2d/scene2d.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plots/gl2d/scene2d.js b/src/plots/gl2d/scene2d.js index ceceba6a225..f7cc4361d54 100644 --- a/src/plots/gl2d/scene2d.js +++ b/src/plots/gl2d/scene2d.js @@ -319,12 +319,12 @@ proto.destroy = function() { this.glplot = null; this.stopped = true; - var traces = this.traces + var traces = this.traces; if(traces) { Object.keys(traces).map(function(key) { traces[key].dispose(); traces[key] = null; - }) + }); } }; From 57c7779d579029f76a2257c615c09311f562b082 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Fri, 8 Jul 2016 12:05:38 +0200 Subject: [PATCH 3/4] Call cleanPlot from newPlot directly; Changed the location of the trace dispose loop; null -> delete --- src/plot_api/plot_api.js | 4 ++++ src/plots/gl2d/scene2d.js | 18 ++++++++++-------- src/plots/plots.js | 10 ---------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 371f834584e..8b3cd559c4b 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -835,6 +835,10 @@ Plotly.redraw = function(gd) { */ Plotly.newPlot = function(gd, data, layout, config) { gd = getGraphDiv(gd); + + // remove gl contexts + Plots.cleanPlot([], {}, gd._fullData || {}, gd._fullLayout || {}); + Plots.purge(gd); return Plotly.plot(gd, data, layout, config); }; diff --git a/src/plots/gl2d/scene2d.js b/src/plots/gl2d/scene2d.js index f7cc4361d54..0d859b2acac 100644 --- a/src/plots/gl2d/scene2d.js +++ b/src/plots/gl2d/scene2d.js @@ -310,6 +310,16 @@ proto.cameraChanged = function() { }; proto.destroy = function() { + + var traces = this.traces; + + if(traces) { + Object.keys(traces).map(function(key) { + traces[key].dispose(); + delete traces[key]; + }); + } + this.glplot.dispose(); if(!this.staticPlot) this.container.removeChild(this.canvas); @@ -318,14 +328,6 @@ proto.destroy = function() { this.glplot = null; this.stopped = true; - - var traces = this.traces; - if(traces) { - Object.keys(traces).map(function(key) { - traces[key].dispose(); - traces[key] = null; - }); - } }; proto.plot = function(fullData, calcData, fullLayout) { diff --git a/src/plots/plots.js b/src/plots/plots.js index 7b16626ec05..4b814bfd85c 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -809,16 +809,6 @@ plots.purge = function(gd) { // remove modebar if(fullLayout._modeBar) fullLayout._modeBar.destroy(); - if(fullLayout._plots) { - Object.keys(fullLayout._plots).map(function(key) { - var plot = fullLayout._plots[key]; - if(plot._scene2d) { - plot._scene2d.destroy(); - plot._scene2d = null; - } - }); - } - // data and layout delete gd.data; delete gd.layout; From 893766890c8382d52b694225746bae64f574b077 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Fri, 8 Jul 2016 17:12:45 +0200 Subject: [PATCH 4/4] Adding test cases that check that Plotly.newPlot indeed causes dereferencing of the GL context or its own canvas --- test/jasmine/tests/gl_plot_interact_test.js | 134 ++++++++++++++++---- 1 file changed, 112 insertions(+), 22 deletions(-) diff --git a/test/jasmine/tests/gl_plot_interact_test.js b/test/jasmine/tests/gl_plot_interact_test.js index eb6b3fec10a..5fca392c06a 100644 --- a/test/jasmine/tests/gl_plot_interact_test.js +++ b/test/jasmine/tests/gl_plot_interact_test.js @@ -507,41 +507,131 @@ describe('Test gl plot interactions', function() { }); }); - describe('Plots.cleanPlot', function() { + describe('Removal of gl contexts', function() { - it('should remove gl context from the graph div of a gl3d plot', function(done) { - gd = createGraphDiv(); + var mockData2d = [{ + type: 'scattergl', + x: [1, 2, 3], + y: [2, 1, 3] + }]; - var mockData = [{ - type: 'scatter3d' - }]; - Plotly.plot(gd, mockData).then(function() { - expect(gd._fullLayout.scene._scene.glplot).toBeDefined(); + var mockData3d = [{ + type: 'scatter3d', + x: [1, 2, 3], + y: [2, 1, 3], + z: [3, 2, 1] + }]; - Plots.cleanPlot([], {}, gd._fullData, gd._fullLayout); - expect(gd._fullLayout.scene._scene.glplot).toBe(null); + describe('Plots.cleanPlot', function() { - done(); + it('should remove gl context from the graph div of a gl3d plot', function(done) { + gd = createGraphDiv(); + + Plotly.plot(gd, mockData3d).then(function() { + expect(gd._fullLayout.scene._scene.glplot).toBeDefined(); + + Plots.cleanPlot([], {}, gd._fullData, gd._fullLayout); + expect(gd._fullLayout.scene._scene.glplot).toBe(null); + + done(); + }); }); - }); - it('should remove gl context from the graph div of a gl2d plot', function(done) { - gd = createGraphDiv(); + it('should remove gl context from the graph div of a gl2d plot', function(done) { + gd = createGraphDiv(); - var mockData = [{ + Plotly.plot(gd, mockData2d).then(function() { + expect(gd._fullLayout._plots.xy._scene2d.glplot).toBeDefined(); + + Plots.cleanPlot([], {}, gd._fullData, gd._fullLayout); + expect(gd._fullLayout._plots).toEqual({}); + + done(); + }); + }); + }); + describe('Plotly.newPlot', function() { + + var mockData2dNew = [{ type: 'scattergl', - x: [1, 2, 3], - y: [1, 2, 3] + x: [1, 3, 2], + y: [2, 3, 1] }]; - Plotly.plot(gd, mockData).then(function() { - expect(gd._fullLayout._plots.xy._scene2d.glplot).toBeDefined(); - Plots.cleanPlot([], {}, gd._fullData, gd._fullLayout); - expect(gd._fullLayout._plots).toEqual({}); + var mockData3dNew = [{ + type: 'scatter3d', + x: [2, 1, 3], + y: [1, 2, 3], + z: [2, 1, 3] + }]; - done(); + + it('should remove gl context from the graph div of a gl3d plot', function(done) { + gd = createGraphDiv(); + + Plotly.plot(gd, mockData3d).then(function() { + + var firstGlplotObject = gd._fullLayout.scene._scene.glplot; + var firstGlContext = firstGlplotObject.gl; + var firstCanvas = firstGlContext.canvas; + + expect(firstGlplotObject).toBeDefined(); + + Plotly.newPlot(gd, mockData3dNew, {}).then(function() { + + var secondGlplotObject = gd._fullLayout.scene._scene.glplot; + var secondGlContext = secondGlplotObject.gl; + var secondCanvas = secondGlContext.canvas; + + expect(secondGlplotObject).not.toBe(firstGlplotObject); + expect(firstGlplotObject.gl === null); + expect(secondGlContext instanceof WebGLRenderingContext); + expect(secondGlContext).not.toBe(firstGlContext); + + // The same canvas can't possibly be reassinged a new WebGL context, but let's leave room + // for the implementation to make the context get lost and have the old canvas stick around + // in a disused state. + expect(firstCanvas.parentNode === null || + firstCanvas !== secondCanvas && firstGlContext.isContextLost()); + + done(); + + }); + }); + }); + + it('should remove gl context from the graph div of a gl2d plot', function(done) { + gd = createGraphDiv(); + + Plotly.plot(gd, mockData2d).then(function() { + + var firstGlplotObject = gd._fullLayout._plots.xy._scene2d.glplot; + var firstGlContext = firstGlplotObject.gl; + var firstCanvas = firstGlContext.canvas; + + expect(firstGlplotObject).toBeDefined(); + expect(firstGlContext).toBeDefined(); + expect(firstGlContext instanceof WebGLRenderingContext); + + Plotly.newPlot(gd, mockData2dNew, {}).then(function() { + + var secondGlplotObject = gd._fullLayout._plots.xy._scene2d.glplot; + var secondGlContext = secondGlplotObject.gl; + var secondCanvas = secondGlContext.canvas; + + expect(Object.keys(gd._fullLayout._plots).length === 1); + expect(secondGlplotObject).not.toBe(firstGlplotObject); + expect(firstGlplotObject.gl === null); + expect(secondGlContext instanceof WebGLRenderingContext); + expect(secondGlContext).not.toBe(firstGlContext); + expect(firstCanvas.parentNode === null || + firstCanvas !== secondCanvas && firstGlContext.isContextLost()); + + done(); + }); + }); }); }); });