Crash presenting generic shield in SwiftUI — unwrapped nil in InstructionPresenter.swift

[zacharyblank]

I'm using version 0.38.1. I'm in Portland Oregon and certain routes always cause a crash with the error:

Thread 1: Fatal error: Unexpectedly found nil while unwrapping an Optional value
in InstructionPresenter.swift on line 236.

The coordinates I use that for sure give me a crash are:

from: 45.5476633, -122.6624749
to: 45.5189373, -122.6771381

I assume there is something in the JSON response for one of the instructions along this route and other routes in Portland that include the same instruction but I cannot hunt it down. Please help.

Most other routes work just fine.

Thank you!

[1ec5]

@zacharyblank, are you developing the application in SwiftUI, by any chance?

The line that’s crashing was originally introduced in #1272:

mapbox-navigation-ios/MapboxNavigation/InstructionPresenter.swift

Line 236 in 965012c

window = UIApplication.shared.delegate!.window!!

The containing method, InstructionPresenter.takeSnapshot(on:), executes whenever a route number or exit number appears in the turn banner, unless there’s a shield like available to represent the route number. It needs to temporarily add a view to a window’s view hierarchy in order for the UIAppearance system to match the view’s appearance to the current style.

The crash means either the application has no application delegate (unlikely) or the application delegate has no window. With SwiftUI, the scene delegate holds onto the window instead of the application delegate.

A short-term fix would be for takeSnapshot(on:) to return nil when there’s no window. That will cause InstructionPresenter to fall back on displaying the component as plain, unadorned text, which would be far preferable to crashing. Or it could snapshot the view without adding it to the view hierarchy, which would preserve the appearance but could make it indiscernible in a night style.

As for a better fix that preserves the capsule-like appearance, I don’t see a straightforward way to get the current window through the current scene, as we can through UIApplication with a conventional UI. Perhaps it wouldn’t be too weird to crawl up the responder chain looking for a window. But that would still require access to a view that’s already in the window’s view hierarchy.

InstructionPresenter converts a visual instruction into an attributed string and respond to image downloads by replacing them in the string. As a standalone class, it’s designed to abstract away that job from any views. The fact that it internally reaches right out to the UIApplication’s main window demonstrates that this abstraction isn’t particularly useful. Moreover, InstructionPresenter is only used by InstructionLabel.instruction’s getter, which does have access to the view hierarchy.

#2275 already rewrote a large portion of InstructionPresenter to make it more intuitive, portable, and reusable. I think we should eliminate InstructionPresenter and split its functionality between extensions on VisualInstruction.Component and InstructionLabel. The portion moved into extensions will be easier to unit-test than what’s currently in InstructionPresenter, for which we currently rely on snapshot tests.

/cc @mapbox/navigation-ios

[zacharyblank]

That is 100% right. SwiftUI. Thanks for the detailed explanation. It makes sense. I can fork and do the quick fix as you recommended and/or I can wait for the better fix as you described depending on timing. What is the update cycle like?

Thanks again!

[1ec5]

A PR with the quick fix would be great. We’re currently in the middle of the v1.0 release cycle, which will take a while longer. We just put out a first alpha and are planning to put out a second alpha in the next week or two. In the meantime, you’re welcome to have your project depend on the master branch if that would unblock your development efforts. We might also consider cherry-picking the fix into the release-v0.38 branch and releasing v0.38.3 with it if we need the fix to go into a stable release sooner.

[zacharyblank]

@1ec5 I'm afraid that, in fact, didn't seem to be the issue. I simply returned nil inside of takeSnapshot as a test to completely remove reference to the window however I get the same error and I cannot seem to hunt down the root cause. Any chance you can take a look?

[1ec5]

I’m able to reproduce this issue with the following code:

ContentView.swift

import SwiftUI
import MapboxDirections
import MapboxCoreNavigation
import MapboxNavigation
import Combine

class RouteCalculator: ObservableObject {
    @Published var route: Route?
    
    func calculate(_ options: RouteOptions) {
        Directions.shared.calculate(options) { [weak self] (waypoints, routes, error) in
            self?.route = routes?.first
        }
    }
}

struct NavigationView: UIViewControllerRepresentable {
    typealias UIViewControllerType = NavigationViewController
    
    @Environment(\.presentationMode) var presentationMode
    var route: Route
    
    class Coordinator: NavigationViewControllerDelegate {
        var parent: NavigationView
        
        init(_ parent: NavigationView) {
            self.parent = parent
        }
        
        func navigationViewControllerDidDismiss(_ navigationViewController: NavigationViewController, byCanceling canceled: Bool) {
            parent.presentationMode.wrappedValue.dismiss()
        }
    }
    
    func makeCoordinator() -> NavigationView.Coordinator {
        return Coordinator(self)
    }
    
    func makeUIViewController(context: UIViewControllerRepresentableContext<NavigationView>) -> NavigationViewController {
        let viewController = NavigationViewController(for: route)
        viewController.modalPresentationStyle = .fullScreen
        viewController.delegate = context.coordinator
        return viewController
    }
    
    func updateUIViewController(_ uiViewController: NavigationViewController, context: UIViewControllerRepresentableContext<NavigationView>) {
        
    }
}

struct ContentView: View {
    @ObservedObject private var routeCalculator = RouteCalculator()
    
    init() {
        let origin = Waypoint(coordinate: CLLocationCoordinate2D(latitude: 45.5476633, longitude: -122.6624749))
        let destination = Waypoint(coordinate: CLLocationCoordinate2D(latitude: 45.5189373, longitude: -122.6771381))

        let options = NavigationRouteOptions(waypoints: [origin, destination])

        routeCalculator.calculate(options)
    }
    
    var body: some View {
        if let route = routeCalculator.route {
            return AnyView(NavigationView(route: route))
        } else {
            return AnyView(Text("Loading…"))
        }
    }
}

struct ContentView_Previews: PreviewProvider {
    static var previews: some View {
        ContentView()
    }
}

The issue reproduces on launch but only with these specific coordinates; I don’t reproduce the issue simulating a route in other cities.

[1ec5]

I’m guessing this comes down to a race condition in which the first visual instruction (which includes a shield) appears before the window has finished loading. At this point, UIApplication.delegate is non-nil, but indeed the window is nil. I’m surprised you’re still getting the same crash after returning nil in takeSnapshot, though I wouldn’t’ve been surprised about a downstream error – quite a bit of the view controller code depends on appearing after the parent view controller appears, not just after it finishes loading.

[1ec5]

In fact, NavigationViewController.viewDidLoad() explicitly causes the first visual instruction to be shown after loading but before appearing, as of 655a153 in #2121:

mapbox-navigation-ios/MapboxNavigation/NavigationViewController.swift

Lines 283 to 286 in d70618e

	guard let firstInstruction = navigationService.routeProgress.currentLegProgress.currentStepProgress.currentVisualInstruction else {
	return
	}
	navigationService(navigationService, didPassVisualInstructionPoint: firstInstruction, routeProgress: navigationService.routeProgress)

/cc @JThramer