diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index ff428077613..7fd808b4457 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -51,7 +51,26 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { // MARK: - Instance Management /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs. - private static var cachedInstances: [String: WeakContainer] = [:] + #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 + ] = [:] + #else + // TODO(Xcode 16): Delete this block when minimum supported Xcode is + // Xcode 16. + private static var cachedInstances: [ + String: WeakContainer + ] = [:] + #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`. @@ -59,12 +78,14 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { /// - 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 diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift index ed24275b88a..79f4cc5abf6 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/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] = [] + // 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 {