From 335648d6132696350571033cb04da27b60ff5012 Mon Sep 17 00:00:00 2001 From: ahocevar Date: Thu, 16 May 2019 20:33:20 +0200 Subject: [PATCH] Remove memory leak caused by label cache listeners --- src/ol/PluggableMap.js | 38 ++++++- src/ol/layer/Layer.js | 7 ++ src/ol/renderer/Composite.js | 10 +- src/ol/renderer/Layer.js | 6 + src/ol/renderer/Map.js | 105 +----------------- src/ol/renderer/canvas/VectorImageLayer.js | 7 ++ src/ol/renderer/canvas/VectorLayer.js | 17 +-- src/ol/renderer/canvas/VectorTileLayer.js | 11 +- .../spec/ol/renderer/canvas/tilelayer.test.js | 2 +- .../ol/renderer/canvas/vectorlayer.test.js | 2 +- .../renderer/canvas/vectortilelayer.test.js | 2 +- 11 files changed, 71 insertions(+), 136 deletions(-) diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js index 9ab36d6b55..a8e7aeaf9d 100644 --- a/src/ol/PluggableMap.js +++ b/src/ol/PluggableMap.js @@ -293,6 +293,11 @@ class PluggableMap extends BaseObject { */ this.interactions = optionsInternal.interactions || new Collection(); + /** + * @type {import("./events.js").EventsKey} + */ + this.labelCacheListenerKey_; + /** * @type {Collection} * @private @@ -498,6 +503,24 @@ class PluggableMap extends BaseObject { overlay.setMap(this); } + /** + * Attach a label cache for listening to font changes. + * @param {import("./events/Target.js").default} labelCache Label cache. + */ + attachLabelCache(labelCache) { + this.detachLabelCache(); + this.labelCacheListenerKey_ = listen(labelCache, EventType.CLEAR, this.redrawText.bind(this)); + } + + /** + * Detach the label cache, i.e. no longer listen to font changes. + */ + detachLabelCache() { + if (this.labelCacheListenerKey_) { + unlistenByKey(this.labelCacheListenerKey_); + } + } + /** * * @inheritDoc @@ -515,6 +538,7 @@ class PluggableMap extends BaseObject { cancelAnimationFrame(this.animationDelayKey_); this.animationDelayKey_ = undefined; } + this.detachLabelCache(); this.setTarget(null); super.disposeInternal(); } @@ -994,7 +1018,6 @@ class PluggableMap extends BaseObject { } if (!targetElement) { - this.renderer_.removeLayerRenderers(); removeNode(this.viewport_); if (this.handleResize_ !== undefined) { removeEventListener(EventType.RESIZE, this.handleResize_, false); @@ -1102,6 +1125,19 @@ class PluggableMap extends BaseObject { this.animationDelay_(); } + /** + * Redraws all text after new fonts have loaded + */ + redrawText() { + const layerStates = this.getLayerGroup().getLayerStatesArray(); + for (let i = 0, ii = layerStates.length; i < ii; ++i) { + const layer = layerStates[i].layer; + if (layer.hasRenderer()) { + layer.getRenderer().handleFontsChanged(); + } + } + } + /** * Request a map rendering (at the next animation frame). * @api diff --git a/src/ol/layer/Layer.js b/src/ol/layer/Layer.js index 775a0cd3d1..23a4324a63 100644 --- a/src/ol/layer/Layer.js +++ b/src/ol/layer/Layer.js @@ -254,6 +254,13 @@ class Layer extends BaseLayer { return this.renderer_; } + /** + * @return {boolean} The layer has a renderer. + */ + hasRenderer() { + return !!this.renderer_; + } + /** * Create a renderer for this layer. * @return {import("../renderer/Layer.js").default} A layer renderer. diff --git a/src/ol/renderer/Composite.js b/src/ol/renderer/Composite.js index 9b4ce2a1e7..e1e68f70cd 100644 --- a/src/ol/renderer/Composite.js +++ b/src/ol/renderer/Composite.js @@ -8,6 +8,7 @@ import RenderEventType from '../render/EventType.js'; import MapRenderer from './Map.js'; import SourceState from '../source/State.js'; import {replaceChildren} from '../dom.js'; +import {labelCache} from '../render/canvas.js'; /** @@ -22,6 +23,7 @@ class CompositeMapRenderer extends MapRenderer { */ constructor(map) { super(map); + map.attachLabelCache(labelCache); /** * @private @@ -111,7 +113,6 @@ class CompositeMapRenderer extends MapRenderer { this.renderedVisible_ = true; } - this.scheduleRemoveUnusedLayerRenderers(frameState); this.scheduleExpireIconCache(frameState); } @@ -128,11 +129,8 @@ class CompositeMapRenderer extends MapRenderer { for (let i = numLayers - 1; i >= 0; --i) { const layerState = layerStates[i]; const layer = layerState.layer; - if (visibleAtResolution(layerState, viewResolution) && layerFilter(layer)) { - const layerRenderer = this.getLayerRenderer(layer); - if (!layerRenderer) { - continue; - } + if (layer.hasRenderer() && visibleAtResolution(layerState, viewResolution) && layerFilter(layer)) { + const layerRenderer = layer.getRenderer(); const data = layerRenderer.getDataAtPixel(pixel, frameState, hitTolerance); if (data) { const result = callback(layer, data); diff --git a/src/ol/renderer/Layer.js b/src/ol/renderer/Layer.js index b63ea42195..f266a28c57 100644 --- a/src/ol/renderer/Layer.js +++ b/src/ol/renderer/Layer.js @@ -115,6 +115,12 @@ class LayerRenderer extends Observable { return this.layer_; } + /** + * @abstract + * Perform action necessary to get the layer rendered after new fonts have loaded + */ + handleFontsChanged() {} + /** * Handle changes in image state. * @param {import("../events/Event.js").default} event Image change event. diff --git a/src/ol/renderer/Map.js b/src/ol/renderer/Map.js index 9955be7a51..8cde8fc7f2 100644 --- a/src/ol/renderer/Map.js +++ b/src/ol/renderer/Map.js @@ -3,8 +3,6 @@ */ import {abstract, getUid} from '../util.js'; import Disposable from '../Disposable.js'; -import {listen, unlistenByKey} from '../events.js'; -import EventType from '../events/EventType.js'; import {getWidth} from '../extent.js'; import {TRUE} from '../functions.js'; import {visibleAtResolution} from '../layer/Layer.js'; @@ -34,18 +32,6 @@ class MapRenderer extends Disposable { */ this.declutterTree_ = null; - /** - * @private - * @type {!Object} - */ - this.layerRenderers_ = {}; - - /** - * @private - * @type {Object} - */ - this.layerRendererListeners_ = {}; - } /** @@ -75,15 +61,6 @@ class MapRenderer extends Disposable { makeInverse(pixelToCoordinateTransform, coordinateToPixelTransform); } - /** - * Removes all layer renderers. - */ - removeLayerRenderers() { - for (const key in this.layerRenderers_) { - this.removeLayerRendererByKey_(key).dispose(); - } - } - /** * @param {import("../coordinate.js").Coordinate} coordinate Coordinate. * @param {import("../PluggableMap.js").FrameState} frameState FrameState. @@ -149,8 +126,8 @@ class MapRenderer extends Disposable { for (i = numLayers - 1; i >= 0; --i) { const layerState = layerStates[i]; const layer = /** @type {import("../layer/Layer.js").default} */ (layerState.layer); - if (visibleAtResolution(layerState, viewResolution) && layerFilter.call(thisArg2, layer)) { - const layerRenderer = this.getLayerRenderer(layer); + if (layer.hasRenderer() && visibleAtResolution(layerState, viewResolution) && layerFilter.call(thisArg2, layer)) { + const layerRenderer = layer.getRenderer(); const source = layer.getSource(); if (layerRenderer && source) { const callback = forEachFeatureAtCoordinate.bind(null, layerState.managed); @@ -203,35 +180,6 @@ class MapRenderer extends Disposable { return hasFeature !== undefined; } - /** - * @param {import("../layer/Layer.js").default} layer Layer. - * @protected - * @return {import("./Layer.js").default} Layer renderer. May return null. - */ - getLayerRenderer(layer) { - const layerKey = getUid(layer); - if (layerKey in this.layerRenderers_) { - return this.layerRenderers_[layerKey]; - } - - const renderer = layer.getRenderer(); - if (!renderer) { - return null; - } - - this.layerRenderers_[layerKey] = renderer; - this.layerRendererListeners_[layerKey] = listen(renderer, EventType.CHANGE, this.handleLayerRendererChange_, this); - return renderer; - } - - /** - * @protected - * @return {Object} Layer renderers. - */ - getLayerRenderers() { - return this.layerRenderers_; - } - /** * @return {import("../PluggableMap.js").default} Map. */ @@ -239,29 +187,6 @@ class MapRenderer extends Disposable { return this.map_; } - /** - * Handle changes in a layer renderer. - * @private - */ - handleLayerRendererChange_() { - this.map_.render(); - } - - /** - * @param {string} layerKey Layer key. - * @return {import("./Layer.js").default} Layer renderer. - * @private - */ - removeLayerRendererByKey_(layerKey) { - const layerRenderer = this.layerRenderers_[layerKey]; - delete this.layerRenderers_[layerKey]; - - unlistenByKey(this.layerRendererListeners_[layerKey]); - delete this.layerRendererListeners_[layerKey]; - - return layerRenderer; - } - /** * Render. * @param {?import("../PluggableMap.js").FrameState} frameState Frame state. @@ -279,21 +204,6 @@ class MapRenderer extends Disposable { frameState.postRenderFunctions.push(expireIconCache); } } - - /** - * @param {!import("../PluggableMap.js").FrameState} frameState Frame state. - * @protected - */ - scheduleRemoveUnusedLayerRenderers(frameState) { - const layerStatesMap = getLayerStatesMap(frameState.layerStatesArray); - for (const layerKey in this.layerRenderers_) { - if (!(layerKey in layerStatesMap)) { - frameState.postRenderFunctions.push(function() { - this.removeLayerRendererByKey_(layerKey).dispose(); - }.bind(this)); - } - } - } } @@ -305,15 +215,4 @@ function expireIconCache(map, frameState) { iconImageCache.expire(); } -/** - * @param {Array} layerStatesArray Layer states array. - * @return {Object} States mapped by layer uid. - */ -function getLayerStatesMap(layerStatesArray) { - return layerStatesArray.reduce(function(acc, state) { - acc[getUid(state.layer)] = state; - return acc; - }, {}); -} - export default MapRenderer; diff --git a/src/ol/renderer/canvas/VectorImageLayer.js b/src/ol/renderer/canvas/VectorImageLayer.js index 25ba444582..13a3e14c14 100644 --- a/src/ol/renderer/canvas/VectorImageLayer.js +++ b/src/ol/renderer/canvas/VectorImageLayer.js @@ -53,6 +53,13 @@ class CanvasVectorImageLayerRenderer extends CanvasImageLayerRenderer { super.disposeInternal(); } + /** + * @inheritDoc + */ + handleFontsChanged() { + this.vectorRenderer_.handleFontsChanged(); + } + /** * @inheritDoc */ diff --git a/src/ol/renderer/canvas/VectorLayer.js b/src/ol/renderer/canvas/VectorLayer.js index 26a9ff6e4b..3c481c9b62 100644 --- a/src/ol/renderer/canvas/VectorLayer.js +++ b/src/ol/renderer/canvas/VectorLayer.js @@ -3,10 +3,7 @@ */ import {getUid} from '../../util.js'; import ViewHint from '../../ViewHint.js'; -import {listen, unlisten} from '../../events.js'; -import EventType from '../../events/EventType.js'; import {buffer, createEmpty, containsExtent, getWidth} from '../../extent.js'; -import {labelCache} from '../../render/canvas.js'; import CanvasBuilderGroup from '../../render/canvas/BuilderGroup.js'; import ExecutorGroup, {replayDeclutter} from '../../render/canvas/ExecutorGroup.js'; import CanvasLayerRenderer from './Layer.js'; @@ -68,16 +65,6 @@ class CanvasVectorLayerRenderer extends CanvasLayerRenderer { * @type {boolean} */ this.replayGroupChanged = true; - - listen(labelCache, EventType.CLEAR, this.handleFontsChanged_, this); - } - - /** - * @inheritDoc - */ - disposeInternal() { - unlisten(labelCache, EventType.CLEAR, this.handleFontsChanged_, this); - super.disposeInternal(); } /** @@ -211,9 +198,9 @@ class CanvasVectorLayerRenderer extends CanvasLayerRenderer { } /** - * @param {import("../../events/Event.js").default} event Event. + * @inheritDoc */ - handleFontsChanged_(event) { + handleFontsChanged() { const layer = this.getLayer(); if (layer.getVisible() && this.replayGroup_) { layer.changed(); diff --git a/src/ol/renderer/canvas/VectorTileLayer.js b/src/ol/renderer/canvas/VectorTileLayer.js index f1d1a3be35..66f3645452 100644 --- a/src/ol/renderer/canvas/VectorTileLayer.js +++ b/src/ol/renderer/canvas/VectorTileLayer.js @@ -5,12 +5,11 @@ import {getUid} from '../../util.js'; import {createCanvasContext2D} from '../../dom.js'; import TileState from '../../TileState.js'; import ViewHint from '../../ViewHint.js'; -import {listen, unlisten, unlistenByKey} from '../../events.js'; +import {listen, unlistenByKey} from '../../events.js'; import EventType from '../../events/EventType.js'; import {buffer, containsCoordinate, equals, getIntersection, intersects} from '../../extent.js'; import VectorTileRenderType from '../../layer/VectorTileRenderType.js'; import ReplayType from '../../render/canvas/BuilderType.js'; -import {labelCache} from '../../render/canvas.js'; import CanvasBuilderGroup from '../../render/canvas/BuilderGroup.js'; import CanvasTileLayerRenderer from './TileLayer.js'; import {getSquaredTolerance as getSquaredRenderTolerance, renderFeature} from '../vector.js'; @@ -132,16 +131,12 @@ class CanvasVectorTileLayerRenderer extends CanvasTileLayerRenderer { // Use nearest lower resolution. this.zDirection = 1; - - listen(labelCache, EventType.CLEAR, this.handleFontsChanged_, this); - } /** * @inheritDoc */ disposeInternal() { - unlisten(labelCache, EventType.CLEAR, this.handleFontsChanged_, this); this.overlayContext_.canvas.width = this.overlayContext_.canvas.height = 0; super.disposeInternal(); } @@ -363,9 +358,9 @@ class CanvasVectorTileLayerRenderer extends CanvasTileLayerRenderer { } /** - * @param {import("../../events/Event.js").default} event Event. + * @inheritDoc */ - handleFontsChanged_(event) { + handleFontsChanged() { const layer = this.getLayer(); if (layer.getVisible() && this.renderedLayerRevision_ !== undefined) { layer.changed(); diff --git a/test/spec/ol/renderer/canvas/tilelayer.test.js b/test/spec/ol/renderer/canvas/tilelayer.test.js index 0df07563a5..a24dc6f622 100644 --- a/test/spec/ol/renderer/canvas/tilelayer.test.js +++ b/test/spec/ol/renderer/canvas/tilelayer.test.js @@ -49,7 +49,7 @@ describe('ol.renderer.canvas.TileLayer', function() { }); source.updateParams({TIME: '1'}); map.renderSync(); - const tiles = map.getRenderer().getLayerRenderer(layer).renderedTiles; + const tiles = layer.getRenderer().renderedTiles; expect(tiles.length).to.be(1); expect(tiles[0]).to.equal(tile); expect(tile.inTransition()).to.be(true); diff --git a/test/spec/ol/renderer/canvas/vectorlayer.test.js b/test/spec/ol/renderer/canvas/vectorlayer.test.js index b34d91ead7..ea591ddc9c 100644 --- a/test/spec/ol/renderer/canvas/vectorlayer.test.js +++ b/test/spec/ol/renderer/canvas/vectorlayer.test.js @@ -75,7 +75,7 @@ describe('ol.renderer.canvas.VectorLayer', function() { style: layerStyle }); map.addLayer(layer); - const spy = sinon.spy(map.getRenderer().getLayerRenderer(layer), + const spy = sinon.spy(layer.getRenderer(), 'renderFeature'); map.renderSync(); expect(spy.getCall(0).args[3]).to.be(layerStyle); diff --git a/test/spec/ol/renderer/canvas/vectortilelayer.test.js b/test/spec/ol/renderer/canvas/vectortilelayer.test.js index 5bed2e1d78..af8d06bc17 100644 --- a/test/spec/ol/renderer/canvas/vectortilelayer.test.js +++ b/test/spec/ol/renderer/canvas/vectortilelayer.test.js @@ -140,7 +140,7 @@ describe('ol.renderer.canvas.VectorTileLayer', function() { }); it('gives precedence to feature styles over layer styles', function() { - const spy = sinon.spy(map.getRenderer().getLayerRenderer(layer), + const spy = sinon.spy(layer.getRenderer(), 'renderFeature'); map.renderSync(); expect(spy.getCall(0).args[2]).to.be(layer.getStyle());