Merge pull request #9561 from ahocevar/map-memory-leak

Remove memory leak caused by label cache listeners

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<import("./Overlay.js").default>}
      * @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

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.

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);

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.

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<string, import("./Layer.js").default>}
-     */
-    this.layerRenderers_ = {};
-
-    /**
-     * @private
-     * @type {Object<string, import("../events.js").EventsKey>}
-     */
-    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<string, import("./Layer.js").default>} 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<import("../layer/Layer.js").State>} layerStatesArray Layer states array.
- * @return {Object<string, import("../layer/Layer.js").State>} States mapped by layer uid.
- */
-function getLayerStatesMap(layerStatesArray) {
-  return layerStatesArray.reduce(function(acc, state) {
-    acc[getUid(state.layer)] = state;
-    return acc;
-  }, {});
-}
-
 export default MapRenderer;

src/ol/renderer/canvas/VectorImageLayer.js

@@ -53,6 +53,13 @@ class CanvasVectorImageLayerRenderer extends CanvasImageLayerRenderer {
     super.disposeInternal();
   }
 
+  /**
+   * @inheritDoc
+   */
+  handleFontsChanged() {
+    this.vectorRenderer_.handleFontsChanged();
+  }
+
   /**
    * @inheritDoc
    */

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();

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();

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);

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);

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());