[v5] [iOS] Register for remote notifications fix

[mcuelenaere]

Summary

We've observed crashes on iOS 13 when calling RNFirebaseMessaging registerForRemoteNotifications:rejecter: in the wild.

When checking the code, registerForRemoteNotifications is not being called from the main thread (while 5 lines above it, in requestPermission, it is being done so).

This also corresponds with the crash stack traces we've seen, none of them are on the main thread.

Bottom line: calling this method from the main thread has no negative impact and should hopefully fix these crashes.

Checklist

• Supports Android
• Supports iOS
• e2e tests added or updated in packages/**/e2e
• Flow types updated
• Typescript types updated

Test Plan

Call registerForRemoteNotifications and notice app not crashing.

Release Plan

[IOS][BUGFIX] [MESSAGING] - Register for remote notifications should run on main thread

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Donate via Open Collective
• 👉 Follow React Native Firebase and Invertase on Twitter
• 👉 Star this repo on GitHub ⭐️
• 👉 Contribute; see our contributing guide

[mcuelenaere]

Further evidence that proves this should be called from the main thread: https://stackoverflow.com/questions/44391367/uiapplication-registerforremotenotifications-must-be-called-from-main-thread-o

[mikehardy]

Very interesting issue - thanks for the PR, and the background link

I investigated the CI failure

I expect ci.android.test to fail right now for unrelated reasons, and you obviously didn't change anything that would affect it. But ci.ios.test failed as well which is unexpected. The CI log had an error that was related to a timeout in firebase.messaging().onTokenRefresh which is in the area, so this might be either a legitimate failure based on this change, or an assumption the test was making that is invalid - failing the test now - but not because your change is wrong.

I simply re-triggered the build so we could see if it a transient network blip causing the timeout, or if it's a repeatable issue and warrants deeper inspection

[mikehardy]

Ok, there is definitely a problem here:

[15:26:23]	[Step 3/5]  messaging()
[15:26:23]	[Step 3/5]  requestPermission()
[15:26:23]	[Step 3/5]  ✓ returns fcm token
[15:26:23]	[Step 3/5]  hasPermission()
[15:26:23]	[Step 3/5]  ✓ returns fcm token
[15:26:23]	[Step 3/5]  RemoteMessage
[15:26:23]	[Step 3/5]  ✓ builds a remote message
[15:26:23]	[Step 3/5]  sendMessage()
[15:26:23]	[Step 3/5]  ✓ sends a message
[15:26:23]	[Step 3/5]  subscribeToTopic()
[15:26:23]	[Step 3/5]  ✓ subscribe without error
[15:26:23]	[Step 3/5]  unsubscribeFromTopic()
[15:26:23]	[Step 3/5]  ✓ unsubscribe without error
[15:26:23]	[Step 3/5]  getToken()
[15:26:23]	[Step 3/5]  ✓ returns fcm token
[15:26:23]	[Step 3/5]  onTokenRefresh()
[15:28:23]	[Step 3/5] ⚠️ A test failed:
[15:28:23]	[Step 3/5] 
[15:28:23]	[Step 3/5] ️ -> triggers when token changes
[15:28:23]	[Step 3/5] ️ -> Retrying... (1)
[15:30:26]	[Step 3/5]  Retry #1 failed...
[15:30:26]	[Step 3/5] ️ -> Retrying... (2)
[15:32:29]	[Step 3/5]  1) triggers when token changes
[15:32:29]	[Step 3/5] 

what and why I can't speculate - as mentioned above it could be the test itself is making an incorrect assumption, it could be the code change here broke something legitimate, or it could just be that bitrot/entropy took over and the tests no longer pass on the current 5.5.6 release - but it is consistently failing right now which unfortunately makes this unreleasable as is

patch-package is of course available for local needs while it is sorted out - your change in general looks reasonable so I'm surprised to see this failure

[mikehardy]

For completeness I will say that I successfully ran the e2e tests locally a while back but it has been a long long time. If you carefully follow the README in the v5.5.x tests app for setting up the iOS side of the e2e tests it should be possible to run it for the current public release (5.5.6) to prove it can work, then integrate this change and see what happened?

[mcuelenaere]

It seems like the tests are flaky. When running them locally, initially the same one failed as well.
After adding some debugging statements to the iOS code, suddenly they started to succeed.

So I tried a clean run and it ran perfectly: https://gist.githubusercontent.com/mcuelenaere/597d8d1240f5c95ac7b9345642c70cf3/raw/af8b1da910afcc8a3a69be9a203e9809c088bd86/gistfile1.txt

Can you trigger a rebuild on the CI environment?

[mikehardy]

Well this is frustrating - those specific tests are a bit flaky but this is failing each time for the last 4 times I have re-run that build step just now. At the same time I read your test session (thank you!) and of course it looks fine. It needs a bit deeper inspection but the actual change does look fine

[yilinjuang]

Got runtime warning in xcode

-[UIApplication registerForRemoteNotifications] must be used from main thread only

This pr makes a lot of sense. @mikehardy Can we have it tested again and merge?

[mikehardy]

@frankyjuang This does make sense - it's targeted enough and causing crashes I could see doing a patch release on v5, I'll put it in my queue and scour the rest of the issues/PRs to see if there's anything else worth putting in there.

In the meantime (since it might take a while, sorry) please check out https://github.com/ds300/patch-package it is ace for this situation. Now that we have code that seems to work you can just patch it in, safely, instead of waiting for a maintainer here.

I would also love to hear that someone else has patched it in and it's working for them since v5 testing is clearly problematic in this method

[yilinjuang]

Thanks @mikehardy really appreciated. Will try the patching.

[russellwheatley]

Hey @mcuelenaere, thank you for your pull request. We’re no longer actively supporting <= v5 development so I’m going to go ahead and close this. If this PR change still applies to the latest version of React Native Firebase then please feel free to raise a new PR targeting the latest version with your changes.