Merge pull request #1412 from OneSignal/fix/user_module_user_defaults…

…_crashes

Fix crashes when encoding user models

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];
     }
 }
 

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, ); }; };
@@ -945,7 +944,6 @@
 		1AF75EAD1E8567FD0097B315 /* NSString+OneSignal.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSString+OneSignal.m"; sourceTree = "<group>"; };
 		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 = "<group>"; };
 		3C0EF49D28A1DBCB00E5434B /* OSUserInternalImpl.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OSUserInternalImpl.swift; sourceTree = "<group>"; };
 		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 = "<group>"; };
@@ -1643,14 +1641,6 @@
 			name = Frameworks;
 			sourceTree = "<group>";
 		};
-		3C0590492B86BC5300450D48 /* Utils */ = {
-			isa = PBXGroup;
-			children = (
-				3C05904A2B86BC9E00450D48 /* UnfairLock.swift */,
-			);
-			path = Utils;
-			sourceTree = "<group>";
-		};
 		3C115162289A259500565C41 /* OneSignalOSCore */ = {
 			isa = PBXGroup;
 			children = (
@@ -1664,7 +1654,6 @@
 			isa = PBXGroup;
 			children = (
 				3C115163289A259500565C41 /* OneSignalOSCore.h */,
-				3C0590492B86BC5300450D48 /* Utils */,
 				3C115188289ADEA300565C41 /* OSModelStore.swift */,
 				3C115186289ADE7700565C41 /* OSModelStoreListener.swift */,
 				3C115184289ADE4F00565C41 /* OSModel.swift */,
@@ -3344,7 +3333,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 */,
 			);

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)
                 }

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)
         }

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,55 +69,57 @@ 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 = [:]
         }
     }
 
     // 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)
     }
 

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,31 +127,31 @@ class OSPropertiesModel: OSModel {
      */
     func clearData() {
         // TODO: What about language, lat, long?
-        tagsLock.locked {
+        tagsLock.withLock {
             self.tags = [:]
         }
     }
 
     // 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:

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

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()
+        }
+    }
 }