From 92469b98b798ade14c827e805040aed1b3a6f587 Mon Sep 17 00:00:00 2001 From: Paul Beusterien Date: Mon, 8 May 2023 14:19:19 -0700 Subject: [PATCH] Add missing weak specifiers --- FirebaseFunctions/CHANGELOG.md | 3 ++ .../Sources/Internal/FunctionsComponent.swift | 2 +- .../Tests/Unit/FunctionsTests.swift | 18 ++++++++++ .../Tests/Unit/SerializerTests.swift | 10 +++--- FirebaseStorage/CHANGELOG.md | 3 ++ FirebaseStorage/Sources/Storage.swift | 4 +-- .../Sources/StorageComponent.swift | 6 ++-- .../Tests/Unit/StorageComponentTests.swift | 35 ++++++++++++++----- 8 files changed, 61 insertions(+), 20 deletions(-) diff --git a/FirebaseFunctions/CHANGELOG.md b/FirebaseFunctions/CHANGELOG.md index 377c3936ab51..0b5bebb69681 100644 --- a/FirebaseFunctions/CHANGELOG.md +++ b/FirebaseFunctions/CHANGELOG.md @@ -1,3 +1,6 @@ +# 10.10.0 +- [fixed] Fixed potential memory leak of Functions instances. (#11248) + # 10.0.0 - [fixed] Remove unnecessary and unused `encoder` and `decoder` parameters from `public func call(_ data: Request) async throws -> Response` API. (#9891) diff --git a/FirebaseFunctions/Sources/Internal/FunctionsComponent.swift b/FirebaseFunctions/Sources/Internal/FunctionsComponent.swift index 6a39535181cb..4072a97a6e8d 100644 --- a/FirebaseFunctions/Sources/Internal/FunctionsComponent.swift +++ b/FirebaseFunctions/Sources/Internal/FunctionsComponent.swift @@ -36,7 +36,7 @@ protocol FunctionsProvider { // MARK: - Private Variables /// The app associated with all functions instances in this container. - private let app: FirebaseApp + private weak var app: FirebaseApp? /// A map of active instances, grouped by app. Keys are FirebaseApp names and values are arrays /// containing all instances of Functions associated with the given app. diff --git a/FirebaseFunctions/Tests/Unit/FunctionsTests.swift b/FirebaseFunctions/Tests/Unit/FunctionsTests.swift index 460cb88e27b2..3cccf0c21b6c 100644 --- a/FirebaseFunctions/Tests/Unit/FunctionsTests.swift +++ b/FirebaseFunctions/Tests/Unit/FunctionsTests.swift @@ -167,4 +167,22 @@ class FunctionsTests: XCTestCase { } waitForExpectations(timeout: 1.5) } + + /// Test that Functions instances get deallocated. + func testFunctionsLifecycle() throws { + weak var weakApp: FirebaseApp? + weak var weakFunctions: Functions? + try autoreleasepool { + let options = FirebaseOptions(googleAppID: "0:0000000000000:ios:0000000000000000", + gcmSenderID: "00000000000000000-00000000000-000000000") + options.projectID = "myProjectID" + let app1 = FirebaseApp(instanceWithName: "transitory app", options: options) + weakApp = try XCTUnwrap(app1) + let functions = Functions(app: app1, region: "transitory-region", customDomain: nil) + weakFunctions = functions + XCTAssertNotNil(weakFunctions) + } + XCTAssertNil(weakApp) + XCTAssertNil(weakFunctions) + } } diff --git a/FirebaseFunctions/Tests/Unit/SerializerTests.swift b/FirebaseFunctions/Tests/Unit/SerializerTests.swift index 1a412c87f666..bcd986ca1a4a 100644 --- a/FirebaseFunctions/Tests/Unit/SerializerTests.swift +++ b/FirebaseFunctions/Tests/Unit/SerializerTests.swift @@ -198,7 +198,7 @@ class SerializerTests: XCTestCase { 1 as Int32, "two", [3 as Int32, ["@type": "type.googleapis.com/google.protobuf.Int64Value", - "value": "9876543210"]], + "value": "9876543210"]] as [Any], ] as NSArray XCTAssertEqual(input, try serializer.encode(input) as? NSArray) } @@ -209,9 +209,9 @@ class SerializerTests: XCTestCase { 1 as Int64, "two", [3 as Int32, ["@type": "type.googleapis.com/google.protobuf.Int64Value", - "value": "9876543210"]], + "value": "9876543210"]] as [Any], ] as NSArray - let expected = [1 as Int64, "two", [3 as Int32, 9_876_543_210 as Int64]] as NSArray + let expected = [1 as Int64, "two", [3 as Int32, 9_876_543_210 as Int64] as [Any]] as NSArray XCTAssertEqual(expected, try serializer.decode(input) as? NSArray) } @@ -219,13 +219,13 @@ class SerializerTests: XCTestCase { let input = [ "foo": 1 as Int32, "bar": "hello", - "baz": [3 as Int32, 9_876_543_210 as Int64], + "baz": [3 as Int32, 9_876_543_210 as Int64] as [Any], ] as NSDictionary let expected = [ "foo": 1, "bar": "hello", "baz": [3, ["@type": "type.googleapis.com/google.protobuf.Int64Value", - "value": "9876543210"]], + "value": "9876543210"]] as [Any] as [Any], ] as NSDictionary let serializer = FUNSerializer() XCTAssertEqual(expected, try serializer.encode(input) as? NSDictionary) diff --git a/FirebaseStorage/CHANGELOG.md b/FirebaseStorage/CHANGELOG.md index f59ecffffd5b..238c3643418b 100644 --- a/FirebaseStorage/CHANGELOG.md +++ b/FirebaseStorage/CHANGELOG.md @@ -1,3 +1,6 @@ +# 10.10.0 +- [fixed] Fixed potential memory leak of Storage instances. (#11248) + # 10.7.0 - [added] Provide server errors via the `NSUnderlyingErrorKey`. diff --git a/FirebaseStorage/Sources/Storage.swift b/FirebaseStorage/Sources/Storage.swift index 8c453eeb403c..970583640688 100644 --- a/FirebaseStorage/Sources/Storage.swift +++ b/FirebaseStorage/Sources/Storage.swift @@ -70,7 +70,7 @@ import FirebaseAuthInterop @objc(storageForApp:) open class func storage(app: FirebaseApp) -> Storage { let provider = ComponentType.instance(for: StorageProvider.self, in: app.container) - return provider.storage(for: Storage.bucket(for: app)) + return provider.storage(for: Storage.bucket(for: app), app: app) } /** @@ -85,7 +85,7 @@ import FirebaseAuthInterop open class func storage(app: FirebaseApp, url: String) -> Storage { let provider = ComponentType.instance(for: StorageProvider.self, in: app.container) - return provider.storage(for: Storage.bucket(for: app, urlString: url)) + return provider.storage(for: Storage.bucket(for: app, urlString: url), app: app) } /** diff --git a/FirebaseStorage/Sources/StorageComponent.swift b/FirebaseStorage/Sources/StorageComponent.swift index 27108f17a97b..a3407728b3fa 100644 --- a/FirebaseStorage/Sources/StorageComponent.swift +++ b/FirebaseStorage/Sources/StorageComponent.swift @@ -22,14 +22,14 @@ import FirebaseCore @objc(FIRStorageProvider) protocol StorageProvider { - @objc func storage(for bucket: String) -> Storage + @objc func storage(for bucket: String, app: FirebaseApp) -> Storage } @objc(FIRStorageComponent) class StorageComponent: NSObject, Library, StorageProvider { // MARK: - Private Variables /// The app associated with all Storage instances in this container. - private let app: FirebaseApp + private weak var app: FirebaseApp? /// A map of active instances, grouped by app. Keys are FirebaseApp names and values are arrays /// containing all instances of Storage associated with the given app. @@ -63,7 +63,7 @@ protocol StorageProvider { // MARK: - StorageProvider conformance - func storage(for bucket: String) -> Storage { + func storage(for bucket: String, app: FirebaseApp) -> Storage { os_unfair_lock_lock(&instancesLock) // Unlock before the function returns. diff --git a/FirebaseStorage/Tests/Unit/StorageComponentTests.swift b/FirebaseStorage/Tests/Unit/StorageComponentTests.swift index d5fe813e288d..a53c0326bd45 100644 --- a/FirebaseStorage/Tests/Unit/StorageComponentTests.swift +++ b/FirebaseStorage/Tests/Unit/StorageComponentTests.swift @@ -34,8 +34,9 @@ class StorageComponentTests: StorageTestHelpers { /// Tests that a Storage instance can be created properly by the StorageComponent. func testStorageInstanceCreation() throws { - let component = StorageComponent(app: StorageComponentTests.app) - let storage = component.storage(for: "someBucket") + let app = try XCTUnwrap(StorageComponentTests.app) + let component = StorageComponent(app: app) + let storage = component.storage(for: "someBucket", app: app) XCTAssertNotNil(storage) } @@ -61,26 +62,42 @@ class StorageComponentTests: StorageTestHelpers { /// Tests that instances of Storage created are different. func testMultipleStorageInstancesCreated() throws { + let app = try XCTUnwrap(StorageComponentTests.app) let registrants = NSMutableSet(array: [StorageComponent.self]) - let container = FirebaseComponentContainer( - app: StorageComponentTests.app, - registrants: registrants - ) + let container = FirebaseComponentContainer(app: app, registrants: registrants) let provider = ComponentType.instance(for: StorageProvider.self, in: container) XCTAssertNotNil(provider) - let storage1 = provider.storage(for: "randomBucket") - let storage2 = provider.storage(for: "randomBucket") + let storage1 = provider.storage(for: "randomBucket", app: app) + let storage2 = provider.storage(for: "randomBucket", app: app) XCTAssertNotNil(storage1) // Ensure they're the same instance. XCTAssert(storage1 === storage2) - let storage3 = provider.storage(for: "differentBucket") + let storage3 = provider.storage(for: "differentBucket", app: app) XCTAssertNotNil(storage3) XCTAssert(storage1 !== storage3) } + + /// Test that Storage instances get deallocated. + func testStorageLifecycle() throws { + weak var weakApp: FirebaseApp? + weak var weakStorage: Storage? + try autoreleasepool { + let options = FirebaseOptions(googleAppID: "0:0000000000000:ios:0000000000000000", + gcmSenderID: "00000000000000000-00000000000-000000000") + options.projectID = "myProjectID" + let app1 = FirebaseApp(instanceWithName: "transitory app", options: options) + weakApp = try XCTUnwrap(app1) + let storage = Storage(app: app1, bucket: "transitory bucket") + weakStorage = storage + XCTAssertNotNil(weakStorage) + } + XCTAssertNil(weakApp) + XCTAssertNil(weakStorage) + } }