Merge pull request #3528 from fredj/layer-group-leak

Fix memory leak when removing layers from ol.layer.Group

src/ol/layer/layergroup.js

@@ -48,9 +48,15 @@ ol.layer.Group = function(opt_options) {
 
   /**
    * @private
-   * @type {Object.<string, goog.events.Key>}
+   * @type {Array.<goog.events.Key>}
    */
-  this.listenerKeys_ = null;
+  this.layersListenerKeys_ = [];
+
+  /**
+   * @private
+   * @type {Object.<string, Array.<goog.events.Key>>}
+   */
+  this.listenerKeys_ = {};
 
   goog.events.listen(this,
       ol.Object.getChangeEventType(ol.layer.GroupProperty.LAYERS),
@@ -89,28 +95,31 @@ ol.layer.Group.prototype.handleLayerChange_ = function() {
  * @private
  */
 ol.layer.Group.prototype.handleLayersChanged_ = function(event) {
-  if (!goog.isNull(this.listenerKeys_)) {
+  goog.array.forEach(this.layersListenerKeys_, goog.events.unlistenByKey);
-    goog.array.forEach(
+  this.layersListenerKeys_.length = 0;
-        goog.object.getValues(this.listenerKeys_), goog.events.unlistenByKey);
-    this.listenerKeys_ = null;
-  }
 
   var layers = this.getLayers();
-  this.listenerKeys_ = {
+  this.layersListenerKeys_.push(
-    'add': goog.events.listen(layers, ol.CollectionEventType.ADD,
+      goog.events.listen(layers, ol.CollectionEventType.ADD,
-        this.handleLayersAdd_, false, this),
+          this.handleLayersAdd_, false, this),
-    'remove': goog.events.listen(layers, ol.CollectionEventType.REMOVE,
+      goog.events.listen(layers, ol.CollectionEventType.REMOVE,
-        this.handleLayersRemove_, false, this)
+          this.handleLayersRemove_, false, this));
-  };
+
+  goog.object.forEach(this.listenerKeys_, function(keys) {
+    goog.array.forEach(keys, goog.events.unlistenByKey);
+  });
+  goog.object.clear(this.listenerKeys_);
 
   var layersArray = layers.getArray();
   var i, ii, layer;
   for (i = 0, ii = layersArray.length; i < ii; i++) {
     layer = layersArray[i];
-    this.listenerKeys_[goog.getUid(layer).toString()] =
+    this.listenerKeys_[goog.getUid(layer).toString()] = [
-        goog.events.listen(layer,
+      goog.events.listen(layer, ol.ObjectEventType.PROPERTYCHANGE,
-            [ol.ObjectEventType.PROPERTYCHANGE, goog.events.EventType.CHANGE],
+          this.handleLayerChange_, false, this),
-            this.handleLayerChange_, false, this);
+      goog.events.listen(layer, goog.events.EventType.CHANGE,
+          this.handleLayerChange_, false, this)
+    ];
   }
 
   this.changed();
@@ -123,9 +132,15 @@ ol.layer.Group.prototype.handleLayersChanged_ = function(event) {
  */
 ol.layer.Group.prototype.handleLayersAdd_ = function(collectionEvent) {
   var layer = /** @type {ol.layer.Base} */ (collectionEvent.element);
-  this.listenerKeys_[goog.getUid(layer).toString()] = goog.events.listen(
+  var key = goog.getUid(layer).toString();
-      layer, [ol.ObjectEventType.PROPERTYCHANGE, goog.events.EventType.CHANGE],
+  goog.asserts.assert(!(key in this.listenerKeys_),
-      this.handleLayerChange_, false, this);
+      'listeners already registered');
+  this.listenerKeys_[key] = [
+    goog.events.listen(layer, ol.ObjectEventType.PROPERTYCHANGE,
+        this.handleLayerChange_, false, this),
+    goog.events.listen(layer, goog.events.EventType.CHANGE,
+        this.handleLayerChange_, false, this)
+  ];
   this.changed();
 };
 
@@ -137,7 +152,8 @@ ol.layer.Group.prototype.handleLayersAdd_ = function(collectionEvent) {
 ol.layer.Group.prototype.handleLayersRemove_ = function(collectionEvent) {
   var layer = /** @type {ol.layer.Base} */ (collectionEvent.element);
   var key = goog.getUid(layer).toString();
-  goog.events.unlistenByKey(this.listenerKeys_[key]);
+  goog.asserts.assert(key in this.listenerKeys_, 'no listeners to unregister');
+  goog.array.forEach(this.listenerKeys_[key], goog.events.unlistenByKey);
   delete this.listenerKeys_[key];
   this.changed();
 };

test/spec/ol/layer/layergroup.test.js

@@ -326,6 +326,31 @@ describe('ol.layer.Group', function() {
 
   });
 
+  describe('layers events', function() {
+
+    it('listen / unlisten for layers added to the collection', function() {
+      var layers = new ol.Collection();
+      var layerGroup = new ol.layer.Group({
+        layers: layers
+      });
+      expect(goog.object.getCount(layerGroup.listenerKeys_)).to.eql(0);
+      var layer = new ol.layer.Layer({});
+      layers.push(layer);
+      expect(goog.object.getCount(layerGroup.listenerKeys_)).to.eql(1);
+
+      var listeners = layerGroup.listenerKeys_[goog.getUid(layer)];
+      expect(listeners.length).to.eql(2);
+      expect(listeners[0]).to.be.a(goog.events.Listener);
+      expect(listeners[1]).to.be.a(goog.events.Listener);
+
+      // remove the layer from the group
+      layers.pop();
+      expect(goog.object.getCount(layerGroup.listenerKeys_)).to.eql(0);
+      expect(listeners[0].removed).to.eql(true);
+      expect(listeners[1].removed).to.eql(true);
+    });
+
+  });
 
   describe('#setLayers', function() {
 
@@ -485,6 +510,7 @@ describe('ol.layer.Group', function() {
 
 goog.require('goog.dispose');
 goog.require('goog.events.EventType');
+goog.require('goog.events.Listener');
 goog.require('goog.object');
 goog.require('ol.ObjectEventType');
 goog.require('ol.extent');