lottie-web misbehaves if event listeners are removed during event processing

[dcwarwick]

Issue summary

I found myself writing some JavaScript code to wait for a particular frame to be passed. In order to do this, I installed an enterFrame listener, and in the listener checked to see if the frame had been passed. However, the animation will continue running long after this, and loops, and I didn't want to be continually receiving notifications or adding overhead long after the required trigger had happened, so when the frame had been passed I performed the required action AND I removed the listener to prevent it being called in the future. However, due to the way the event listeners are being called, this causes the next event listener of the same type to be skipped and a JavaScript error to occur when the end of the array is encountered prematurely.

What did you expect to happen?

An event listener calling mechanism should be robust to event listeners being added or removed during the triggering of an event.

Steps to recreate

Here is a CodePen that can illustrate the problem: https://codepen.io/dcwarwick/pen/MWmaWyG

When the animation is running, press the "Pause when next at start" button TWICE in succession. This installs two identical enterFrame event handlers each waiting for frame zero to occur. Now wait for the end of the loop. The animation will stop, and the following will be seen in the console:

The button calls this function:

demo1.pauseAt = frame => {
  const then = demo1.currentRawFrame;
  const listener = e => {
    if ((e.currentTime >= frame) && ((then < frame) || (then > e.currentTime))) {
      demo1.pause();
      demo1.removeEventListener('enterFrame', listener);
    }
  };
  demo1.addEventListener('enterFrame', listener);
}

This adds an enterFrame event listener which checks to see if a particular frame has just been reached, and if it has it (a) pauses the animation and (b) removes itself as an event listener.

The code in lottie-web looks something like this:

triggerEvent: function (eventName, args) {
    if (this._cbs[eventName]) {
      var len = this._cbs[eventName].length;
      for (var i = 0; i < len; i += 1) {
        this._cbs[eventName][i](args);
      }
    }
  },

The length of the array this._cbs[eventName] is captured before the loop starts, but not checked throughout. Thus, if the array is shortened during the loop execution, the above error occurs. Also, event listeners are skipped because of the way the array gets spliced by removeEventListener. NB if event listeners are added during the loop execution then they will not be called, but that is entirely to be expected and correct behaviour.

One way to fix this would be to capture the whole array, rather than just the length of it, before the loop starts. Eg:

triggerEvent: function (eventName, args) {
    if (this._cbs[eventName]) {
      var cbs = this._cbs[eventName].slice();
      var len = cbs.length;
      for (var i = 0; i < len; i += 1) {
        cbs[i](args);
      }
    }
  },

This would ensure that the triggering of an event remains robust to changes to the event listener list while the callback functions are being called.

Workarounds

The problem can be worked around by deferring the removal of the event handler until after the callback has finished. In my case I simply put the removing of the event handler into a setTimeout.

[dcwarwick]

PS I'd also be interested to know if I was going about this the best way. I want to tell an animation to keep playing UNTIL it reaches a particular frame and THEN pause, and couldn't see anything in the current API to help with that. So the above code adds a pauseAt method onto the animation, so I can go demo1.pauseAt(58) and it will pause when the animation next reaches frame 58, taking account of the fact we might already be past frame 58 but looping. Are there other ways I could do this? Do let me know if my whole approach is daft! This issue is still valid, I believe, though, in any case.

[bodymovin]

hi, there is a PR related to this, but all approaches have some trade-offs. https://github.com/airbnb/lottie-web/pull/2291 I do think that your suggested solution is better than the existing one, but it will also "break" by executing a removed listener in some scenarios. Regarding of whether this is the best approach for what you need, could you use "playSegments()" in your scenario? https://github.com/airbnb/lottie-web#playsegmentssegments-forceflag

[dcwarwick]

Oh yes, so there is! -- and it proposes exactly the same code fix :-) Sorry, I looked for existing issues but didn't look for existing PRs.

I suggest that it is "philosophically" preferable to snapshot the array before looping through it and calling the listeners, because that way the listeners that are registered WHEN THE EVENT IS TRIGGERED all get called. If a listener is removed during processing of the event, and it's one that hasn't yet been called, it will still be called. If a listener is added during processing of the event, it will not be called for this event. If your user was hoping that removing the listener would prevent it being called, they can easily refactor their code once they discover this is not how it works. On the other hand, it does mean cloning the array of listeners on every frame (in the case of enterFrame events), and I can see one might want to avoid that.

btw, in my scenario I don't really care whether the removed listener still gets called or does not get called -- I can code around it whichever way it works. A solution that doesn't lead to the JavaScript error is the key, I think! If a brilliant alternative occurs to me I'll suggest it.

[dcwarwick]

I did think of using playSegments. I was thinking that if animation.currentRawFrame < targetFrame I could do playSegments([animation.currentRawFrame, targetFrame]), and otherwise I could do playSegments([[animation.currentRawFrame, animation.totalFrames], [0, targetFrame]]) in order to reach the target frame on the next loop around. However, I found that when I called playSegments, it then started looping those segments. I would have to turn off looping, but then somehow turn looping back on after the animation eventually pauses, in order that the animation would loop again when it is next resumed. Also, I think I would need to clear the segments, so that when the animation is next resumed it continues looping the whole animation and not just start looping those segments again. It still seems a bit fiddly this way. Also, in my full scenario I need the pauseAt to be cancelable, and I wasn't sure how to undo the playSegments, whereas in the above case I simply remove the listener. Basically, I just want to set up an "advance pause" without otherwise affecting the animation setup and looping.

Out of interest, the full scenario I am trying to build is a looping animation that plays on a button during hover. When the mouse enters, I call play(), and at first I had it call pause() when the mouse leaves, but we then decided that it would be better if the animation finishes its current cycle when the mouse leaves rather than freezing abruptly in whatever mid-state it happens to be in. So this is why I found myself looking for a runTo or pauseAt kind of capability. And this is why it needs to be cancelable: in case the mouse leaves, but enters again before the animation has finished the current cycle.