[Core] Make instance manager conform to Swift 6 principles (#13933)

Author: ncooke3

FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift

@@ -51,20 +51,41 @@ final class HeartbeatStorage: HeartbeatStorageProtocol {
   // MARK: - Instance Management
 
   /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs.
-  private static var cachedInstances: [String: WeakContainer<HeartbeatStorage>] = [:]
+  #if compiler(>=6)
+    // In Swift 6, this property is not concurrency-safe because it is
+    // nonisolated global shared mutable state. Because this target's
+    // deployment version does not support Swift concurrency, it is marked as
+    // `nonisolated(unsafe)` to disable concurrency-safety checks. The
+    // property's access is protected by an external synchronization mechanism
+    // (see `instancesLock` property).
+    private nonisolated(unsafe) static var cachedInstances: [
+      String: WeakContainer<HeartbeatStorage>
+    ] = [:]
+  #else
+    // TODO(Xcode 16): Delete this block when minimum supported Xcode is
+    // Xcode 16.
+    private static var cachedInstances: [
+      String: WeakContainer<HeartbeatStorage>
+    ] = [:]
+  #endif // compiler(>=6)
+
+  /// Used to synchronize concurrent access to  the `cachedInstances` property.
+  private static let instancesLock = NSLock()
 
   /// Gets an existing `HeartbeatStorage` instance with the given `id` if one exists. Otherwise,
   /// makes a new instance with the given `id`.
   ///
   /// - Parameter id: A string identifier.
   /// - Returns: A `HeartbeatStorage` instance.
   static func getInstance(id: String) -> HeartbeatStorage {
-    if let cachedInstance = cachedInstances[id]?.object {
-      return cachedInstance
-    } else {
-      let newInstance = HeartbeatStorage.makeHeartbeatStorage(id: id)
-      cachedInstances[id] = WeakContainer(object: newInstance)
-      return newInstance
+    instancesLock.withLock {
+      if let cachedInstance = cachedInstances[id]?.object {
+        return cachedInstance
+      } else {
+        let newInstance = HeartbeatStorage.makeHeartbeatStorage(id: id)
+        cachedInstances[id] = WeakContainer(object: newInstance)
+        return newInstance
+      }
     }
   }
 
@@ -88,7 +109,9 @@ final class HeartbeatStorage: HeartbeatStorageProtocol {
 
   deinit {
     // Removes the instance if it was cached.
-    Self.cachedInstances.removeValue(forKey: id)
+    _ = Self.instancesLock.withLock {
+      Self.cachedInstances.removeValue(forKey: id)
+    }
   }
 
   // MARK: - HeartbeatStorageProtocol

FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift

@@ -399,6 +399,36 @@ class HeartbeatStorageTests: XCTestCase {
     // Then
     wait(for: expectations, timeout: 1.0, enforceOrder: true)
   }
+
+  func testForMemoryLeakInInstanceManager() {
+    // Given
+    let id = "testID"
+    var weakRefs: [WeakContainer<HeartbeatStorage>] = []
+    // Lock is used to synchronize `weakRefs` during concurrent access.
+    let weakRefsLock = NSLock()
+
+    // When
+    // Simulate concurrent access. This will help expose race conditions that could cause a crash.
+    let group = DispatchGroup()
+    for _ in 0 ..< 100 {
+      group.enter()
+      DispatchQueue.global().async {
+        let instance = HeartbeatStorage.getInstance(id: id)
+        weakRefsLock.withLock {
+          weakRefs.append(WeakContainer(object: instance))
+        }
+        group.leave()
+      }
+    }
+    group.wait()
+
+    // Then
+    // The `weakRefs` array's references should all be nil; otherwise, something is being
+    // unexpectedly strongly retained.
+    for weakRef in weakRefs {
+      XCTAssertNil(weakRef.object, "Potential memory leak detected.")
+    }
+  }
 }
 
 private class StorageFake: Storage {