Fix for a race condition in the usage of URLCache (#4821)

Author: maru-biz

CHANGELOG.md

@@ -10,13 +10,15 @@
 
 * Added `RouteControllerNotificationUserInfoKey.shouldPlayRerouteSoundKey` to the user info dictionary of `Notification.Name.routeControllerDidReroute` notification. ([#4822](https://github.com/mapbox/mapbox-navigation-ios/pull/4822))
 * Fixed a bug with excessive `VisualInstructionDelegate.label(_:willPresent:as:)` delegate method call during initialization.
+* Fixed a randomly occuring race condition related to the usage of `URLCache` that could cause a crash.
 
 ## v2.20.4
 
 ### Other changes
 
 * Reroute notification sound now plays only after a new route is successfully retrieved.
 * Fixed a bug with excessive `VisualInstructionDelegate.label(_:willPresent:as:)` delegate method call during initialization.
+* Fixed a randomly occuring race condition related to the usage of `URLCache` that could cause a crash.
 
 ## v2.20.3
 

Sources/MapboxNavigation/Cache.swift

@@ -38,20 +38,29 @@ protocol URLCaching {
  A general purpose URLCache used by `SpriteRepository` implementations.
  */
 internal class URLDataCache: URLCaching {
-    let defaultDiskCacheURL: URL = {
+    private static var defaultDiskCacheURL: URL {
         let fileManager = FileManager.default
         let basePath = fileManager.urls(for: .cachesDirectory, in: .userDomainMask).first!
         let identifier = Bundle.mapboxNavigation.bundleIdentifier!
         return basePath.appendingPathComponent(identifier).appendingPathComponent("URLDataCache")
-    }()
+    }
+    
+    private let urlCache: URLCache
+    private static let defaultCapacity = 5 * 1024 * 1024
+    private let cacheLock = NSLock()
+    
+    var currentMemoryUsage: Int {
+        urlCache.currentMemoryUsage
+    }
 
-    let urlCache: URLCache
-    let defaultCapacity = 5 * 1024 * 1024
+    var currentDiskUsage: Int {
+        urlCache.currentDiskUsage
+    }
 
     init(memoryCapacity: Int? = nil, diskCapacity: Int? = nil, diskCacheURL: URL? = nil) {
-        let memoryCapacity = memoryCapacity ?? defaultCapacity
-        let diskCapacity = diskCapacity ?? defaultCapacity
-        let diskCacheURL = diskCacheURL ?? defaultDiskCacheURL
+        let memoryCapacity = memoryCapacity ?? Self.defaultCapacity
+        let diskCapacity = diskCapacity ?? Self.defaultCapacity
+        let diskCacheURL = diskCacheURL ?? Self.defaultDiskCacheURL
         if #available(iOS 13.0, *) {
             urlCache = URLCache(memoryCapacity: memoryCapacity, diskCapacity: diskCapacity, directory: diskCacheURL)
         } else {
@@ -60,18 +69,34 @@ internal class URLDataCache: URLCaching {
     }
 
     func store(_ cachedResponse: CachedURLResponse, for url: URL) {
+        cacheLock.lock()
+        defer {
+            cacheLock.unlock()
+        }
         urlCache.storeCachedResponse(cachedResponse, for: URLRequest(url))
     }
 
     func response(for url: URL) -> CachedURLResponse? {
+        cacheLock.lock()
+        defer {
+            cacheLock.unlock()
+        }
         return urlCache.cachedResponse(for: URLRequest(url))
     }
 
     func clearCache() {
+        cacheLock.lock()
+        defer {
+            cacheLock.unlock()
+        }
         urlCache.removeAllCachedResponses()
     }
 
     func removeCache(for url: URL) {
+        cacheLock.lock()
+        defer {
+            cacheLock.unlock()
+        }
         urlCache.removeCachedResponse(for: URLRequest(url))
     }
 }

Sources/MapboxNavigation/NavigationViewController.swift

@@ -867,10 +867,6 @@ open class NavigationViewController: UIViewController, NavigationStatusPresenter
         styleManager = StyleManager()
         styleManager.delegate = self
         styleManager.styles = navigationOptions?.styles ?? [DayStyle(), NightStyle()]
-        
-        if let currentStyle = styleManager.currentStyle {
-            updateMapStyle(currentStyle)
-        }
     }
 
     var currentStatusBarStyle: UIStatusBarStyle = .default

Tests/MapboxNavigationTests/URLDataCacheTests.swift

@@ -5,13 +5,25 @@ import TestHelper
 class URLDataCacheTest: TestCase {
     let url = ShieldImage.i280.baseURL
     var cache: URLDataCache!
+    
+    private static var cacheURL: URL {
+        let fileManager = FileManager.default
+        let basePath = fileManager.urls(for: .cachesDirectory, in: .userDomainMask).first!
+        let identifier = Bundle.main.bundleIdentifier!
+        return basePath.appendingPathComponent(identifier).appendingPathComponent("TestURLDataCache")
+    }
 
     override func setUp() {
         super.setUp()
 
         self.continueAfterFailure = false
-        cache = URLDataCache()
-        cache.urlCache.diskCapacity = 0
+        cache = URLDataCache(diskCapacity: 0, diskCacheURL: Self.cacheURL)
+        cache.clearCache()
+    }
+    
+    override func tearDown() {
+        cache.clearCache()
+        super.tearDown()
     }
 
     private func exampleResponse(with storagePolicy: URLCache.StoragePolicy) -> CachedURLResponse {
@@ -39,7 +51,7 @@ class URLDataCacheTest: TestCase {
 
         cache.clearCache()
         XCTAssertNil(cache.response(for: url)?.data)
-        XCTAssertEqual(cache.urlCache.currentMemoryUsage, 0)
+        XCTAssertEqual(cache.currentMemoryUsage, 0)
     }
 
     func testRemoveRequestCache() {
@@ -59,7 +71,7 @@ class URLDataCacheTest: TestCase {
 
         let cachedResponse = cache.response(for: url)
         XCTAssertEqual(cachedResponse, response)
-        XCTAssertEqual(cache.urlCache.currentMemoryUsage, response.data.count)
+        XCTAssertEqual(cache.currentMemoryUsage, response.data.count)
     }
 
     func testStoreCacheWithMemoryWarning() {
@@ -76,15 +88,19 @@ class URLDataCacheTest: TestCase {
         let response = exampleResponse(with: .allowedInMemoryOnly)
 
         let limitCapacity = 1
-        let limitCache = URLDataCache(memoryCapacity: limitCapacity, diskCapacity: limitCapacity)
         XCTAssertTrue(response.data.count > limitCapacity)
 
+        let limitCache = URLDataCache(
+            memoryCapacity: limitCapacity,
+            diskCapacity: limitCapacity,
+            diskCacheURL: Self.cacheURL
+        )
+        limitCache.clearCache()
+        
         limitCache.store(response, for: url)
         XCTAssertNil(cache.response(for: url))
-        XCTAssertEqual(limitCache.urlCache.currentMemoryUsage, 0)
-        
-        limitCache.urlCache.diskCapacity = 0
-        limitCache.clearCache()
+        XCTAssertEqual(limitCache.currentMemoryUsage, 0)
+        // not checking disk usage as it is always non-zero
     }
 }