[ios,macos] Fix for infinite loop crash when setting shape to nil in MGLMapViewDelegate methods (#11614)

2 files changed, 49 insertions, 10 deletions

diff --git a/src/mbgl/map/transform.cpp b/src/mbgl/map/transform.cpp
index 105adf0400..da8e243d91 100644
--- a/src/mbgl/map/transform.cpp
+++ b/src/mbgl/map/transform.cpp
@@ -594,13 +594,10 @@ void Transform::startTransition(const CameraOptions& camera,
                 animation.transitionFrameFn(t);
             }
             observer.onCameraIsChanging();
+            return false;
         } else {
-            transitionFinishFn();
-            transitionFinishFn = nullptr;
-
-            // This callback gets destroyed here,
-            // we can only return after this point.
-            transitionFrameFn = nullptr;
+            // Indicate that we need to terminate this transition
+            return true;
         }
     };
 
@@ -615,7 +612,14 @@ void Transform::startTransition(const CameraOptions& camera,
     };
 
     if (!isAnimated) {
-        transitionFrameFn(Clock::now());
+        auto update = std::move(transitionFrameFn);
+        auto finish = std::move(transitionFinishFn);
+
+        transitionFrameFn = nullptr;
+        transitionFinishFn = nullptr;
+
+        update(Clock::now());
+        finish();
     }
 }
 
@@ -624,8 +628,43 @@ bool Transform::inTransition() const {
 }
 
 void Transform::updateTransitions(const TimePoint& now) {
-    if (transitionFrameFn) {
-        transitionFrameFn(now);
+
+    // Use a temporary function to ensure that the transitionFrameFn lambda is
+    // called only once per update.
+
+    // This addresses the symptoms of https://github.com/mapbox/mapbox-gl-native/issues/11180
+    // where setting a shape source to nil (or similar) in the `onCameraIsChanging`
+    // observer function causes `Map::Impl::onUpdate()` to be called which
+    // in turn calls this function (before the current iteration has completed),
+    // leading to an infinite loop. See https://github.com/mapbox/mapbox-gl-native/issues/5833
+    // for a similar, related, issue.
+    //
+    // By temporarily nulling the `transitionFrameFn` (and then restoring it
+    // after the temporary has been called) we stop this recursion.
+    //
+    // It's important to note that the scope of this change is stop the above
+    // crashes. It doesn't address any potential deeper issue (for example
+    // user error, how often and when transition callbacks are called).
+
+    auto transition = std::move(transitionFrameFn);
+    transitionFrameFn = nullptr;
+
+    if (transition && transition(now)) {
+        // If the transition indicates that it is complete, then we should call
+        // the finish lambda (going via a temporary as above)
+        auto finish = std::move(transitionFinishFn);
+
+        transitionFinishFn = nullptr;
+        transitionFrameFn = nullptr;
+
+        if (finish) {
+            finish();
+        }
+    } else if (!transitionFrameFn) {
+        // We have to check `transitionFrameFn` is nil here, since a new transition
+        // may have been triggered in a user callback (from the transition call
+        // above)
+        transitionFrameFn = std::move(transition);
     }
 }
 
diff --git a/src/mbgl/map/transform.hpp b/src/mbgl/map/transform.hpp
index d429c57661..bff44a2dcd 100644
--- a/src/mbgl/map/transform.hpp
+++ b/src/mbgl/map/transform.hpp
@@ -165,7 +165,7 @@ private:
 
     TimePoint transitionStart;
     Duration transitionDuration;
-    std::function<void(const TimePoint)> transitionFrameFn;
+    std::function<bool(const TimePoint)> transitionFrameFn;
     std::function<void()> transitionFinishFn;
 };