Transition loop and eventual crash at flyTo #5833
Comments
boundsj
added
the
navigation
|
The stack in #5833 (comment) strongly implicates Here’s a possible scenario that I suspect may lead to a race condition and eventual crash:
Similarly:
The
If any of these methods gets called just before a location update in targeted course tracking mode and a nontrivial completion handler is provided, we have everything we need for the recursion seen here. As part of #5983, @incanus is swapping some equality checks, which should help to avoid this scenario. |
|
Recreated the crash: https://gist.github.com/friedbunny/1a9faa1666d0446c7c81206fb96b52c0 I appreciate that Xcode doesn’t try to show tens of thousands of lines in the sidebar:
|
|
OK, so that validates the hypothesis above. To be sure, this particular case can be avoided by putting the call to resetNorth inside a dispatch_async block. Maybe there's a way for the regionDidChange mechanism to put developer- or user-initiated viewport changes inside a dispatch_async block if another animation is already ongoing. |
|
It's not clear to me why the SDK is calling |
|
I propose we:
|
|
I agree that we need more core discipline here. The |
These calls are partly cargo-cult programming, but they stem from #2313 and #3086, in which we were trying to serialize viewport changes and ensure the consistent notifications @incanus refers to above. (The Android SDK did something similar with #2296, and naturally the macOS SDK is canceling transitions too.) I’m all in favor of an approach where
If I’m not mistaken, the Android SDK relies on /cc @tobrun |
|
Can we test this again after #9381 got merged? |
|
Sure, we should double-check this crash. However, I don’t think the infinite recursion is related to #9381; |
|
As of 06a0c4a, this still recurses and crashes: |
|
FYI PR #11614 should address the infinite recursion stack crasher (I'm adding a test for this case). |
|
#11614 fixes the stack crasher. But I believe we still need to address the larger issue of how we handle chained transitions (and what happens about cancelling previous ones, and comparing new ones to existing ones). |
|
Closing, since #11614 addresses the crash. Any deeper refactoring is a longer-term issue. EDIT: /cc @pozdnyakov for visibility on the refactoring we have discussed recently. |
Mapbox SDK version: iOS v3.3.3 (release branch)
This is fairly silly, but @1ec5 suggested I post it because it represents 1. a crash that should not happen (regardless), and 2. this issue may be illustrative of a larger async/sync animation problem that exists in core.
Steps to trigger behavior
Try to reset the map to north when also using MGLUserTrackingModeFollowWithCourse.
Expected behavior
The map direction should oscillate between north and the current course.
Actual behavior
-[MGLMapView resetNorth]triggers a loop, wherein_setDirection:animated:callsMap::cancelTransitions()before starting to a new transition (to north). Eventually this finishes (after 70k steps) and_flyToCamerais called... which crashes:The text was updated successfully, but these errors were encountered: