diff --git a/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m b/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m index 6833ff5dc..e4dbf20e9 100644 --- a/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m +++ b/iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m @@ -121,10 +121,12 @@ - (IBAction)removeAliasButton:(UIButton *)sender { } - (IBAction)sendTagButton:(id)sender { - if (self.tagKey.text && self.tagKey.text.length - && self.tagValue.text && self.tagValue.text.length) { + if (self.tagValue.text && self.tagValue.text.length) { NSLog(@"Sending tag with key: %@ value: %@", self.tagKey.text, self.tagValue.text); [OneSignal.User addTagWithKey:self.tagKey.text value:self.tagValue.text]; + } else { + NSLog(@"Removing tag with key: %@", self.tagKey.text); + [OneSignal.User removeTag:self.tagKey.text]; } } diff --git a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj index 6cac7ee9d..434711331 100644 --- a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj +++ b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj @@ -53,7 +53,6 @@ 03E56DD328405F4A006AA1DA /* OneSignalAppDelegateOverrider.m in Sources */ = {isa = PBXBuildFile; fileRef = 03E56DD228405F4A006AA1DA /* OneSignalAppDelegateOverrider.m */; }; 16664C4C25DDB195003B8A14 /* NSTimeZoneOverrider.m in Sources */ = {isa = PBXBuildFile; fileRef = 16664C4B25DDB195003B8A14 /* NSTimeZoneOverrider.m */; }; 37E6B2BB19D9CAF300D0C601 /* UIKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 37E6B2BA19D9CAF300D0C601 /* UIKit.framework */; settings = {ATTRIBUTES = (Weak, ); }; }; - 3C05904B2B86BC9E00450D48 /* UnfairLock.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */; }; 3C0EF49E28A1DBCB00E5434B /* OSUserInternalImpl.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3C0EF49D28A1DBCB00E5434B /* OSUserInternalImpl.swift */; }; 3C115165289A259500565C41 /* OneSignalOSCore.docc in Sources */ = {isa = PBXBuildFile; fileRef = 3C115164289A259500565C41 /* OneSignalOSCore.docc */; }; 3C115171289A259500565C41 /* OneSignalOSCore.h in Headers */ = {isa = PBXBuildFile; fileRef = 3C115163289A259500565C41 /* OneSignalOSCore.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -944,7 +943,6 @@ 1AF75EAD1E8567FD0097B315 /* NSString+OneSignal.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSString+OneSignal.m"; sourceTree = ""; }; 37747F9319147D6500558FAD /* libOneSignal.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libOneSignal.a; sourceTree = BUILT_PRODUCTS_DIR; }; 37E6B2BA19D9CAF300D0C601 /* UIKit.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = UIKit.framework; path = System/Library/Frameworks/UIKit.framework; sourceTree = SDKROOT; }; - 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UnfairLock.swift; sourceTree = ""; }; 3C0EF49D28A1DBCB00E5434B /* OSUserInternalImpl.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OSUserInternalImpl.swift; sourceTree = ""; }; 3C115161289A259500565C41 /* OneSignalOSCore.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = OneSignalOSCore.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 3C115163289A259500565C41 /* OneSignalOSCore.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = OneSignalOSCore.h; sourceTree = ""; }; @@ -1640,14 +1638,6 @@ name = Frameworks; sourceTree = ""; }; - 3C0590492B86BC5300450D48 /* Utils */ = { - isa = PBXGroup; - children = ( - 3C05904A2B86BC9E00450D48 /* UnfairLock.swift */, - ); - path = Utils; - sourceTree = ""; - }; 3C115162289A259500565C41 /* OneSignalOSCore */ = { isa = PBXGroup; children = ( @@ -1661,7 +1651,6 @@ isa = PBXGroup; children = ( 3C115163289A259500565C41 /* OneSignalOSCore.h */, - 3C0590492B86BC5300450D48 /* Utils */, 3C115188289ADEA300565C41 /* OSModelStore.swift */, 3C115186289ADE7700565C41 /* OSModelStoreListener.swift */, 3C115184289ADE4F00565C41 /* OSModel.swift */, @@ -3338,7 +3327,6 @@ 3C115187289ADE7700565C41 /* OSModelStoreListener.swift in Sources */, 3CE5F9E3289D88DC004A156E /* OSModelStoreChangedHandler.swift in Sources */, 3C2D8A5928B4C4E300BE41F6 /* OSDelta.swift in Sources */, - 3C05904B2B86BC9E00450D48 /* UnfairLock.swift in Sources */, 3C11518D289AF5E800565C41 /* OSModelChangedHandler.swift in Sources */, 3C8E6DF928A6D89E0031E48A /* OSOperationExecutor.swift in Sources */, ); diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift deleted file mode 100644 index 9d9f69ad2..000000000 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Utils/UnfairLock.swift +++ /dev/null @@ -1,47 +0,0 @@ -/* - Modified MIT License - - Copyright 2024 OneSignal - - Permission is hereby granted, free of charge, to any person obtaining a copy - of this software and associated documentation files (the "Software"), to deal - in the Software without restriction, including without limitation the rights - to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - copies of the Software, and to permit persons to whom the Software is - furnished to do so, subject to the following conditions: - - 1. The above copyright notice and this permission notice shall be included in - all copies or substantial portions of the Software. - - 2. All copies of substantial portions of the Software may only be used in connection - with services provided by OneSignal. - - THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - THE SOFTWARE. - */ - -// Taken from https://swiftrocks.com/thread-safety-in-swift -// Read http://www.russbishop.net/the-law for more information on why this is necessary -public final class UnfairLock { - private var _lock: UnsafeMutablePointer - - public init() { - _lock = UnsafeMutablePointer.allocate(capacity: 1) - _lock.initialize(to: os_unfair_lock()) - } - - deinit { - _lock.deallocate() - } - - public func locked(_ closure: () throws -> ReturnValue) rethrows -> ReturnValue { - os_unfair_lock_lock(_lock) - defer { os_unfair_lock_unlock(_lock) } - return try closure() - } -} diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift index 06cfe3175..fd4674e9f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift @@ -124,7 +124,7 @@ class OSIdentityOperationExecutor: OSOperationExecutor { addRequestQueue.append(request) case OS_REMOVE_ALIAS_DELTA: - if let label = aliases.first?.key { + for (label, _) in aliases { let request = OSRequestRemoveAlias(labelToRemove: label, identityModel: model) removeRequestQueue.append(request) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index be0758ebc..0bb0ee47f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -520,10 +520,6 @@ extension OSUserExecutor { return } - if let identityObject = parseIdentityObjectResponse(response) { - OneSignalUserManagerImpl.sharedInstance.user.identityModel.hydrate(identityObject) - } - if let propertiesObject = parsePropertiesObjectResponse(response) { OneSignalUserManagerImpl.sharedInstance.user.propertiesModel.hydrate(propertiesObject) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift index 72a5671e2..194e6f16f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift @@ -31,19 +31,16 @@ import OneSignalOSCore class OSIdentityModel: OSModel { var onesignalId: String? { - aliasesLock.locked { - return aliases[OS_ONESIGNAL_ID] - } + return internalGetAlias(OS_ONESIGNAL_ID) } var externalId: String? { - aliasesLock.locked { - return aliases[OS_EXTERNAL_ID] - } + return internalGetAlias(OS_EXTERNAL_ID) } + // All access to aliases should go through helper methods with locking var aliases: [String: String] = [:] - private let aliasesLock = UnfairLock() + private let aliasesLock = NSRecursiveLock() // TODO: We need to make this token secure public var jwtBearerToken: String? @@ -57,8 +54,10 @@ class OSIdentityModel: OSModel { } override func encode(with coder: NSCoder) { - super.encode(with: coder) - coder.encode(aliases, forKey: "aliases") + aliasesLock.withLock { + super.encode(with: coder) + coder.encode(aliases, forKey: "aliases") + } } required init?(coder: NSCoder) { @@ -70,11 +69,29 @@ class OSIdentityModel: OSModel { self.aliases = aliases } + /** Threadsafe getter for an alias */ + private func internalGetAlias(_ label: String) -> String? { + aliasesLock.withLock { + return self.aliases[label] + } + } + + /** Threadsafe setter or removal for aliases */ + private func internalAddAliases(_ aliases: [String: String]) { + aliasesLock.withLock { + for (label, id) in aliases { + // Remove the alias if the ID field is "" + self.aliases[label] = id.isEmpty ? nil : id + } + } + self.set(property: "aliases", newValue: aliases) + } + /** Called to clear the model's data in preparation for hydration via a fetch user call. */ func clearData() { - aliasesLock.locked { + aliasesLock.withLock { self.aliases = [:] } } @@ -82,43 +99,27 @@ class OSIdentityModel: OSModel { // MARK: - Alias Methods func addAliases(_ aliases: [String: String]) { - aliasesLock.locked { - for (label, id) in aliases { - self.aliases[label] = id - } - self.set(property: "aliases", newValue: aliases) - } + internalAddAliases(aliases) } func removeAliases(_ labels: [String]) { - aliasesLock.locked { - for label in labels { - self.aliases.removeValue(forKey: label) - self.set(property: "aliases", newValue: [label: ""]) - } + let aliasesToRemoveAsDict = labels.reduce(into: [String: String]()) { result, label in + result[label] = "" } + internalAddAliases(aliasesToRemoveAsDict) } public override func hydrateModel(_ response: [String: Any]) { - OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSIdentityModel hydrateModel()") - var newOnesignalId: String? - var newExternalId: String? - - aliasesLock.locked { - for property in response { - switch property.key { - case "external_id": - newExternalId = property.value as? String - aliases[OS_EXTERNAL_ID] = newExternalId - case "onesignal_id": - newOnesignalId = property.value as? String - aliases[OS_ONESIGNAL_ID] = newOnesignalId - default: - aliases[property.key] = property.value as? String - } - self.set(property: "aliases", newValue: aliases) - } + guard let remoteAliases = response as? [String: String] else { + OneSignalLog.onesignalLog(.LL_ERROR, message: "OSIdentityModel.hydrateModel failed to parse response \(response) as Strings") + return } + + OneSignalLog.onesignalLog(.LL_VERBOSE, message: "OSIdentityModel hydrateModel with aliases: \(remoteAliases)") + let newOnesignalId = remoteAliases[OS_ONESIGNAL_ID] + let newExternalId = remoteAliases[OS_EXTERNAL_ID] + + internalAddAliases(remoteAliases) fireUserStateChanged(newOnesignalId: newOnesignalId, newExternalId: newExternalId) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift index edc23fc88..3a44f6e73 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSPropertiesModel.swift @@ -82,7 +82,7 @@ class OSPropertiesModel: OSModel { var timezoneId = TimeZone.current.identifier var tags: [String: String] = [:] - private let tagsLock = UnfairLock() + private let tagsLock = NSRecursiveLock() // MARK: - Initialization @@ -102,10 +102,12 @@ class OSPropertiesModel: OSModel { } override func encode(with coder: NSCoder) { - super.encode(with: coder) - coder.encode(language, forKey: "language") - coder.encode(tags, forKey: "tags") - // ... and more + tagsLock.withLock { + super.encode(with: coder) + coder.encode(language, forKey: "language") + coder.encode(tags, forKey: "tags") + // ... and more + } } required init?(coder: NSCoder) { @@ -125,7 +127,7 @@ class OSPropertiesModel: OSModel { */ func clearData() { // TODO: What about language, lat, long? - tagsLock.locked { + tagsLock.withLock { self.tags = [:] } } @@ -133,23 +135,23 @@ class OSPropertiesModel: OSModel { // MARK: - Tag Methods func addTags(_ tags: [String: String]) { - tagsLock.locked { + tagsLock.withLock { for (key, value) in tags { self.tags[key] = value } - self.set(property: "tags", newValue: tags) } + self.set(property: "tags", newValue: tags) } func removeTags(_ tags: [String]) { - tagsLock.locked { - var tagsToSend: [String: String] = [:] + var tagsToSend: [String: String] = [:] + tagsLock.withLock { for tag in tags { self.tags.removeValue(forKey: tag) tagsToSend[tag] = "" } - self.set(property: "tags", newValue: tagsToSend) } + self.set(property: "tags", newValue: tagsToSend) } public override func hydrateModel(_ response: [String: Any]) { @@ -158,7 +160,7 @@ class OSPropertiesModel: OSModel { case "language": self.language = property.value as? String case "tags": - tagsLock.locked { + tagsLock.withLock { self.tags = property.value as? [String: String] ?? [:] } default: diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml b/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml index c6778fb1d..c22fc771a 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/.swiftlint.yml @@ -1,3 +1,4 @@ # in tests, we may want to force cast and throw any errors disabled_rules: - force_cast + - variable_name diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index b87e3b126..d8111396c 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -93,4 +93,48 @@ final class OneSignalUserTests: XCTestCase { // 1. OpRepo: `deltaQueue.remove(at: index)` index out of bounds // 2. OSPropertyOperationExecutor: `deltaQueue.append(delta)` EXC_BAD_ACCESS } + + /** + This test reproduced a crash when the property model is being encoded. + */ + func testEncodingPropertiesModel_withConcurrency_doesNotCrash() throws { + /* Setup */ + let propertiesModel = OSPropertiesModel(changeNotifier: OSEventProducer()) + + /* When */ + DispatchQueue.concurrentPerform(iterations: 5_000) { i in + // 1. Add tags + for num in 0...9 { + propertiesModel.addTags(["\(i)tag\(num)": "value"]) + } + + // 2. Encode the model + OneSignalUserDefaults.initShared().saveCodeableData(forKey: "PropertyModel", withValue: propertiesModel) + + // 3. Clear the tags + propertiesModel.clearData() + } + } + + /** + This test reproduced a crash when the identity model is being encoded. + */ + func testEncodingIdentityModel_withConcurrency_doesNotCrash() throws { + /* Setup */ + let identityModel = OSIdentityModel(aliases: nil, changeNotifier: OSEventProducer()) + + /* When */ + DispatchQueue.concurrentPerform(iterations: 5_000) { i in + // 1. Add aliases + for num in 0...9 { + identityModel.addAliases(["\(i)alias\(num)": "value"]) + } + + // 2. Encode the model + OneSignalUserDefaults.initShared().saveCodeableData(forKey: "IdentityModel", withValue: identityModel) + + // 2. Clear the aliases + identityModel.clearData() + } + } }