From b3a8fcd26a84ac5e8905477738d27a1545e4bf7f Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Oct 2024 20:07:21 -0400 Subject: [PATCH 1/8] [Core] Make instance manager conform to Swift 6 principles --- .../HeartbeatLogging/HeartbeatStorage.swift | 28 +++++++++---- .../Tests/Unit/HeartbeatStorageTests.swift | 39 +++++++++++++++++++ 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index ff428077613..0a04d8a75c9 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -51,7 +51,13 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { // MARK: - Instance Management /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs. - private static var cachedInstances: [String: WeakContainer] = [:] + private nonisolated(unsafe) static var cachedInstances: [ + String: WeakContainer + ] = + [:] + + /// Used to synchronize mutations + 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 +65,16 @@ 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 { + print("fred") + return cachedInstance + } else { + print("bob") + let newInstance = HeartbeatStorage.makeHeartbeatStorage(id: id) + cachedInstances[id] = WeakContainer(object: newInstance) + return newInstance + } } } @@ -88,7 +98,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..3f8af5037bd 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift @@ -399,6 +399,45 @@ class HeartbeatStorageTests: XCTestCase { // Then wait(for: expectations, timeout: 1.0, enforceOrder: true) } + + func testForMemoryLeakInInstanceManagemer() { + // 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() + + #if !os(macOS) + // Simulate memory pressure. This will force the system to more aggressively reclaim memory. + // This simulation makes the test more sensitive to potential leaks. + NotificationCenter.default.post( + name: UIApplication.didReceiveMemoryWarningNotification, + object: UIApplication.shared + ) + #endif // !os(macOS) + + // 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 { From a0bec6eafd02027b05ff1f16961ca8204fff1cd0 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Oct 2024 20:15:35 -0400 Subject: [PATCH 2/8] Finish comment --- .../Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index 0a04d8a75c9..7c58e2c9e67 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -56,7 +56,7 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { ] = [:] - /// Used to synchronize mutations + /// 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, From 37c09c14525fffc5c6eedc70364a10bbfe720fee Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 18 Oct 2024 20:16:16 -0400 Subject: [PATCH 3/8] remove debug code --- .../Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index 7c58e2c9e67..cf5ded26da0 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -67,10 +67,8 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { static func getInstance(id: String) -> HeartbeatStorage { instancesLock.withLock { if let cachedInstance = cachedInstances[id]?.object { - print("fred") return cachedInstance } else { - print("bob") let newInstance = HeartbeatStorage.makeHeartbeatStorage(id: id) cachedInstances[id] = WeakContainer(object: newInstance) return newInstance From 6bc0aa30a201dadb92809a75f80a357645e9f2ea Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Oct 2024 10:24:43 -0400 Subject: [PATCH 4/8] Conditional compilation --- .../HeartbeatLogging/HeartbeatStorage.swift | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index cf5ded26da0..865512ecb86 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -51,10 +51,17 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { // MARK: - Instance Management /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs. - private nonisolated(unsafe) static var cachedInstances: [ - String: WeakContainer - ] = - [:] + #if compiler(>=6) + private nonisolated(unsafe) static var cachedInstances: [ + String: WeakContainer + ] = + [:] + #else + private static var cachedInstances: [ + String: WeakContainer + ] = + [:] + #endif // compiler(>=6) /// Used to synchronize concurrent access to the `cachedInstances` property. private static let instancesLock = NSLock() From 8e21fff20e165605b2389603f15b53ab40eea8fe Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Oct 2024 10:38:44 -0400 Subject: [PATCH 5/8] Add comment --- .../Sources/HeartbeatLogging/HeartbeatStorage.swift | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index 865512ecb86..1b8e84c0687 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -52,11 +52,19 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs. #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 ] = From 8bf67fd8fec04c96ee52fea2c779eb06c741799a Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Oct 2024 10:58:04 -0400 Subject: [PATCH 6/8] [skip ci] fix typo --- FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift index 3f8af5037bd..2b8e4e43935 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift @@ -400,7 +400,7 @@ class HeartbeatStorageTests: XCTestCase { wait(for: expectations, timeout: 1.0, enforceOrder: true) } - func testForMemoryLeakInInstanceManagemer() { + func testForMemoryLeakInInstanceManager() { // Given let id = "testID" var weakRefs: [WeakContainer] = [] From 969d0952408927842069180dad647de317c0ab83 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Oct 2024 11:00:18 -0400 Subject: [PATCH 7/8] Remove notification --- .../Internal/Tests/Unit/HeartbeatStorageTests.swift | 9 --------- 1 file changed, 9 deletions(-) diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift index 2b8e4e43935..79f4cc5abf6 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift @@ -422,15 +422,6 @@ class HeartbeatStorageTests: XCTestCase { } group.wait() - #if !os(macOS) - // Simulate memory pressure. This will force the system to more aggressively reclaim memory. - // This simulation makes the test more sensitive to potential leaks. - NotificationCenter.default.post( - name: UIApplication.didReceiveMemoryWarningNotification, - object: UIApplication.shared - ) - #endif // !os(macOS) - // Then // The `weakRefs` array's references should all be nil; otherwise, something is being // unexpectedly strongly retained. From b3f06a64dc4d43f6ac32fb84dcc4025d97796fba Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Mon, 21 Oct 2024 15:50:35 -0400 Subject: [PATCH 8/8] Review --- .../Sources/HeartbeatLogging/HeartbeatStorage.swift | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index 1b8e84c0687..7fd808b4457 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -60,15 +60,13 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { // (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.