Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix retain cycle causing crash in RouteOverlayController on NavigationMapView reuse #3222

Merged
merged 2 commits into from Aug 4, 2021

Conversation

irtemed88
Copy link
Contributor

@irtemed88 irtemed88 commented Aug 4, 2021

Description

This PR fixes a crash due to a retain cycle caused as part of NavigationMapView reuse across multiple instances of NavigationViewController which was made available with the injection work in #3186.

  • Updated Changelog

Implementation

RouteOverlayController.navigationViewDidLoad defines two closures for handling map events. Each closure captures a strong reference to self which will result in a retain cycle if the referenced NavigationMapView object is used across multiple instances of NavigationViewController. The crash occurs on presentation of the subsequent NavigationViewController

Steps to reproduce this bug are:

  1. Create an instance of NavigationMapView
  2. Create and present an instance of NavigationViewController and inject the map view created in step 1.
  3. Dismiss the instance of NavigationViewController ensuring the map view is still retained.
  4. Create and present a second instance of NavigationViewController and inject the map view created in step 1.
  5. Observe crash in RouteOverlayController.showRouteIfNeeded when RouteOverlayController.navigationViewData (which is implicitly unwrapped) is accessed. The crashing controller instance will be the one created by NavigationViewController in step 2.

Using weak self references breaks the retain cycle allowing RouteOverlayController to deinit when the owning NavigationViewController is deinit.

It's unclear to me at this point if the handlers added to navigationMapView.mapView.mapboxMap also need to be cleaned, but in the interim I believe this fix should address the more immediate crash.

@irtemed88
Copy link
Contributor Author

Looks like CI failed for some reason? Is this something I need to address before merging?

cc @MaximAlien

navigationMapView.mapView.showsTraffic = false
navigationMapView.mapView.mapboxMap.onNext(.styleLoaded) { [weak self] _ in
self?.navigationMapView.localizeLabels()
self?.navigationMapView.mapView.showsTraffic = false
Copy link
Member

Choose a reason for hiding this comment

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

In theory self can be deallocated between these lines, so as a rule of thumb I always add guard let self = self else { return } if self is used more than once.
Not sure if it's critical in this case.

@MaximAlien
Copy link
Contributor

Looks like CI failed for some reason? Is this something I need to address before merging?

cc @MaximAlien

Looks like it's temporary glitch on CircleCI. I've restarted failed jobs. Besides this everything looks good.

@irtemed88 irtemed88 merged commit 5101cb2 into main Aug 4, 2021
@irtemed88 irtemed88 deleted the demetri/route-controller-retain-cycle branch August 4, 2021 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants