Delay Subscription Observer Notification (#327)

* Fix Extension Crash

• Fixes an issue (#322) where exceptions that occurred while downloading attachments were not handled correctly.
• This would have occurred in different situations. For example, if a device is out of space and doesn't have enough storage to download an attachment, it would throw an exception and crash the extension.
• This commit would obviously not resolve the fact that the attachment cannot be downloaded for various reasons, but it will stop unnecessary crashes and crash reports being sent to developers.

* Delay Subscription Settings Notification

• Makes a change so that if the user accepts the Notification Permission prompt, the SDK will delay the OSSubscription observer update until after the HTTP request to update the OneSignal has finished.
• This prevents a bug where, on some occasions when developers used the OSSubscription observer method to send out a push notification, the notification would fail because the observer method got called instantly the moment the user tapped 'Accept'.
• This caused a race condition where the HTTP request to update the user's subscription status to Accepted, and the HTTP request to send a push notification to this user, were both sent at the same time, causing it to sometimes fail.

* Added Test for Delayed Subscription Observer

• Added a test for making sure that the subscription observer doesn't fire immediately
• Changed OneSignalClientOverrider to let it switch threads before executing the HTTP callback so that it simulates the delay for a network request
• Created a separate queue for OneSignalClientOverrider so that XCTests can call runThreads to finish pending HTTP request calls
• This appears to have resolved some issues we've had with thread synchronization in our tests
• Added some clang compiler flags to ignore warnings about undeclared selectors in places where it is unnecessary

* Removed shouldDelaySubscriptionSettingsUpdate() from OneSignal.h

• Method shouldDelaySubscriptionSettingsUpdate() should not be exposed to clients.

* Minor Fixes

• Removes an unnecessary property (subscriptionLock) and an unnecessary define (EXECUTION_LOCK)
• Also removes an unnecessary comment

Author: Nightsd01

iOS_SDK/OneSignalSDK/Source/OSObservable.m

@@ -50,35 +50,47 @@ - (instancetype)init {
 }
 
 - (void)addObserver:(id)observer {
-    [observers addObject:observer];
+    @synchronized(observers) {
+        [observers addObject:observer];
+    }
 }
 
 - (void)removeObserver:(id)observer {
-    [observers removeObject:observer];
+    @synchronized(observers) {
+        [observers removeObject:observer];
+    }
 }
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Warc-performSelector-leaks"
 
 - (BOOL)notifyChange:(id)state {
     BOOL fired = false;
-     for (id observer in observers) {
-         fired = true;
-         if (changeSelector) {
-             // Any Obserable setup to fire a custom selector with changeSelector
-             //  is external to our SDK. Run on the main thread in case the
-             //  app developer needs to update UI elements.
-             [OneSignalHelper dispatch_async_on_main_queue: ^{
-                 [observer performSelector:changeSelector withObject:state];
-             }];
-         }
-         else
-             [observer onChanged:state];
-     }
+    
+    @synchronized(observers) {
+        for (id observer in observers) {
+            fired = true;
+            if (changeSelector) {
+                // Any Obserable setup to fire a custom selector with changeSelector
+                //  is external to our SDK. Run on the main thread in case the
+                //  app developer needs to update UI elements.
+                
+                [self callObserver:observer withSelector:changeSelector withState:state];
+                
+            } else
+                [observer onChanged:state];
+        }
+    }
 
     return fired;
 }
 
+- (void)callObserver:(id)observer withSelector:(SEL)selector withState:(id)state {
+    [OneSignalHelper dispatch_async_on_main_queue:^{
+        [observer performSelector:changeSelector withObject:state];
+    }];
+}
+
 #pragma clang diagnostic pop
 
 @end

iOS_SDK/OneSignalSDK/Source/OSSubscription.h

@@ -69,6 +69,9 @@ typedef OSObservable<NSObject<OSSubscriptionStateObserver>*, OSSubscriptionState
 - (void)setAccepted:(BOOL)inAccpeted;
 - (void)persistAsFrom;
 - (BOOL)compare:(OSSubscriptionState*)from;
+
+@property (nonatomic) BOOL delayedObserverUpdate;
+
 @end
 
 // Redefine OSSubscriptionStateChanges

iOS_SDK/OneSignalSDK/Source/OSSubscription.m

@@ -28,6 +28,9 @@
 #import "OSSubscription.h"
 
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wundeclared-selector"
+
 @implementation OSSubscriptionState
 
 - (ObserableSubscriptionStateType*)observable {
@@ -126,6 +129,16 @@ - (void)setUserSubscriptionSetting:(BOOL)userSubscriptionSetting {
 
 - (void)setAccepted:(BOOL)inAccpeted {
     BOOL lastSubscribed = self.subscribed;
+    
+    // checks to see if we should delay the observer update
+    // This is to prevent a problem where the observer gets updated
+    // before the OneSignal server does. (11f7f49841339317a334c5ec928db7edccb21cfe)
+    
+    if ([OneSignal performSelector:@selector(shouldDelaySubscriptionSettingsUpdate)]) {
+        self.delayedObserverUpdate = true;
+        return;
+    }
+    
     _accpeted = inAccpeted;
     if (lastSubscribed != self.subscribed)
         [self.observable notifyChange:self];

iOS_SDK/OneSignalSDK/Source/OneSignal.h

@@ -341,7 +341,6 @@ typedef NS_ENUM(NSUInteger, ONE_S_LOG_LEVEL) {
 + (void)deleteTags:(NSArray*)keys onSuccess:(OSResultSuccessBlock)successBlock onFailure:(OSFailureBlock)failureBlock;
 + (void)deleteTags:(NSArray*)keys;
 + (void)deleteTagsWithJsonString:(NSString*)jsonString;
-
 // Optional method that sends us the user's email as an anonymized hash so that we can better target and personalize notifications sent to that user across their devices.
 // Sends as MD5 and SHA1 of the provided email
 + (void)syncHashedEmail:(NSString*)email;

iOS_SDK/OneSignalSDK/Source/OneSignal.m

@@ -126,6 +126,8 @@ @implementation OneSignal
 static NSString* mSDKType = @"native";
 static BOOL coldStartFromTapOnNotification = NO;
 
+static BOOL shouldDelaySubscriptionUpdate = false;
+
 static NSMutableArray* pendingSendTagCallbacks;
 static OSResultSuccessBlock pendingGetTagsSuccessBlock;
 static OSFailureBlock pendingGetTagsFailureBlock;
@@ -294,7 +296,6 @@ + (void)clearStatics {
     _currentSubscriptionState = nil;
 
     _permissionStateChangesObserver = nil;
-    _subscriptionStateChangesObserver = nil;
 }
 
 // Set to false as soon as it's read.
@@ -303,6 +304,10 @@ + (BOOL)coldStartFromTapOnNotification {
     coldStartFromTapOnNotification = NO;
     return val;
 }
+
++ (BOOL)shouldDelaySubscriptionSettingsUpdate {
+    return shouldDelaySubscriptionUpdate;
+}
 
 + (id)initWithLaunchOptions:(NSDictionary*)launchOptions appId:(NSString*)appId {
     return [self initWithLaunchOptions: launchOptions appId: appId handleNotificationReceived: NULL handleNotificationAction : NULL settings: @{kOSSettingsKeyAutoPrompt : @YES, kOSSettingsKeyInAppAlerts : @YES, kOSSettingsKeyInAppLaunchURL : @YES}];
@@ -817,6 +822,8 @@ + (void)setSubscription:(BOOL)enable {
     [[NSUserDefaults standardUserDefaults] setObject:value forKey:@"ONESIGNAL_SUBSCRIPTION"];
     [[NSUserDefaults standardUserDefaults] synchronize];
 
+    shouldDelaySubscriptionUpdate = true;
+    
     self.currentSubscriptionState.userSubscriptionSetting = enable;
 
     if (app_id)
@@ -1113,7 +1120,17 @@ + (BOOL) sendNotificationTypesUpdate {
 
         mLastNotificationTypes = [self getNotificationTypes];
 
-        [OneSignalClient.sharedClient executeRequest:[OSRequestUpdateNotificationTypes withUserId:self.currentSubscriptionState.userId appId:self.app_id notificationTypes:@([self getNotificationTypes])] onSuccess:nil onFailure:nil];
+        //delays observer update until the OneSignal server is notified
+        shouldDelaySubscriptionUpdate = true;
+        
+        [OneSignalClient.sharedClient executeRequest:[OSRequestUpdateNotificationTypes withUserId:self.currentSubscriptionState.userId appId:self.app_id notificationTypes:@([self getNotificationTypes])] onSuccess:^(NSDictionary *result) {
+            
+            shouldDelaySubscriptionUpdate = false;
+            
+            if (self.currentSubscriptionState.delayedObserverUpdate)
+                [self.currentSubscriptionState setAccepted:[self getNotificationTypes] > 0];
+            
+        } onFailure:nil];
 
         if ([self getUsableDeviceToken])
             [self fireIdsAvailableCallback];

iOS_SDK/OneSignalSDK/Source/OneSignalHelper.m

@@ -725,20 +725,29 @@ + (NSString*)downloadMediaAndSaveInBundle:(NSString*)url {
     NSArray* paths = NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES);
     NSString* filePath = [paths[0] stringByAppendingPathComponent:name];
 
-    NSError* error = nil;
-    [NSURLSession downloadItemAtURL:URL toFile:filePath error:error];
-    NSArray* cachedFiles = [[NSUserDefaults standardUserDefaults] objectForKey:@"CACHED_MEDIA"];
-    NSMutableArray* appendedCache;
-    if (cachedFiles) {
-        appendedCache = [[NSMutableArray alloc] initWithArray:cachedFiles];
-        [appendedCache addObject:name];
+    //guard against situations where for example, available storage is too low
+    
+    @try {
+        NSError* error = nil;
+        [NSURLSession downloadItemAtURL:URL toFile:filePath error:error];
+        NSArray* cachedFiles = [[NSUserDefaults standardUserDefaults] objectForKey:@"CACHED_MEDIA"];
+        NSMutableArray* appendedCache;
+        if (cachedFiles) {
+            appendedCache = [[NSMutableArray alloc] initWithArray:cachedFiles];
+            [appendedCache addObject:name];
+        }
+        else
+            appendedCache = [[NSMutableArray alloc] initWithObjects:name, nil];
+        
+        [[NSUserDefaults standardUserDefaults] setObject:appendedCache forKey:@"CACHED_MEDIA"];
+        [[NSUserDefaults standardUserDefaults] synchronize];
+        return name;
+    } @catch (NSException *exception) {
+        [OneSignal onesignal_Log:ONE_S_LL_ERROR message:[NSString stringWithFormat:@"OneSignal encountered an exception while downloading file (%@), exception: %@", url, exception.description]];
+        
+        return nil;
     }
-    else
-        appendedCache = [[NSMutableArray alloc] initWithObjects:name, nil];
-    
-    [[NSUserDefaults standardUserDefaults] setObject:appendedCache forKey:@"CACHED_MEDIA"];
-    [[NSUserDefaults standardUserDefaults] synchronize];
-    return name;
+
 }
 
 +(void)clearCachedMedia {

iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.h

@@ -17,6 +17,7 @@
 +(int)networkRequestCount;
 +(void)setLastUrl:(NSString*)value;
 +(NSString*)lastUrl;
-
++(void)setShouldExecuteInstantaneously:(BOOL)instant;
++ (dispatch_queue_t)getHTTPQueue;
 @end
 

iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m

@@ -23,23 +23,43 @@
 #import "OneSignalRequest.h"
 #import "OneSignalSelectorHelpers.h"
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+
 @implementation OneSignalClientOverrider
 
 static dispatch_queue_t serialMockMainLooper;
 static NSString* lastUrl;
 static int networkRequestCount;
 static NSDictionary* lastHTTPRequest;
 static XCTestCase* currentTestInstance;
+static BOOL executeInstantaneously = true;
+static dispatch_queue_t executionQueue;
 
 + (void)load {
     serialMockMainLooper = dispatch_queue_create("com.onesignal.unittest", DISPATCH_QUEUE_SERIAL);
 
 
     //with refactored networking code, need to replace the implementation of the execute request method so tests don't actually execite HTTP requests
     injectToProperClass(@selector(overrideExecuteRequest:onSuccess:onFailure:), @selector(executeRequest:onSuccess:onFailure:), @[], [OneSignalClientOverrider class], [OneSignalClient class]);
+    
+    executionQueue = dispatch_queue_create("com.onesignal.execution", NULL);
 }
 
 - (void)overrideExecuteRequest:(OneSignalRequest *)request onSuccess:(OSResultSuccessBlock)successBlock onFailure:(OSFailureBlock)failureBlock {
+    NSLog(@"Executing request: %@", NSStringFromClass([request class]));
+    if (executeInstantaneously) {
+        [OneSignalClientOverrider finishExecutingRequest:request onSuccess:successBlock onFailure:failureBlock];
+    } else {
+        dispatch_async(executionQueue, ^{
+            [OneSignalClientOverrider finishExecutingRequest:request onSuccess:successBlock onFailure:failureBlock];
+        });
+    }
+}
+
++ (void)finishExecutingRequest:(OneSignalRequest *)request onSuccess:(OSResultSuccessBlock)successBlock onFailure:(OSFailureBlock)failureBlock {
+    NSLog(@"completing HTTP request: %@", NSStringFromClass([request class]));
+    
     NSMutableDictionary *parameters = [request.parameters mutableCopy];
 
     if (!parameters[@"app_id"] && ![request.request.URL.absoluteString containsString:@"/apps/"])
@@ -62,6 +82,14 @@ - (void)overrideExecuteRequest:(OneSignalRequest *)request onSuccess:(OSResultSu
     }
 }
 
++(dispatch_queue_t)getHTTPQueue {
+    return executionQueue;
+}
+
++(void)setShouldExecuteInstantaneously:(BOOL)instant {
+    executeInstantaneously = instant;
+}
+
 +(void)reset:(XCTestCase*)testInstance {
     currentTestInstance = testInstance;
 

iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalHelperOverrider.m

@@ -32,6 +32,9 @@
 #import "OneSignal.h"
 #import "OneSignalHelper.h"
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wundeclared-selector"
+
 @implementation OneSignalHelperOverrider
 
 static dispatch_queue_t serialMockMainLooper;