notifications generated by background checks don't open the app correctly #712
Comments
|
Continuing the tracking to see how often it happens (from #710)
So it does not work for the majority of the time. |
|
Let's try and generate the notification from a UI thread to confirm that it does work reliably in that case.
Tried ~ 10 trials, all of them worked |
|
the entries are saved into the I also use SharedPreferences to store the current state machine state, and I remember switching from
That's why I moved from Then, we need to figure out how to incorporate it into the plugin... |
|
FYI, there are multiple locations where we use There are also multiple other places where we use |
|
Testing with commit from the background:
Let's briefly see if the toast is visible right after it is created. Otherwise, we may want to store in local storage instead. |
|
notification was saved on create worked |
|
ok, hit the failure case again, we are able to retrieve the notification right after creation but fails when clicked notification generated again clicked again The id from the bundle is correct, but it cannot find any matches. The SharedPreferences are null. |
|
Now the background sync process is not even running reliably so it is super hard to test. A fairly easy workaround is to store the information in the database, similar to e-mission/e-mission-phone@a09385b on iOS. However, I am concerned that we also store the FSM state in shared preferences, and if there's something funky about it, it could cause us to fail in unpredictable ways. Let's spend a little more time working on the reason, then just track if the current state is null and move on to storing in the database. |
|
Ha right after that, noticed that the sync had run in the During sync, we were able to read the notification And when we clicked, we were not Although we had the exact same copy pasted code in both locations I do note that the click reciever uses the application context while the background checker uses the passed in context Let's see if that is the difference. If that is the problem, the database may not work either. Let's try changing the context. |
|
After restart, the first call works with both contexts, which is not unusual. True test is tomorrow morning when the contexts will presumably start differing |
|
Next morning:
7:31/7:32: again didn't work Trying the database to store info now |
|
Wait, I retrieved the app context but not actually passed it in to the creator Fixed and restarted at 7:38 |
Note that I also see that the logs with the application context show up in the e-mission process and the logs with the regular context show up in the emission:sync process. Note that this worked (and works) in the transition notifier code (https://github.com/e-mission/e-mission-transition-notify/blob/master/src/android/TransitionNotificationReceiver.java#L79) (`onRecieve -> fireGenericTransition -> postNativeAndNotify), which is why I assumed it would work here. But that is launched from a receiver, which doesn't create a new process. This is launched from a sync service, which does.
https://github.com/e-mission/cordova-server-sync/blob/master/plugin.xml#L67 |
|
Next steps:
Open question:
Decision: Right now, we don't touch the plugin. File a new issue to handle the crash, if we have to change the code around that anyway, we might as well change apply to commit at that time. |
|
Reverted changes by reinstalling plugins, starting test... 10:32: turned off motion activity permissions |
|
Made changes to the notification handler, starting test... 10:54: deployed changes Having issues with the background process, may run on a physical device instead for improved testing. |
|
Uninstalled and reinstalled 11:50: worked This is because the app context is apparently not the same when invoked from the sync code. Worked
Did not work
|
|
Printing out the application context, we get
in the sync So it looks like the contexts are different, and the log is printed in both?! What the heck are the two contexts? worked. |
|
Looks like we call the background checker twice and each call has a different application context?! |
|
The sync process has
The emission process has the previous invocation only.
Only one call in process ID 19147, which generates the same application context So it shouldn't work? Yup. |
|
So the sync process is the same The emission process shows up too So it should work this time. And it did And the application context is indeed the same as emission. Next time, there were no logs in the emission process and it didn't work |
|
Now we have a bunch of options:
Let's first see if the first option works (to prove to ourselves that we understand the problem). |
|
On physical phone (to ensure that the task runs every 10 minutes)
Phone disconnected, so no logs, but we know what is going on. |
|
Without sync as a separate process (in the emulator):
This works, but it looks like it is only invoked once an hour. Deployed onto physical phone to avoid issue with background operation on emulator
|
|
Reinstalled at around 4:30 with a 10 min cadence Emulator:
Physical device:
So it seems pretty clear that this works. |
|
Tried with the physical device multiple times over the weekend and they all worked. |
|
Has been working in the emulator all morning And has also worked consistently in the physical device over the weekend. This is definitely one solution. |
|
Before we decide that this is the solution, let us look at the crashes related to accessing shared preferences while the screen is locked. |
|
This has a fix that is both in the master branch and in the timkelleypa fork We should switch to one of them. Let's try timkelleypa, which seems to have some more recent updates, although they are still from 2020. |
|
timkelleypa seems to build and not have any regressions. So we now need to make the final decision on whether:
Since we don't need to switch to local storage to fix the crash, and we don't have a principled reason for using a separate process for the sync[1], let's go with the first option. The only downside that I can think of is that then a crash in the sync will crash the app process. But the app process can always be killed due to memory issues, and we have to remember to restart it. So this does not appear to significantly impact robustness. [1] other than "that's what the content provider template used" |
|
Testing on physical device before pushing the changes. |
|
6:37: notification works on emission, does not open correctly on CanBikeCO I claim that this is fixed and will push out the change tonight |
So that notifications generated from sync open the app properly and launch the status screen. This implements e-mission/e-mission-docs#712 (comment)
- Update the sync plugin so that it doesn't run in a separate process - Update the local notification plugin so that it doesn't crash on startup if the screen is locked Both of these are tracked in e-mission/e-mission-docs#712
Google finally started enforcing background restrictions on its apps
The background check was not re-entrant, so we ran into
#710
After fixing that, I got a notification that the background restrictions were not set correctly.
On clicking it, however, the app was not launched correctly; instead a separate activity was created
After much debugging, it turned out that this was because when we click on the notification, the local notification plugin tries to read the notification config from SharedPreferences.
#710 (comment)
Moving the notification generation into the UI thread did not work
#710 (comment)
So I looked to see if this was also a new regression, and:
Note that I had to run this on an emulator which hadn't been upgraded to enforce the background restrictions to avoid hanging. But the fact that it happens for a different permission and without running in the background indicates that there is likely a more substantial issue here.
So tracking it in a separate issue.
This is not a fire drill like the other issue, but it would be good to fix before reaching out to participants to get them to fix the app.
Since this is an issue with the local notification plugin, we may want to fix the crashes around accessing the shared preferences at the same time.
The text was updated successfully, but these errors were encountered: