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

Push notifications not received in iOS debug builds #437

Closed
shankari opened this issue Jul 20, 2019 · 27 comments
Closed

Push notifications not received in iOS debug builds #437

shankari opened this issue Jul 20, 2019 · 27 comments

Comments

@shankari
Copy link
Contributor

While rebuilding the app to test the local changes required for #433 et al, we stopped getting the silent push notifications on the iOS phones. Fixing this was a lot more complex than I originally thought, so documenting this just in case I have to revert and come back to this later.

@shankari
Copy link
Contributor Author

shankari commented Jul 20, 2019

Push notifications are not delivered in debug builds due to https://issues.apache.org/jira/browse/CB-14264 (Error while reading embedded mobileprovision) This is fixed in cordova-ios@5.0.0 although there is a workaround listed as well.
Trying to upgrade to cordova-ios@5.0.0 since we are in here anyway.

@shankari
Copy link
Contributor Author

Build failure after upgrade. Failure is at this code fragment because

/Users/shankari/e-mission/e-mission-base/platforms/ios/emTripLog/Plugins/de.appplant.cordova.plugin.local-notification-ios9-fix/AppDelegate+APPLocalNotificationAction.m:40:64: Use of undeclared identifier 'CDVLocalNotification'

at this code

    NSLog(@"Received local notification %@ while in the foreground, reposting", notification);
    [[NSNotificationCenter defaultCenter] postNotificationName:CDVLocalNotification object:notification];

which was added by
shankari/cordova-plugin-local-notifications@f091e73

@shankari
Copy link
Contributor Author

The change from 4.5.4 to 5.0.0 removed CDVLocalNotification in this change
apache/cordova-ios@6465a94
because it is apparently deprecated.

Let's check the plugin and see what happens about the constant

@shankari
Copy link
Contributor Author

Ah, the most recent version of the plugin, which has been significantly updated, only works for iOS 10+
while we currently support iOS 9+

https://github.com/katzer/cordova-plugin-local-notifications/blob/master/plugin.xml

        <engine name="apple-ios"       version=">=10.0.0" />

So just updating to the latest version of the plugin won't work.

@shankari shankari changed the title Push notifications not received in debug builds Push notifications not received in iOS debug builds Jul 20, 2019
@shankari
Copy link
Contributor Author

shankari commented Jul 20, 2019

This is not working even after the upgrade to cordova-ios@5.0.0

bash-3.2$ cordova platform ls
Installed platforms:
  android 6.4.0
  ios 5.0.0

I get a server side error

DEBUG:root:firebase push result for ios: success 0 failure 1 results [{'error': 'InvalidParameters: Failed to decode APNS token: s_e5Wfh829KIE:...'}]

The actual APNS token, as verified by the phone debug logs is e5Wfh829KIE.... (i.e. without the initial s_). Trying now with a release build through TestFlight, but I wonder if @jf87 and @DO3B saw similar issues in their testing?

@shankari
Copy link
Contributor Author

ok so after poking around the push plugin issue list, it looks like the notification token that is generated an FCM token, not an APNS token. According to phonegap/phonegap-plugin-push#2784 APNS tokens look like this

864CD0D436C9FD41695B59584437EC57B4F4CF94A4A604C20F9852475FAAB540

while the token that we get looks like this

e0jPBtF9a3M:APA91bE6pC0HPMWFYDa5DDYzArhpF3klaZDdaTsOYRHo4FF1UfgM-c4TBYGJqHwjTNybDBUCP-zf6C_B72u6VWZ54kHoDLKsWcz-YKmAvg5CVvgdAvBRUbP5Bbf0LPFL_R5TVXhIFhHC

Now that I look at it more carefully, the token is also of type "FCM".

2019-07-20 14:18:29.748053-0700 emTripLog[6681:3047817] DEBUG: Token = {"token":"e0jPBtF9a3M:APA91bE6pC0HPMWFYDa5DDYzArhpF3klaZDdaTsOYRHo4FF1UfgM-c4TBYGJqHwjTNybDBUCP-zf6C_B72u6VWZ54kHoDLKsWcz-YKmAvg5CVvgdAvBRUbP5Bbf0LPFL_R5TVXhIFhHC","type":"FCM"}

Let's try the fix in phonegap/phonegap-plugin-push#2784 and also investigate further.

@shankari
Copy link
Contributor Author

shankari commented Jul 20, 2019

The fix is consistent with the code.
And the fix worked!

Couple of notes in case this helps somebody else - since this is a debug build, we need to use the sandbox APNS server, which means that we need to use the -d option - e.g.

(emission) $ ./e-mission-py.bash bin/push/push_to_users.py -e <email> -t Test Visible -d

Under the hood, using -d passes in the dev parameter, which we pass to FCM to let it know whether this is a sandbox token or not. If you don't do this, you will get a NotRegistered error.

DEBUG:root:{'ios': {'multicast_ids': [...], 'success': 0, 'failure': 1, 'canonical_ids': 0, 'results': [{'error': 'NotRegistered'}], 'topic_message_id': None}, 'android': {'multicast_ids': [], 'success': 0, 'failure': 0, 'canonical_ids': 0, 'results': [], 'topic_message_id': None}}

@shankari
Copy link
Contributor Author

FYI: I am not sure how this ever worked. I checked the GoogleService-Info.plist on e-mission-phone, which I didn't edit and it has

    <key>IS_GCM_ENABLED</key>
    <true></true>

Maybe this is because I didn't update the version of the iOS app in the stores after the GCM -> FCM switch (version updated Oct 31, 2018) but surely the others who tested this must have run into it...

@shankari
Copy link
Contributor Author

Switching to cordova-ios@5.0.0 also seems to remove the dependency on the legacy build system. Will have to test a bit more, and see if this is what others encounter as well. But that is a strong reason to make the switch.

@shankari
Copy link
Contributor Author

On further testing, it looks like switching to cordova-ios@5.0.0 breaks ionic deploy. The redirect works after the first install, but does not work subsequently although the redirect code is called correctly both times.

First run

2019-07-20 18:26:22.669662 -0700	emTripLog	Redirecting to: file:/var/mobile/Containers/Data/Application/4538DF79-7AFC-4D22-8CBC-A14788DB12F4/Library/Application%20Support/34347c79-eb0b-4442-8694-67910c993c7e/index.html?cordova_js_bootstrap_resource=/var/containers/Bundle/Application/F6D8CB5F-9AAC-4EE5-B470-D130300B3B3E/emTripLog.app/www/cordova.js

Second run

2019-07-20 18:29:37.159924 -0700	emTripLog	Redirecting to: file:/var/mobile/Containers/Data/Application/4538DF79-7AFC-4D22-8CBC-A14788DB12F4/Library/Application%20Support/34347c79-eb0b-4442-8694-67910c993c7e/index.html?cordova_js_bootstrap_resource=/var/containers/Bundle/Application/F6D8CB5F-9AAC-4EE5-B470-D130300B3B3E/emTripLog.app/www/cordova.js

This is kind of a big deal because we use deploy for emevalzephyr :)

@shankari
Copy link
Contributor Author

Is this a race, it looks like we finish the reload before the platform was ready. In this case, how did this ever work with the older version? That plugin should also have finished before all the plugins were ready. Or is this some subtle change to plugin init complete behavior in cordova-ios@5.0.0?

@shankari
Copy link
Contributor Author

shankari commented Jul 21, 2019

Probably a subtle change to plugin init behavior in cordova-ios@5.0.0
On further debugging, it turned out that the first redirect was called before all plugins initialized, and it set the
self.currentUUID = uuid;
but the actual reload never completed. And because the currentUUID was set, no further reloads would succeed.

I worked around this by commenting out the doRedirect code in pluginInitialize and manually adding it to the javascript. iOS works now and android (since we did not update the plugins, never had a issue). But this is definitely a hack and we should really consider updating a bunch of the plugins as well.

@shankari
Copy link
Contributor Author

This is the last hack to work with cordova-ios@5.0.0. If I have to make any more hacks, I am reverting to cordova-ios@4.5.4

@shankari
Copy link
Contributor Author

Just a quick comment on the race -

                NSLog(@"Redirecting to: %@", components.URL.absoluteString);
                dispatch_async(dispatch_get_main_queue(), ^(void) {
                    NSLog(@"Reloading the web view.");
                    SEL reloadSelector = NSSelectorFromString(@"reload");
                    ((id (*)(id, SEL))objc_msgSend)(self.webView, reloadSelector);
                    [self.webViewEngine loadRequest:[NSURLRequest requestWithURL:components.URL]];

from this code, we see

2019-07-20 18:29:37.159924 -0700	emTripLog	Redirecting to: file:/var/mobile/Containers/Data/Application/4538DF79-7AFC-4D22-8CBC-A14788DB12F4/Library/Application%20Support/34347c79-eb0b-4442-8694-67910c993c7e/index.html?cordova_js_bootstrap_resource=/var/containers/Bundle/Application/F6D8CB5F-9AAC-4EE5-B470-D130300B3B3E/emTripLog.app/www/cordova.js
...
2019-07-20 18:29:37.659989 -0700	emTripLog	Reloading the web view.
2019-07-20 18:29:39.510163 -0700	emTripLog	DEBUG: ionicPlatform is ready

So there do not appear to be any restrictions on background operation; it is just that we reload the web view before all plugins are complete. Are plugins slower in the new cordova version? Are they executed in a different order? I am not sure, and of course, this is going to fall firmly into the category of wont-fix for ionic.

@shankari
Copy link
Contributor Author

And just to confirm, with the hacky fix, updates do work smoothly without resetting the UI.

@shankari
Copy link
Contributor Author

Closing this since hacky fix works

@shankari
Copy link
Contributor Author

I don't have the time to investigate in great detail right now, but this seems to have regressed android. I have frequently encountered the behavior that when launched from a cold start, the android UI load hangs. After waiting for ~ 1 hour, it shows up. I have multiple logs, and I have looked through at least one of them carefully, but I don't see anything indicating why the error occurred.

Will just have to try to reproduce later with a more careful accounting of when the failures were detected. Reopening this for now....

@shankari shankari reopened this Mar 30, 2020
@shankari
Copy link
Contributor Author

In the interim, will just not enable ionic deploy on iOS

@shankari
Copy link
Contributor Author

It turns out that we have to switch to iOS 5 anyway because of the need to switch to WKWebView in order to upload apps to the app store. So let us continue to work through this and document our findings.

@shankari
Copy link
Contributor Author

I am now back at the failure due to the UILocalNotification code, but this time I am going to try upgrading to the most recent version of the plugin. I already documented that we were going to move to iOS10 (#438) and haven't seen any objections.

@shankari
Copy link
Contributor Author

that fixes at least the compile problem. Next issue is that cocoapods are broken after the upgrade to Catalina. The first break was with auth, and I removed that since it is fairly self-contained. But then the comm plugin also needs GTMSessionFetcher/GTMSessionFetcherService.h which is a podspec.

./platforms/ios/Pods/GTMSessionFetcher
./platforms/ios/Pods/GTMSessionFetcher/Source/GTMSessionFetcherLogging.m
./platforms/ios/Pods/GTMSessionFetcher/Source/GTMSessionFetcher.h
./platforms/ios/Pods/GTMSessionFetcher/Source/GTMSessionFetcherService.m
./platforms/ios/Pods/GTMSessionFetcher/Source/GTMSessionFetcherLogging.h
./platforms/ios/Pods/GTMSessionFetcher/Source/GTMSessionFetcher.m
./platforms/ios/Pods/GTMSessionFetcher/Source/GTMSessionFetcherService.h

Removing comm would be problematic, since other modules have dependencies on it and I don't want to go down that rabbit hole. And eventually, we may want to include auth anyway. Let's solve this now.

@shankari
Copy link
Contributor Author

For the record, since I was curious about this, we don't explicitly list the GTMSessionFetcher as a podspec for the comm module. The full set of explicitly specified pods is

  • FirebaseMessaging
  • GoogleSignIn
  • AppAuth
  • JWT

However, the session fetcher is a dependency for GoogleSignIn

  - GoogleSignIn (4.0.2):
    - "GoogleToolboxForMac/NSDictionary+URLArguments (~> 2.1)"
    - "GoogleToolboxForMac/NSString+URLArguments (~> 2.1)"
    - GTMOAuth2 (~> 1.0)
    - GTMSessionFetcher/Core (~> 1.1)
...
  - GTMOAuth2 (1.1.6):
    - GTMSessionFetcher (~> 1.1)

We should really add it as an explicit dependency for comm in case modularize auth further and want to remove GoogleSignIn later.

@shankari
Copy link
Contributor Author

We have had a ton of build/dependency issues, so I really want to enable reproducible setup and keep that ongoing checking going. I found this site on how to enable CI for iOS builds
https://andreaslydemann.com/continuous-integration-using-github-actions-for-ios-projects/
which uses mint instead of brew. I wonder if it is worth creating a setup.sh for the phone projects as well.

@shankari
Copy link
Contributor Author

So the CI example there uses mint + brew + gems. That seems like a lot. Exploring whether we can standardize on one thing (e.g. brew)

For the record, the current failure is:

-bash: /usr/local/bin/pod: /System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/bin/ruby: bad interpreter: No such file or directory

@shankari
Copy link
Contributor Author

It looks like homebrew takes ~ 7G. Tempted to uninstall it and then reinstall the parts that I want. Will uninstall mongodb, but maybe that will inspire me to use docker more often.

shankari $ du -sh /usr/local/
7.6G	/usr/local/

@shankari
Copy link
Contributor Author

ok so I think that what I need is:

  • brew to install xcode and node
  • gem to install cocoapods
  • npm to install the other modules

shankari added a commit to shankari/e-mission-phone that referenced this issue Jul 13, 2020
So that the workaround in
e-mission/e-mission-docs#437 (comment)
doesn't need to be manually applied any more
@shankari
Copy link
Contributor Author

Reproducible setup complete: e-mission/e-mission-phone#700, e-mission/e-mission-phone#701
Enabling APNS tokens complete: e-mission/e-mission-phone@0ab3357, committed as part of e-mission/e-mission-phone#702

Closing this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant