Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Tests and fix for infinite loop when setting shape to nil in MGLMapViewDelegate methods #11614

Merged
merged 17 commits into from
May 21, 2018

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Apr 6, 2018

Addresses #11180, #5833.
Adds integration tests and work-around to stop crash.

@julianrex julianrex added iOS Mapbox Maps SDK for iOS tests ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl labels Apr 6, 2018
@@ -595,12 +595,16 @@ void Transform::startTransition(const CameraOptions& camera,
}
observer.onCameraIsChanging();
} else {
transitionFinishFn();
auto finish = transitionFinishFn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comments explaining why this code is structured this way?

@@ -595,12 +595,16 @@ void Transform::startTransition(const CameraOptions& camera,
}
observer.onCameraIsChanging();
} else {
transitionFinishFn();
auto finish = transitionFinishFn;

transitionFinishFn = nullptr;

// This callback gets destroyed here,
// we can only return after this point.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment was meant to serve as a reminder not to change this lambda such that any code executes "after this point". But this change does that. Is it safe? If so, please update this comment to explain why.

@julianrex
Copy link
Contributor Author

Bisecting and testing with @jmkiley's test case, it looks like this issue was introduced somewhere in:

39a732d...b6d56ad

/cc @ivovandongen

@julianrex julianrex force-pushed the jrex-11180-nil-shape-source branch from 153d26e to 3ce7934 Compare April 7, 2018 05:04
@ivovandongen
Copy link
Contributor

@julianrex Looking at the test case I wonder if this is an actual bug. If the change of a source is to trigger an update of the transitions and thus callback with onCameraDidChange(), this would be user error. Preventable by checking if the action has already been performed or clearing the callback when executing.

If the update of a source shouldn't trigger onCameraDidChange() in any case, we should address that problem at the root.

@julianrex
Copy link
Contributor Author

@jfirebaugh @ivovandongen What functionality should be disallowed in the various callbacks (e.g. onCameraDidChange)?

It sounds like solution should be a two part fix:

  1. The proposed solution to stop the infinite loop if it gets to that point, and
  2. raising an exception in client (iOS, Android) code for those operations that we should disallow?

What are you thoughts on: 1) could make it into the boba release, and 2) is tackled in a patch release.

/cc @tobrun as this may affect Android too.

@julianrex
Copy link
Contributor Author

julianrex commented Apr 10, 2018

Side-note: I've extracted the test changes into a new PR: #11649, and will update this PR when that has been merged.

@julianrex julianrex requested a review from 1ec5 April 10, 2018 17:23
@julianrex julianrex force-pushed the jrex-11180-nil-shape-source branch 2 times, most recently from ddc9748 to c75da65 Compare April 11, 2018 22:32
@julianrex
Copy link
Contributor Author

Pushed changes that I believe addresses #5833.

@julianrex julianrex removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 18, 2018
@1ec5 1ec5 added this to the ios-v4.0.1 milestone Apr 18, 2018
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix deserves a blurb in the iOS and macOS changelogs.

}

[NSObject cancelPreviousPerformRequestsWithTarget:expectation selector:@selector(fulfill) object:nil];
[expectation performSelector:@selector(fulfill) withObject:nil afterDelay:5.0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of time to leave a test hanging. Is there any way to avoid such a long delay? If this delayed fulfillment is intended to account for rotation animation, let’s use a KVO expectation that checks whether the direction has returned to 0°.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using this rather than KVO, so I can check the number of calls to this delegate method, as we were having an infinite recursion issue (and the later waitForExpectations:timeout catches this). I do agree that the time is too long - so perhaps there's an alternative here.

// after the temporary has been called) we stop this recursion.

auto transition = transitionFrameFn;
transitionFrameFn = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using std::move() to perform this swap, as well as below when swapping with finish.

@julianrex julianrex force-pushed the jrex-11180-nil-shape-source branch 3 times, most recently from 235f24f to 7ef5047 Compare April 26, 2018 18:28
@julianrex
Copy link
Contributor Author

Rebased.

@julianrex julianrex added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 26, 2018
@1ec5 1ec5 added this to the ios-v4.1.0 milestone May 15, 2018
if (finish) {
finish();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: } and else if ... on the same line.

finish();
}
}
else if (!transitionFrameFn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not -- I can imagine a situation where the camera change notification triggers the start of another transition, in which case transitionFrameFn would be legitimately non-null.

@julianrex
Copy link
Contributor Author

Rebasing due to release-boba merge.

@julianrex julianrex force-pushed the jrex-11180-nil-shape-source branch from 4343bc7 to 4849c49 Compare May 18, 2018 03:47
@julianrex julianrex removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label May 18, 2018
@julianrex julianrex self-assigned this May 18, 2018
@friedbunny
Copy link
Contributor

friedbunny commented May 18, 2018

@julianrex I think this needs to be retargeted at master? Update: I did this. 😁

@friedbunny friedbunny changed the base branch from release-boba to master May 18, 2018 18:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants