Don't unlisten image twice when disposing an ol.ImageTile

[fredj]

Fixes #3325

[ahocevar]

Your change implies that #disposeInternal() might be called more than once, which makes me suspicious. Do we call it manually somewhere?

[fredj]

If the ol.ImageTile is disposed after the image has loaded, calling this.unlistenImage_() raises an error because of this check

[elemoine]

I have the same patch in one of branches. I think it is correct. We dispose of tiles in ol.TileCache#expireCache, and the cache may contain tiles that are still in the idle state.

[ahocevar]

I still think that the underlying issue is somewhere else. So if you want to merge this, please add a BIG FAT TODO reminding us to investigate this properly.

[elemoine]

I still think that the underlying issue is somewhere else.

I don't get it Andreas. disposeInternal calls unlistenImage_, which currently assumes that the image is in loading (or error) state. That assumption is wrong, as the image may be in idle or loaded state.

[fredj]

disposeInternal can't be called more that once; goog.Disposable#dispose is taking care of that.

The problem is that we may call unlistenImage_ more that once: in handleImageError_ / handleImageLoad_ and then in disposeInternal

[ahocevar]

If the above reasoning is true, then the following - more straightforward - version of disposeInternal should fix the issue too:

ol.ImageTile.prototype.disposeInternal = function() {
  if (this.state == ol.TileState.LOADING || this.state == ol.TileState.ERROR) {
    this.unlistenImage_();
  }
  goog.base(this, 'disposeInternal');
};

Am I right?

[elemoine]

Am I right?

Not quite, because handleImageError_ also calls unlistenImage_. So this should work:

ol.ImageTile.prototype.disposeInternal = function() {
  if (this.state == ol.TileState.LOADING) {
    this.unlistenImage_();
  }
  goog.base(this, 'disposeInternal');
};

[ahocevar]

👍, the above would make me happy.

[fredj]

Eric's proposal is more explicit, will change

[elemoine]

Another option would be to change the unlistenImage_ function itself.

Instead of

/**
 * Discards event handlers which listen for load completion or errors.
 *
 * @private
 */
ol.ImageTile.prototype.unlistenImage_ = function() {
  goog.asserts.assert(!goog.isNull(this.imageListenerKeys_));
  goog.array.forEach(this.imageListenerKeys_, goog.events.unlistenByKey);
  this.imageListenerKeys_ = null;
};

it would be:

/**
 * Discards event handlers which listen for load completion or errors.
 *
 * @private
 */
ol.ImageTile.prototype.unlistenImage_ = function() {
  if (!goog.isNull(this.imageListenerKeys_)) {
    goog.array.forEach(this.imageListenerKeys_, goog.events.unlistenByKey);
    this.imageListenerKeys_ = null;
  }
};

[ahocevar]

The previous proposal was more explicit. I always like to know the exact state from reading the code. Just checking whether there are still listeners registered does not look like an exact state to me.

[elemoine]

The previous proposal was more explicit. I always like to know the exact state from reading the code. Just checking whether there are still listeners registered does not look like an exact state to me.

I see what you mean!

[fredj]

Thanks for the review