Event.removeEventListener removes listeners incorrectly

[speigg]

When removing multiple listeners from an instance ofEvent, while the event is being raised, the listeners are not removed correctly.

See:

var event = new Cesium.Event();

// add a bunch of listeners, but the even ones should 
// remove themselves the first time they are called. 
['A','B','C','D','E','F','G'].forEach((value, index) => {
	var remove = e.addEventListener(() => { 
                var isEven = index % 2 === 0;
                console.log(value, index, isEven ? '--removing this one!' : ''); 
                if (isEven) remove(); 
        });
});

// raise the event!
e.raiseEvent(); 

Outputs the following (as expected):

A 0 --removing this one!
B 1
C 2 --removing this one!
D 3
E 4 --removing this one!
F 5
G 6 --removing this one!

But when calling e.raiseEvent() again, we see:

B 1
F 5

instead of the expected:

B 1
D 3
F 5

[speigg]

Why is this happening?

1. When an event is actively being raised, removeEventListener stores the index of the listener in the internal _listeners array, to be removed later (when raiseEvent is next called).
2. When raiseEvent is next called, it checks the list of indexes to remove, and then proceeds to remove them one-by-one by calling splice(index, 1) on the internal _listeners array... This is a problem! Since we are mutating the array while we are iterating through it, our indexes lose their original meaning after the first removal!

Possible Solutions:

1. Sort the list of indexes to remove, and then splice the array at those indexes by calling splice(index,1) in reverse order of the indexes to be removed. This way, the indexes are not affected by the mutations we make while removing items from the list. The downside is we have to sort the array of indexes to be removed.

2. Rather than storing indexes, we can store the actual listener that needs to be removed, and then iterate through these listeners when it's time to remove them (in raiseEvent), by finding and removing each one from the _listeners array individually (via a call to indexOf followed by splice). This is precisely how the EventDispatcher in threejs works. The downside is we have to search the _listeners array for each listener in our remove list (via indexOf).

Either solution is trivial, and I'm happy to make a PR, but I'm not sure which one would be preferred. It seems like it comes down to a choice between sorting or searching (though either of these operations would only occur when there are listeners pending to be removed, which only happens if removeEventListener is called while an event is actively being raised—so the performance implications should be negligible either way). Please advise on which solution is better (if any), and I will submit a PR.

[ggetz]

Thanks for investigating this @speigg !

I think Solution # 1 would probably be the fix with the smallest impact, but I'm sure you will get more opinions when you actually open you PR. 😏

[ggetz]

Reported on the forum https://groups.google.com/forum/#!searchin/cesium-dev/dynamic$20grid$20lines%7Csort:relevance/cesium-dev/WgPxGHPWhBs/gdQXrgxHAgAJ

[hpinkos]

Fixed in #5833

[cesium-concierge]

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!searchin/cesium-dev/dynamic$20grid$20lines%7Csort

If this issue affects any of these threads, please post a comment like the following:

The issue at #5827 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

I am a bot who helps you make Cesium awesome! Thanks again.