PluggableMap: avoid crash when multiple interactions are removed

[megawac]

As shown in this codepen it is possible to cause a crash when handling a map browser event. The way to achieve this error is to remove multiple interactions are part of one of the map browser event handlers. Due to the way the for loop is structure, we changes in the interactionsArray can cause inconsistencies.

In my case I ran into this issue when using a draw event -- when the draw was completed the interaction would remove itself and a snap interaction from the map. As the draw was the last interaction in the interactionsArray its drawend handler was called first by PluggableMap, then my event handler removed 2 interactions, the handleMapBrowserEvent continue to the index that had the snap provider which caused interaction = undefined and the following error to occur.

PluggableMap.js:353 Uncaught TypeError: Cannot read property 'getActive' of undefined
    at Map.PluggableMap.handleMapBrowserEvent (PluggableMap.js:353)
    at MapBrowserEventHandler.Target.dispatchEvent (Target.js:71)
    at MapBrowserEventHandler.handlePointerUp_ (MapBrowserEventHandler.js:88)

Can you provide pointers of where the unit test for this should be added?

An alternative fix which may be cleaner may be to slice the interactionsArray prior to looping over it in order to guarantee the collection doesn't change during the event handler.

[mike-000]

I think it has to be done using slice. Working through a collection's array as it is being updated may mean an active interaction is moved up the array and you miss it. e.g. looping through the getLayers() array calling removeLayer only removes half the layers.

[megawac]

Sure, I'm happy to swap that. I was leaning towards that approach anyway.

[megawac]

Just checking is there a reason interactionArray isnt built in this if condition?

[ahocevar]

Doing slice() on an array on this hot path puts a lot of pressure on the garbage collector. So

That said, the issue could also be easily solved on the application level, by removing the interactions in the next tick. I've done something similar before:

  draw.on("drawend", () => {
    setTimeout(() => {
      map.getInteractions().getArray().slice().forEach(i => map.removeInteraction(i));
    }, 0);
  });

[mike-000]

Yes, I agree it is more an applicable issue, it's not a good idea for an interaction to remove itself directly within its own event.

(Also to make sure all interactions were removed would you need map.getInteractions().getArray().slice().forEach( as with layers?)

[megawac]

Doing slice() on an array on this hot path puts a lot of pressure on the garbage collector.

Yeah, that's why the first implementation was changing the if condition as below to avoid creating any performance differences

-        if (!interaction.getActive()) {
+        if (!interaction || !interaction.getActive()) {
           continue;
         }

(Also to make sure all interactions were removed would you need map.getInteractions().getArray().slice().forEach( as with layers?)

That wouldn't be necessary as the sliced array won't be affected by interactions being removed. That said I'm not entirely sure what happens when you remove an interaction from the map and then attempt to call interaction.handleEvent()

That said, the issue could also be easily solved on the application level, by removing the interactions in the next tick. I've done something similar before:

That solution actually didn't work in my application when I tried it (after some minor changes I got this workaround working). The solution I was hoping would work was to simply do this

  draw.on("drawend", () => {
    map.getInteractions().getArray().slice().forEach(i => map.removeInteraction(i)); // or whatever
    return false; // Avoid continue propogating the event in PluggableMap
  });

[ahocevar]

@megawac In your application, it would be better to use setActive(false) and setActive(true) on interactions, instead of removing and re-adding them.

The first version of your pull request had the problem that it might result in an interaction's handleEvent() getting called more than once.

[ahocevar]

An acceptable change I could think of would be to change removeInteraction (and maybe also addInteraction) to remove the interaction in the next tick. Something like this:

diff --git a/src/ol/PluggableMap.js b/src/ol/PluggableMap.js
index b5818bd71..d2c5f0e06 100644
--- a/src/ol/PluggableMap.js
+++ b/src/ol/PluggableMap.js
@@ -156,6 +156,12 @@ class PluggableMap extends BaseObject {
     /** @private */
     this.boundHandleBrowserEvent_ = this.handleBrowserEvent.bind(this);
 
+    /**
+     * @type {boolean}
+     * @private
+     */
+    this.handlingMapBrowserEvent_ = false;
+
     /**
      * @type {number}
      * @private
@@ -1022,6 +1028,7 @@ class PluggableMap extends BaseObject {
     mapBrowserEvent.frameState = this.frameState_;
     const interactionsArray = this.getInteractions().getArray();
     if (this.dispatchEvent(mapBrowserEvent) !== false) {
+      this.handlingMapBrowserEvent_ = true;
       for (let i = interactionsArray.length - 1; i >= 0; i--) {
         const interaction = interactionsArray[i];
         if (!interaction.getActive()) {
@@ -1032,6 +1039,7 @@ class PluggableMap extends BaseObject {
           break;
         }
       }
+      this.handlingMapBrowserEvent_ = false;
     }
   }
 
@@ -1329,7 +1337,18 @@ class PluggableMap extends BaseObject {
    * @api
    */
   removeInteraction(interaction) {
-    return this.getInteractions().remove(interaction);
+    const interactions = this.getInteractions();
+    const foundInteraction = interactions.getArray().indexOf(interaction) !== -1;
+    if (this.handlingMapBrowserEvent_) {
+      setTimeout(
+        function () {
+          interactions.remove(interaction);
+        }.bind(this)
+      );
+    } else {
+      interactions.remove(interaction);
+    }
+    return foundInteraction ? interaction : undefined;
   }
 
   /** 

[megawac]

There is a slight chance that if the same isn't done for addInteraction as well there could be minor inconsistencies from the current behaviour. Honestly I don't think anyone would ever do this but this could break :)

draw.on("drawend", () => {
  map.removeInteraction(interaction);
  map.addInteraction(interaction); 
});

[ahocevar]

You're right @megawac, addInteraction() should get the same protection.

[ahocevar]

However, it might be better to refactor all interactions to not fire events during MapBrowserEvent handling. This would make the behavior much more predictable and avoid unnecessary setTimeout() calls elsewhere.

[ahocevar]

I still think what I suggested in #11305 (comment) would be a safer solution.

[megawac]

I agree, I would need to look into it further to take a crack at such an implementation. Regarding unit testing for this, in which suite would it be best to implement a regression test for either of these changes?

[ahocevar]

@megawac I would say in the suite for the respective interaction. Thanks for willing to address this!

[ahocevar]

@megawac I just took a closer look at this, and I think firing interaction events in the next tick when they occur in a map browser event handler will be hard to achieve because at least for Modify and Draw, all the geometry modifications would then be in a different sequence with the events.

That said, I'm inclined to accept your solution here.