Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIRInstanceID.instanceIDWithHandler() handler is not called if unregisterForRemoteNotifications() has previously been called more than once #3229

Closed
jasongiss opened this issue Jun 20, 2019 · 14 comments · Fixed by #3251

Comments

@jasongiss
Copy link

jasongiss commented Jun 20, 2019

  • Xcode version: 10.2.1
  • Firebase SDK version: 6.2.0
  • Firebase Component: Messaging
  • Component version: 4.0.2

[REQUIRED] Step 3: Describe the problem

The instanceIDWithHandler() method of FIRInstanceID docs say that the handler will always be called. However, seemingly if you have previously called your Application's unregisterForRemoteNotifications() twice, then the handler is never called.

This is happening in my application as I allow the user to enable/disable notifications from within the app, and if they turn them off and on a few times, the callback handler is never called and so things lock up.

The docs say that you should be able to call this at any point to get the current token, rather than storing it yourself.
(The anchor link I have used doesn't seem to work - I am referring to the "Fetching the current registration token" section)

Steps to reproduce:

I've created a simple test project which shows the problem, which is attached.
FirebaseMessagingTest.zip

  • Add your own GoogleService-Info.plist, and change the Bundle Identifier accordingly.
  • Run pod install to install the Firbase SDK via Cocoapods.
  • Run the app, with device attached, click the button to enable/disable notifications several times.
  • Watch the Output window in Xcode for log messages: Once it goes wrong, you'll see it starts fetching the token, but never finishes.

Relevant Code:

Alternatively, below is the code (minus the delegates):

class ViewController: UIViewController {
	
	typealias CompletionHandler = (_ success: Bool, _ error: Error?) -> Void
	
	private var mNotificationsEnabled = false

	override func viewDidLoad() {
		super.viewDidLoad()
		
		FirebaseApp.configure()
	}

	@IBAction func ButtonPressed(_ sender: UIButton) {
		mNotificationsEnabled = !mNotificationsEnabled
		let newTitle = mNotificationsEnabled ? "Disable Notifications" : "Enable Notifications"
		sender.setTitle(newTitle, for: .normal)
		sender.isEnabled = false
		
		setNotifications(enabled: mNotificationsEnabled, completionHandler: { (success, error) in
			sender.isEnabled = true
		})
	}
	
	func setNotifications(enabled: Bool, completionHandler: @escaping CompletionHandler) {
		if (enabled) {
			enableNotifications(completionHandler: {(success, error) in
				self.getToken(completionHandler)
			})
		}
		else {
			disableNotifications(completionHandler: completionHandler)
		}
	}
	
	func enableNotifications(completionHandler: @escaping CompletionHandler) {
		print("Enabling...")
		
		let notificationCenter = UNUserNotificationCenter.current()
		notificationCenter.requestAuthorization(options: [.alert, .sound, .badge], completionHandler: {(success, error) in
			if (success) {
				notificationCenter.delegate = self;
				Messaging.messaging().delegate = self
				
				DispatchQueue.main.async(execute: {
					UIApplication.shared.registerForRemoteNotifications() // Must run on main thread
					print("Notifications Enabled")
					completionHandler(success, error)
				})
				
			}
			else {
				print("Enabling failed")
				completionHandler(false, error)
			}
		})
	}
	
	func disableNotifications(completionHandler: @escaping CompletionHandler) {
		print("Disabling...")
		
		DispatchQueue.main.async(execute: {
			UIApplication.shared.unregisterForRemoteNotifications() // Must run on main thread
			print("Notifications Disabled")
			completionHandler(true, nil)
		})
	}
	
	func getToken(_ completionHandler: @escaping CompletionHandler) {
		print("Fetching Token...")
		
		//MARK: BUG? The following never calls its handler if invoked after unregisterForRemoteNotifications() has been called twice.
		InstanceID.instanceID().instanceID(handler: { (result, error) in
			if let error = error {
				print("Error fetching remote instance ID: \(error)")
				completionHandler(false, error)
			}
			else if let result = result {
				print("Token found")
				completionHandler(true, nil)
			}
		})
	}
	
}
@morganchen12
Copy link
Contributor

morganchen12 commented Jun 20, 2019

Sounds like a bug--the callback should be invoked with an error in this case. In your sample code, the closure can be invoked without printing anything to console iff result and both error are nil; can you add a print statement outside of the if-else to be sure?

It's still worth fixing if it's returning nil for both result and error, but I'd like to better pinpoint where the failure is.

@charlotteliang
Copy link
Contributor

This is expected behavior. The handler will not be called if IID and token haven't changed, and they are not changed when you calling apple api unregisterForRemoteNotifications and it's probably also because Apple hasn't changed the apns token (unless that's not true, please let me know). It's just that system disable for remote notifications. And the FCMToken will not be able to sent for push notification after client unregister it.
Unless client has a way to determine that the APNS token is invalid, I don't think we should report any error here.

@jasongiss
Copy link
Author

jasongiss commented Jun 21, 2019

Thank you both very much for taking a look at this.

@morganchen12 Apologies for missing the code path where error & result are both nil - I can confirm that when the issue occurs, the closure is not called at all.

It looks like the handler goes onto a queue, because there is currently no cached token, in instanceIDWithHandler:(FIRInstanceIDResultHandler)handler. (it ends up calling defaultTokenWithRetry:(BOOL)retry handler:(nullable FIRInstanceIDTokenHandler)aHandler with retry set to NO). But it never gets pulled off and executed.

@chliangGoogle I wonder if I may have caused some confusion as to when this is occurring? It's not when unregisterForRemoteNotifications is called, but after Push Notifications have been re-enabled (after previously enabling/disabling Push Notifications multiple times)

If this is expected behaviour, it seems to contradict the docs, which say:

Generally, the token is available locally, so this method does not open a network connection. You can use this method at any time to access the token instead of storing it.

@jasongiss
Copy link
Author

I think perhaps that there may be a deeper issue (or misunderstanding on my part)?

If I instead rely purely on the Firebase MessagingDelegate's didReceiveRegistrationToken method to obtain the token, I find that after going through the same process of enabling and disabling Push Notifications twice, when I then re-enable Push Notifications, the device no longer receives notifications.

When I next restart the app, the MessagingDelegate receives a new (different) token.

Is there a different way that I should be disabling Push Notifications on the fly, rather than calling * unregisterForRemoteNotifications*?

@morganchen12
Copy link
Contributor

Your application must be registered for notifications before it can receive a device token (which the instance ID is associated with). In this case the documentation is not accurate. It is definitely weird that the trailing closure parameter is not called, but it's also kind of an edge-case behavior.

Is there a different way that I should be disabling Push Notifications on the fly, rather than calling unregisterForRemoteNotifications?

You can leave your application registered for notifications and then just not send them. A common way to send notifications is to write your instance ID somewhere and then send only to that instance ID; disabling notifications then would just require deleting your instance ID from that location.

We should consider renaming that parameter to completion: and guaranteeing its invocation so that the API is less weird.

@jasongiss
Copy link
Author

Thanks very much, @morganchen12.

Although I think the application is registered to receive notifications.
I only ever call to request the instanceID after (re) enabling Remote Notifications (registerForRemoteNotifications).
Apple's callback for doing this comes back successful.

Also, the Firebase API allows you to send messages to Topics - so I don't think the idea of disabling the token just on my server would prevent the client receiving notifications sent to a topic to which the client had subscribed?

@morganchen12
Copy link
Contributor

You'd have to unsubscribe the client from those topics. The instance ID behavior you're describing sounds like a bug.

@charlotteliang
Copy link
Contributor

charlotteliang commented Jun 21, 2019

Apple's callback for doing this comes back successful.

Can you tell us which callback you mean? If it's the apnsToken refresh callback, then there could be a bug inside swizzling.

@jasongiss
Copy link
Author

Thank you both again for continuing with this - I really appreciate your time.

The callback I mean is:
application(_:didRegisterForRemoteNotificationsWithDeviceToken:)

@morganchen12
Copy link
Contributor

Thanks @jasongiss. I'll investigate early next week and post updates here.

@morganchen12
Copy link
Contributor

I wasn't able to reproduce this issue. Here's the code I used:

  [[UIApplication sharedApplication] registerForRemoteNotifications];
  [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:10]];
  [[UIApplication sharedApplication] unregisterForRemoteNotifications];
  [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:10]];
  [[UIApplication sharedApplication] registerForRemoteNotifications];
  [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:10]];
  [[UIApplication sharedApplication] unregisterForRemoteNotifications];

  [[FIRInstanceID instanceID] instanceIDWithHandler:^(FIRInstanceIDResult * _Nullable result,
                                                      NSError * _Nullable error) {
    if (error != nil) {
      NSLog(@"Error fetching remote instance ID: %@", error);
    } else {
      NSLog(@"Remote instance ID token: %@", result.token);
      NSString* message =
        [NSString stringWithFormat:@"Remote InstanceID token: %@", result.token];
      self.instanceIDTokenMessage.text = message;
    }
    NSLog(@"IID %@", result.token);
  }];

The output from running the above code (on simulator) is always an instance ID token. Here's the source for the instance ID method:

- (void)instanceIDWithHandler:(FIRInstanceIDResultHandler)handler {
  FIRInstanceID_WEAKIFY(self);
  [self getIDWithHandler:^(NSString *identity, NSError *error) {
    FIRInstanceID_STRONGIFY(self);
    // This is in main queue already
    if (error) {
      if (handler) {
        handler(nil, error);
      }
      return;
    }
    FIRInstanceIDResult *result = [[FIRInstanceIDResult alloc] init];
    result.instanceID = identity;
    NSString *cachedToken = [self cachedTokenIfAvailable];
    if (cachedToken) {
      if (handler) {
        result.token = cachedToken;
        handler(result, nil);
      }
      // If no handler, simply return since client has generated iid and token.
      return;
    }
    [self defaultTokenWithHandler:^(NSString *_Nullable token, NSError *_Nullable error) {
      if (handler) {
        if (error) {
          handler(nil, error);
          return;
        }
        result.token = token;
        handler(result, nil);
      }
    }];
  }];
}

Can you share an example project that reproduces this issue?

@jasongiss
Copy link
Author

Thank you, @morganchen12.

I already provided a project showing the issue.
There is a link to download it in the 'steps to reproduce' section of my initial report.

I was using an actual device - iPad running iOS 12.x (not sure on the exact dot version at the moment).

Please let me know if you need any more than this from me.

@morganchen12
Copy link
Contributor

I've been able to repro the issue on device, and I've submitted a fix. Thanks for the clear repro example!

@jasongiss
Copy link
Author

That's brilliant! Thank you so much, @morganchen12 - I really appreciate your effort.

@maksymmalyhin maksymmalyhin added this to the M51 milestone Jun 25, 2019
@firebase firebase locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants