e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 32 forks source link

Update targetSdk to 26 and switch to dynamic permissions #325

Closed shankari closed 5 years ago

shankari commented 5 years ago

starting November 1, 2018, updates to apps and games on Google Play will be required to target Android Oreo (API level 26) or higher. After this date, the Play Console will prevent us from submitting new APKs with a targetSdkVersion less than 26.

Supporting apk version 26 requires us to switch to dynamic permissions. This is not a huge issue since we already support dynamic permissions on iOS, but it does require non-trivial implementation and testing time.

It is not a coincidence that the last update of e-mission and emTripLog were updated in Oct 2019.

However, as other groups, notably @jf87 @deepalics0044 @PatGendre want to deploy custom apps to the store, we need to fix this pretty soon.

shankari commented 5 years ago

this is a cordova app, so each plugin that we choose to include can have permissions, in addition to the permissions that the native plugins that I wrote. So let's start with a list of all permissions, figure out the plugins that add them (and whether they are really necessary), and classify them into runtime versus compile time.

This is the full list of permissions.

$ grep "uses-permission" platforms/android/AndroidManifest.xml
    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.GET_ACCOUNTS" />
    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
    <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" />
    <uses-permission android:name="android.permission.MANAGE_ACCOUNTS" />
    <uses-permission android:name="com.google.android.gms.permission.ACTIVITY_RECOGNITION" />
    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
    <uses-permission android:name="android.permission.READ_SYNC_SETTINGS" />
    <uses-permission android:name="android.permission.WRITE_SYNC_SETTINGS" />
    <uses-permission android:name="android.permission.AUTHENTICATE_ACCOUNTS" />
    <uses-permission android:name="android.permission.CAMERA" />
    <uses-permission android:name="android.permission.FLASHLIGHT" />
    <uses-permission android:name="android.permission.WAKE_LOCK" />
    <uses-permission android:name="android.permission.VIBRATE" />
    <uses-permission android:name="com.google.android.c2dm.permission.RECEIVE" />
    <uses-permission android:name="${applicationId}.permission.C2D_MESSAGE" />
    <uses-permission android:name="${applicationId}.permission.PushHandlerActivity" />
shankari commented 5 years ago
Permission Plugin Needed? Dangerous
android.permission.INTERNET edu.berkeley.eecs.emission.cordova.serversync, phonegap-plugin-push Yes Normal
android.permission.GET_ACCOUNTS cordova-plugin-email-composer, edu.berkeley.eecs.emission.cordova.auth ? Dangerous
android.permission.WRITE_EXTERNAL_STORAGE cordova-plugin-file, de.appplant.cordova.plugin.local-notification-ios9-fix, edu.berkeley.eecs.emission.cordova.auth, cordova-plugin-x-socialsharing ? Dangerous
android.permission.RECEIVE_BOOT_COMPLETED de.appplant.cordova.plugin.local-notification-ios9-fix, edu.berkeley.eecs.emission.cordova.datacollection Yes Normal
android.permission.MANAGE_ACCOUNTS edu.berkeley.eecs.emission.cordova.auth N/A Removed in API 22+
com.google.android.gms.permission.ACTIVITY_RECOGNITION edu.berkeley.eecs.emission.cordova.datacollection Yes Seems to be normal, but unsure
android.permission.ACCESS_FINE_LOCATION edu.berkeley.eecs.emission.cordova.datacollection Yes Dangerous
android.permission.READ_SYNC_SETTINGS edu.berkeley.eecs.emission.cordova.serversync Yes normal
android.permission.WRITE_SYNC_SETTINGS edu.berkeley.eecs.emission.cordova.serversync Yes normal
android.permission.CAMERA phonegap-plugin-barcodescanner ? Need to check most recent version
android.permission.FLASHLIGHT phonegap-plugin-barcodescanner ? Removed in API level 24
android.permission.WAKE_LOCK phonegap-plugin-push Need to see what the most recent version needs normal
android.permission.VIBRATE phonegap-plugin-push Yes normal
com.google.android.c2dm.permission.RECEIVE phonegap-plugin-push GCM is deprecated ???
${applicationId}.permission.C2D_MESSAGE phonegap-plugin-push GCM is deprecated ???
${applicationId}.permission.C2D_MESSAGE phonegap-plugin-push GCM is deprecated ???
shankari commented 5 years ago

ok so some obvious fixes are:

shankari commented 5 years ago

see whether barcodescanner has a more recent update

We are at 7.1.0; most recent release is at https://github.com/phonegap/phonegap-plugin-barcodescanner/releases/tag/v8.0.1

The changes don't seem significant; we might as well update to address backwards compatibility. https://github.com/phonegap/phonegap-plugin-barcodescanner/compare/v7.1.0...v8.0.1

The most recent version also has FLASHLIGHT; will file an issue to remove it since it is not needed any more.

Filed https://github.com/phonegap/phonegap-plugin-barcodescanner/issues/769

shankari commented 5 years ago

upgrade phonegap-plugin-push

Current 1.x release is v1.11.1 Current 2.x release is v2.2.3 We are at 1.9.2

Trying to move to v2.2.3. Do I need to switch to a fork with https://github.com/phonegap/phonegap-plugin-push/pull/2369 included? I think we are fine, because I already split out the android v/s ios tokens and send a data-only message to the android tokens. https://github.com/e-mission/e-mission-server/blob/master/emission/net/ext_service/push/notify_interface_impl/firebase.py#L116

shankari commented 5 years ago

ok so after upgrading, downloading the google-services.json and GoogleService-Info.plist and adding links to them in the config.xml as per the installation instructions, I get the compilation error https://github.com/phonegap/phonegap-plugin-push/blob/master/docs/INSTALLATION.md

shankari commented 5 years ago

This is clearly related to the compatibility library. I checked the support repository against the instructions, and I am at 47+

screen shot 2019-03-07 at 4 43 14 pm
shankari commented 5 years ago

Looking at the support plugins included with various plugins,

plugins/cordova-plugin-email-composer/plugin.xml:        <framework src="com.android.support:support-v4:24.1.1+" />
plugins/cordova-plugin-x-socialsharing/plugin.xml:    <framework src="com.android.support:support-v4:24.1.1+" />
plugins/de.appplant.cordova.plugin.local-notification-ios9-fix/plugin.xml:        <framework src="com.android.support:support-v4:+" />
plugins/edu.berkeley.eecs.emission.cordova.unifiedlogger/plugin.xml:    <framework src="com.android.support:support-v4:+" />
plugins/phonegap-plugin-barcodescanner/plugin.xml:    <framework src="com.android.support:support-v4:$ANDROID_SUPPORT_V4_VERSION"/>
plugins/phonegap-plugin-push/plugin.xml:    <framework src="com.android.support:support-v13:26.+"/>

The rest of them are v4 and this is v13. Let's look at the v4 versus v13 documentation.

shankari commented 5 years ago

Looking at the NotificationCompat documentation, https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

NotificationCompat.Builder(Context context) was deprecated in API 26.1.0. But that is what this code uses

        mBuilder = new NotificationCompat.Builder(context, channelID);
shankari commented 5 years ago

so that code was changed as part of https://github.com/phonegap/phonegap-plugin-push/issues/1949 Apparently, if the app targets sdk 26+ (which we will have to), then notifications won't be received unless we use the new form of the notification builder.

This may imply that we need to update all the other plugins too. Argh!

shankari commented 5 years ago

According to this https://stackoverflow.com/questions/45462666/notificationcompat-builder-deprecated-in-android-o we may be able to set the channel id separately instead. But we still need to set the channelId...

shankari commented 5 years ago

opening the NotificationCompat class and checking to see where it is from, I find ~/Library/Android/sdk/extras/android/m2repository/com/android/support/support-compat/25.3.1/support-compat-25.3.1-sources.jar

why isn't the project using version 26 or 27 instead?

shankari commented 5 years ago

Aha! If I look at the SDK manager https://github.com/e-mission/e-mission-docs/issues/325#issuecomment-470759414, it does have the Android Support Repository at 47.0.0 installed, but the Android Support Library is only 23.1.0. The example in the installation instructions does not have an entry for the Android Support Library.

Hm, let's try uninstalling the library first...

shankari commented 5 years ago

If I uninstall the library, the entry disappears, but the error persists. Checking the compat directory, I still see

ls ~/Library/Android/sdk/extras/android/m2repository/com/android/support/support-compat/
24.2.0          25.1.1          maven-metadata.xml
24.2.1          25.2.0          maven-metadata.xml.md5
25.0.0          25.3.0          maven-metadata.xml.sha1
25.0.1          25.3.1
25.1.0          26.0.0-alpha1

And I can't see any support library entries so that I can install the latest version. Apparently, this is a known issue - e.g. https://stackoverflow.com/questions/12382555/android-support-library-manual-download

shankari commented 5 years ago

digging a bit further, the new method is was added in version 26.1.0 https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html#notificationcompat.builder_1

According to the libraries tab in android studio, we have three sets of support libraries. https://stackoverflow.com/questions/25233510/how-to-find-the-version-of-library-from-gradle-dependency-android-studio

Gradle__com_android_support_support_v4_25_3_1_aar.xml
Gradle__com_android_support_support_v13_23_4_0_aar.xml
Gradle__com_android_support_support_v13_26_1_0_aar.xml

But I don't actually see the version 26.1.0 version in .../android/m2repository/com/android/support/support-v13/maven-metadata.xml

but it is in ~/.gradle/caches/modules-2/files-2.1/com.android.support/support-v13/26.1.0/. Why does gradle pick the one in maven instead of the one in gradle?

Because for some of them, the sources are in the local library.

    <SOURCES>
      <root url="jar://$USER_HOME$/Library/Android/sdk/extras/android/m2repository/com/android/support/support-compat/25.3.1/support-compat-25.3.1-sources.jar!/" />
    </SOURCES>

I'm going to try upgrading gradle since I keep getting Android Studio warnings about it. Although I am kind of paranoid about making the change since it is not easily reversible.

The warning says

To take advantage of the latest features, improvements, and security fixes, we strongly recommend that you update the Android Gradle plugin to version 3.3.1 and Gradle to version 4.10.1.

shankari commented 5 years ago

I figured it out and it was really annoying. Basically, in edu.berkeley.eecs.emission.cordova.auth/embase-openid-config.gradle, we set

configurations.all {
    resolutionStrategy.force 'com.android.support:support-v4:25.3.1'
}

This was what was forcing us to use 25.3.1 although 26.1.0 was available. I changed that to

configurations.all {
    resolutionStrategy.force 'com.android.support:support-v13:26.1.0'
}

Next, I ran into a multi-dex error, caused by a conflict between

    compile "com.google.android.gms:play-services-location:10.2+"
    compile "com.google.firebase:firebase-messaging:11.0.1"

There is not much of an explanation for the bump https://github.com/phonegap/phonegap-plugin-push/commit/576135bf576be6e2cd183101d3d7219b4d297030

After making both of them be the same version, the build succeeds. So the next step is to figure out whether we should downgrade firebase-messaging or upgrading the location services. I am tempted to upgrade the location services, although I should remember to update the motion activity code to handle the new field names...

shankari commented 5 years ago

finally getting back to this after dealing with other issues all morning. we should start with upgrading location services since:

shankari commented 5 years ago

see whether we are really using the file plugin, what for, and whether it needs that permission

The file plugin is used at

www//js/services.js:            parentDir = cordova.file.dataDirectory+"../LocalDatabase";
www//js/recent.js:            parentDir = cordova.file.dataDirectory+"../LocalDatabase";

and they are both for emailing logs/data out from the app, to determine the location of the database file on iOS. So changes in android permissions should not affect us.

services.js

  this.emailLog = function() {
....
        if (ionic.Platform.isIOS()) {
            alert("You must have the mail app on your phone configured with an email address. Otherwise, this won't work");
            parentDir = cordova.file.dataDirectory+"../LocalDatabase";
        }
        alert("Going to email database from "+parentDir+"/loggerDB");
  }

recent.js

$scope.emailCache = function() {
    if (ionic.Platform.isIOS()) {
        alert("You must have the mail app on your phone configured with an email address. Otherwise, this won't work");
        parentDir = cordova.file.dataDirectory+"../LocalDatabase";
    }
    alert("Going to email database from "+parentDir+"/userCacheDB");

}

We are using version 6.0.1. Current version is 6.0.1. It supports dynamic permissions already https://github.com/apache/cordova-plugin-file/blob/6.0.1/src/android/FileUtils.java.

So to recap:

So as long as we handle errors in the javascript correctly, we should be good.

shankari commented 5 years ago

cordova-plugin-email-composer

required android.permission.GET_ACCOUNTS in our version 0.8.15, but the current version 0.9.2 does not have it any more. https://github.com/katzer/cordova-plugin-email-composer/blob/0.9.2/plugin.xml

It looks like it uses the standard intent instead. So let's upgrade that.

Fails because of

/Users/shankari/e-mission/e-mission-base/platforms/android/src/de/appplant/cordova/emailcomposer/Impl.java:83: error: diamond operator is not supported in -source 1.6
        List<Intent> targets = new ArrayList<>();
                                             ^
  (use -source 7 or higher to enable diamond operator)
1 error

Also, it looks like the app doesn't automatically ask for permissions; instead the user has to ask them https://github.com/katzer/cordova-plugin-email-composer/tree/0.9.2#permissions and add the permission to the manifest.

And in 0.8.15, the plugin actually did so automatically. https://github.com/katzer/cordova-plugin-email-composer/tree/0.8.15#permissions

So it seems like for our use case, it is actually safer to stick with 0.8.15. Checking for the reason for the change to make sure this is not a boneheaded decision.

This is the commit. No reason specified, no link to any issue. I also don't see any issues related to the change by searching. Going to retain the current version.

It looks like the plugin also supports the app:// URL, not just for android but for iOS. We should try using that instead of our current workaround with files, and then we may be able to remove our dependency on the file plugin.

shankari commented 5 years ago

ok, so now I'm ready to make changes to our plugins. So far, I have: edu.berkeley.eecs.emission.cordova.auth/

edu.berkeley.eecs.emission.cordova.comm/ edu.berkeley.eecs.emission.cordova.datacollection/

edu.berkeley.eecs.emission.cordova.serversync/ edu.berkeley.eecs.emission.cordova.settings/ edu.berkeley.eecs.emission.cordova.transitionnotify/ edu.berkeley.eecs.emission.cordova.unifiedlogger/ edu.berkeley.eecs.emission.cordova.usercache/

Investigate further:

shankari commented 5 years ago

Further investigations:

So I think we are fine wrt follow ups even without updating anything.

shankari commented 5 years ago

Made all changes in https://github.com/e-mission/e-mission-docs/issues/325#issuecomment-471120394 except implementing runtime permissions. Tried to rebuild. Ran into persistent error that I don't remember.

Removed android platform, re-added android platform.

Now getting failure with

AGPBI: {"kind":"error","text":"error: resource android:attr/ttcIndex not found.","sources":[{"file":"/Users/shankari/.gradle/caches/transforms-1/files-1.1/appcompat-v7-25.3.1.aar/ac54b5ada41f1975b65c69e36aa22b4e/res/values/values.xml","position":{"startLine":202,"startColumn":4,"startOffset":24572,"endColumn":68,"endOffset":24636}}],"original":"","tool":"AAPT"}
/Users/shankari/e-mission/e-mission-base/platforms/android/build/intermediates/incremental/mergeDebugResources/merged.dir/values/values.xml:267: error: resource android:attr/fontVariationSettings not found.
/Users/shankari/e-mission/e-mission-base/platforms/android/build/intermediates/incremental/mergeDebugResources/merged.dir/values/values.xml:267: error: resource android:attr/ttcIndex not found.

which is the same as https://stackoverflow.com/questions/49171052/error-android-resource-linking-failed-aapt2-27-0-3-daemon-0

The issue seems to be the use of version 27.1.0 of the support libraries. But how did it work when I just edited the gradle file? Maybe things weren't cleaned enough until I removed and re-added the values?

At any rate, the appcompat dependency is loaded from

+--- com.auth0.android:jwtdecode:1.1.1
|    +--- com.google.code.gson:gson:2.7 -> 2.8.5
|    \--- com.android.support:appcompat-v7:25.3.1
|         +--- com.android.support:support-annotations:25.3.1 -> 26.1.0
|         +--- com.android.support:support-v4:25.3.1 (*)
|         +--- com.android.support:support-vector-drawable:25.3.1
|         |    +--- com.android.support:support-annotations:25.3.1 -> 26.1.0
|         |    \--- com.android.support:support-compat:25.3.1 (*)
|         \--- com.android.support:animated-vector-drawable:25.3.1
|              \--- com.android.support:support-vector-drawable:25.3.1 (*)

which is included in the auth library <framework src="com.auth0.android:jwtdecode:1.1.1"/> I guess that is why we had to specify the gradle file?

But why do we even need the com.auth0.android:jwtdecode framework? that doesn't seem to be a dependency of AppAuth-Android...

shankari commented 5 years ago

But why do we even need the com.auth0.android:jwtdecode framework? that doesn't seem to be a dependency of AppAuth-Android...

No, but we use it to decode the JWT and extract the email in our code. https://github.com/e-mission/cordova-jwt-auth/blob/master/src/android/OpenIDAuth.java#L207

Switched to the most recent version of com.auth0.android:jwtdecode and forced the use of appcompat v27. still didn't help. Then I found this entry which indicated that one of the included libraries had been compiled with API 28. https://stackoverflow.com/a/54125070/4040267

So I grepped around to figure out which library this was in my case.

$ grep -r android:fontVariationSettings ~/.gradle/caches/transforms-1/files-1.1/
/Users/shankari/.gradle/caches/transforms-1/files-1.1//support-compat-28.0.0.aar/18ae3a5455fcd714f8848baafe002aa3/res/values/values.xml:        <attr name="android:fontVariationSettings"/>
/Users/shankari/.gradle/caches/transforms-1/files-1.1//support-compat-28.0.0-rc02.aar/40fcdc5b24df8850d365b77b72a4f7f6/res/values/values.xml:        <attr name="android:fontVariationSettings"/>

and it was support-compat. checking the dependencies, we see what is going on. Basically, since we specified that the v4 support libraries were 24.1.1+, gradle picked version 28.

+--- com.android.support:support-v4:24.1.1+ -> 28.0.0
|    +--- com.android.support:support-compat:28.0.0
|    |    +--- com.android.support:support-annotations:28.0.0
|    |    +--- com.android.support:collections:28.0.0
|    |    |    \--- com.android.support:support-annotations:28.0.0

We can't really fix the versioning (24.1.1+) by changing to (24.1.1), since some are from other plugins.

plugins//cordova-plugin-email-composer/plugin.xml:        <framework src="com.android.support:support-v4:24.1.1+" />

so we fix by setting both the v4 and v13 support library versions in jwtauth. Should really move to cordova-android-support-gradle-release instead of piggy-backing on auth. https://stackoverflow.com/a/49200782/4040267

Build succeeded!!

Now just need to fix dynamic permissions, and we are done!

deepalics0044 commented 5 years ago

any update on this? Before launching the app we need to pull from master and build it again.

shankari commented 5 years ago

After the weekend, let's get back to testing this. First failure: the broadcast receiver doesn't receive any broadcasts. I tested this with adding breakpoints in the code and we send the broadcast but don't receive it. A brief search finds this breaking change in API 26. https://developer.android.com/guide/components/broadcast-exceptions

We can fix this in one of three ways:

  • Apps can continue to register for explicit broadcasts in their manifests.
  • Apps can use Context.registerReceiver() at runtime to register a receiver for any broadcast, whether implicit or explicit.
  • Broadcasts that require a signature permission are exempted from this restriction, since these broadcasts are only sent to apps that are signed with the same certificate, not to all the apps on the device.

The original thought behind defining implicit broadcasts was that other apps could receive them and perform actions, which would make it easier to build interoperable apps. However, we have since moved away from that approach, and towards plugins that can be directly incorporated into other apps. Note that we are getting pushback for the two step process already; it is likely to be even less likely that people will want to actually install two separate apps.

So barring a strong use case for multiple cooperating apps, I am going to go with an explicit broadcast, configured with the package name

shankari commented 5 years ago

So barring a strong use case for multiple cooperating apps, I am going to go with an explicit broadcast, configured with the package name

Changing the intent creation to specify the packagename of the app works.

        i.setPackage(ctxt.getPackageName());

need to figure out where we should put that for the long-term though. Not sure that all classes should import TripDiaryStateMachineReceiver, but also not sure that we should copy that code around everywhere.

Once this is resolved, the next error is the expected one for permissions.

    java.lang.SecurityException: Client must have ACCESS_FINE_LOCATION permission to request PRIORITY_HIGH_ACCURACY locations.
        at edu.berkeley.eecs.emission.cordova.tracker.location.actions.GeofenceActions.readAndReturnCurrentLocation(GeofenceActions.java:114)
        at edu.berkeley.eecs.emission.cordova.tracker.location.actions.GeofenceActions.create(GeofenceActions.java:76)
        at edu.berkeley.eecs.emission.cordova.tracker.location.TripDiaryStateMachineService$3.run(TripDiaryStateMachineService.java:499)
        at java.lang.Thread.run(Thread.java:764)
2019-03-11 13:56:38.230 10661-10661/edu.berkeley.eecs.embase D/CordovaActivity: Paused the activity.

We could:

But the challenge is that both hasPermission and requestPermission are currently defined in the cordova interface (e.g. cordova.hasPermission), which is not really available from the broadcast receiver or the services. Even the PermissionHelper class that they created for backwards compatibility uses a CordovaPlugin class as a parameter.

So I think that the plugin should actually do all the checking, and in the service and actions, we should just generate a tracking error if we don't have permissions, and move to the start state, just like when location services are turned off and on. The good news is that we should be able to re-use a lot of the same logic in the FSM.

Before we commit to this, let's ensure that passing in the cordova interface or the plugin doesn't really work.

shankari commented 5 years ago

Before we commit to this, let's ensure that passing in the cordova interface or the plugin doesn't really work.

Actually I don't see how I can even pass it in. The broadcast receiver is defined in the manifest and launched automatically by the system, not the cordova plugin. So there is no plugin in the interface that we can pass in. Note also that we shouldn't really check the permissions unless the user has consented.

shankari commented 5 years ago

It seems like the overall correct design is to check for permission errors in the actions, which is where we perform the dangerous operations. That will allow each action to check for its own permission. However, if an action forgets, we should still have a backup in the service. And given that there is only one dangerous permission that we really need, that will make it much simpler than spreading the checks across a bunch of places.

Ok, so here's the final plan:

shankari commented 5 years ago

first going to convert all broadcasts to explicit broadcasts to keep the commits clean

Converted everything in the tracker package

Screen Shot 2019-03-11 at 3 43 35 PM

Fixed the one use in the transition notify package

        genericTransitionIntent.setPackage(context.getPackageName());
shankari commented 5 years ago

in the plugin, if the user has consented, we check for permissions on init (similar to checking for google play services initialization)

we don't really need to check for the google play initialization in the plugin init because we have to do it anyway in the service. so that additional check is redundant. However, we can't do the check for the permissions from the service.

Actually, although we can't do the check for permissions from the service using the cordova APIs, we can call the permission methods directly, and they work off context. https://developer.android.com/reference/android/support/v4/content/ContextCompat.html#checkSelfPermission(android.content.Context,%20java.lang.String)

So we should be able to do all the checks directly from the service.

shankari commented 5 years ago

First, let's refactor the plugin init code to be consistent with iOS. It can't be identical with iOS, because on android, the service can be launched directly without launching the activity and initializing the plugin. So here's what we actually want to do:

So we really just need to fix the order in onReceive. Let's do that and then test that the location services stuff still works.

shankari commented 5 years ago

Code seemed to work OK, and we generated a notification, but it was not displayed. This is probably related to https://github.com/e-mission/e-mission-docs/issues/325#issuecomment-470768830 which in turn is https://developer.android.com/training/notify-user/channels

2019-03-11 21:05:11.059 25374-25757/edu.berkeley.eecs.embase I/TripDiaryStateMachineService: After removing transition local.transition.initialize, currActions = []
2019-03-11 21:05:11.777 25374-25374/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: isLocationUsable() true
2019-03-11 21:05:12.580 25374-25374/edu.berkeley.eecs.embase D/NotificationHelper: Generating notify with id 87225377, message Error 6 in location settings and pending intent PendingIntent{83b55ea: android.os.BinderProxy@26d10db}
2019-03-11 21:05:12.739 25374-25374/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: About to disconnect the api client
shankari commented 5 years ago

Testing done (including fix for https://github.com/e-mission/e-mission-docs/issues/267)

shankari commented 5 years ago

I am almost done; generate a prompt if the runtime permissions are not present. I just need to handle the related intent in the activity and test. This should be done by EOD tomorrow Pacific time.

shankari commented 5 years ago

Handled prompting for permissions.

Test case 1

Test case 2

Looking at the logcat logs, this is because of

03-12 11:19:37.846  1700  1714 W BroadcastQueue: Background execution not allowed: receiving Intent { act=android.location.MODE_CHANGED flg=0x10 } to edu.berkeley.eecs.embase/edu.berkeley.eecs.emission.cordova.tracker.location.TripDiaryStateMachineReceiver

Since MODE_CHANGED is an implicit intent, and not on the list of approved implicit intents https://developer.android.com/guide/components/broadcast-exceptions we don't get it

First let's merge this and then figure out how to resolve the MODE_CHANGED. May have to change the entire handling. Argh!

shankari commented 5 years ago

There is essentially no way to resolve the MODE_CHANGED.

Note that we can also fail to get locations because of lack of permission, and there is no callback for that change. So I think that we have to give up on notifications/callbacks and just poll periodically.

First change: we were relying on MODE_CHANGED to enable location when the user clicked on the resolution. So we now need to enable onActivityResult in the plugin after all. That should fix Test case 2 above.

Second change: as part of the periodic call, we check for both permissions and settings. We were always checking whether we could move out of start state, we now check whether we should enter start state due to the location being unavailable.

With these changes, we have Test case 1

Test case 2

Test case 3

Test case 4

Tested all 4 use cases, merging...

shankari commented 5 years ago

Next, we need to handle the re-entrant nature of the service. I am not sure what the best practices for re-entrant services are. In a previous change, I made it so that the service was not re-entrant, but in this change, we can generate a tracking error while we are trying to process other transitions, which makes the service re-entrant.

I think that is fine; we just need to wait until the second transition is also processed before we move to the final state and stop the service.

shankari commented 5 years ago

I just ran into another error with the service not handling dual transitions.

Basically, we got an initialize transition, we

03-12 19:41:22.878  2973  2973 D TripDiaryStateMachineService: service started with flags = 0 startId = 2 action = local.transition.initialize
03-12 19:41:22.894  2973  2973 I TripDiaryStateMachineService: Handling new action local.transition.initialize existing actions are [] adding it to list
03-12 19:41:22.911  2973  2973 D TripDiaryStateMachineService: after reading from the prefs, the current state is local.state.start
03-12 19:41:22.941  2973  2973 D BuiltinUserCache: Added value for key statemachine/transition at time 1.552444882912E9
03-12 19:41:22.955  2973  2973 D TripDiaryStateMachineService: client is already connected, can directly handle the action
03-12 19:41:22.969  2973  2973 D TripDiaryStateMachineService: handleAction(local.state.start, local.transition.initialize) calle
03-12 19:41:23.004  2973  2973 D BuiltinUserCache: Added value for key background/battery at time 1.552444882972E9
03-12 19:41:23.022  2973  2973 D TripDiaryStateMachineService: TripDiaryStateMachineReceiver handleStarted(local.transition.initialize) called

we started a new thread to create the geofence

03-12 19:41:23.023  2973  4456 I System.out: Running in new thread!!
03-12 19:41:23.031  2001  2593 W GCoreFlp: No location to return for getLastLocation()

But we destroy the original service, which disconnects the client. I am not able to see where we set the newState that causes this service to be destroyed though. I wonder if this is a change in background execution as well that we need to handle.

03-12 19:41:23.041  2973  2973 D TripDiaryStateMachineService: About to disconnect the api client
03-12 19:41:23.056  2973  4456 D CreateGeofenceAction: Last location would have been null if we hadn't reset it
03-12 19:41:23.072  2973  2973 I TripDiaryStateMachineService: Service destroyed. So long, suckers!
03-12 19:41:23.086  2973  4456 W CreateGeofenceAction: mLastLocationTime = null, launching callback to read it and thencreate the geofence
shankari commented 5 years ago

ok so to handle the re-entrant code.

I think that this is it.

Just to be on the safe side, let's retry the failure case from https://github.com/e-mission/e-mission-data-collection/issues/127#issuecomment-465421452 and https://github.com/e-mission/e-mission-data-collection/issues/127#issuecomment-465461988

    Start in `waiting for trip_start` state
    Turn location off: stays in `waiting_for_trip_start`
    Force sync: moves to `start` state
    Force start trip: stays in `start` state
    Force sync: generates prompt again (crash!)

basically, I am in start state, location is off, and the force sync generates an initialize

2019-03-12 22:58:42.804 14956-14956/edu.berkeley.eecs.embase I/TripDiaryStateMachineService: Handling new action local.transition.initialize existing actions are [local.transition.exited_geofence] adding it to list
2019-03-12 22:58:42.830 14956-14956/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: after reading from the prefs, the current state is local.state.start
2019-03-12 22:58:42.889 14956-14956/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: handleAction(local.state.start, local.transition.initialize) called

we start a new thread to create the geofence

2019-03-12 22:58:42.917 14956-14956/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: TripDiaryStateMachineReceiver handleStarted(local.transition.initialize) called
2019-03-12 22:58:42.917 14956-15806/edu.berkeley.eecs.embase I/System.out: Running in new thread!!

we try to read the location asynchronously

2019-03-12 22:58:42.939 14956-15806/edu.berkeley.eecs.embase D/CreateGeofenceAction: Last location would have been null if we hadn't reset it
2019-03-12 22:58:42.951 14956-14956/edu.berkeley.eecs.embase I/TripDiaryStateMachineRcvr: TripDiaryStateMachineReciever onReceive(android.app.ReceiverRestrictedContext@8648c54, Intent { act=local.transition.tracking_error flg=0x10 pkg=edu.berkeley.eecs.embase cmp=edu.berkeley.eecs.embase/edu.berkeley.eecs.emission.cordova.tracker.location.TripDiaryStateMachineReceiver }) called
2019-03-12 22:58:42.958 14956-15806/edu.berkeley.eecs.embase W/CreateGeofenceAction: mLastLocationTime = null, launching callback to read it and thencreate the geofence

but we also get a tracking error in parallel

2019-03-12 22:58:42.969 14956-14956/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: service started with flags = 0 startId = 3 action = local.transition.tracking_error
2019-03-12 22:58:42.977 14956-15806/edu.berkeley.eecs.embase D/CreateGeofenceAction: Successfully started tracking location, about to start waiting for location update
2019-03-12 22:58:42.987 14956-14956/edu.berkeley.eecs.embase I/TripDiaryStateMachineService: Handling new action local.transition.tracking_error existing actions are [local.transition.exited_geofence, local.transition.initialize] adding it to list

which we handle and then stop the service

2019-03-12 22:58:43.073 14956-14956/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: newState after handling action is local.state.start
2019-03-12 22:58:43.082 14956-14956/edu.berkeley.eecs.embase D/TripDiaryStateMachineService: newState saved in prefManager is local.state.start
2019-03-12 22:58:43.087 14956-14956/edu.berkeley.eecs.embase I/TripDiaryStateMachineService: About to stop service after handling [local.transition.exited_geofence, local.transition.initialize, local.transition.tracking_error]

which then fails when we generate the error and try to remove the geofence location updates listener

Note that we got the tracking_error, presumably from the checkLocation... after the initialize, because it is asynchronous.

shankari commented 5 years ago

I am going to take a break from the FSM stuff and finish a couple of the other things first.

  1. Turn firebase analytics off https://firebase.google.com/support/guides/disable-analytics

    Tried to add the following lines to config.xml

        <config-file parent="/manifest/application" target="AndroidManifest.xml">
            <meta-data android:name="firebase_analytics_collection_deactivated" android:value="true" />
            <meta-data android:name="google_analytics_adid_collection_enabled" android:value="false" />
        </config-file>

    gives an error while building, but the resulting AndroidManifest.xml does have those lines. And now Firebase looks like it is not measuring things.

    also deleted the firebase storage and database URLs from the project info

    03-12 23:43:38.525 17729 17729 D FirebaseApp: com.google.firebase.auth.FirebaseAuth is not linked. Skipping initialization.
    03-12 23:43:38.527 17729 17729 D FirebaseApp: com.google.firebase.crash.FirebaseCrash is not linked. Skipping initialization.
    03-12 23:43:38.591 17729 17729 I FirebaseInitProvider: FirebaseApp initialization successful
    03-12 23:43:53.997 17838 17838 D FirebaseApp: com.google.firebase.auth.FirebaseAuth is not linked. Skipping initialization.
    03-12 23:43:54.024 17838 17838 D FirebaseApp: com.google.firebase.crash.FirebaseCrash is not linked. Skipping initialization.
    03-12 23:43:54.061 17838 17838 I FirebaseInitProvider: FirebaseApp initialization successful
  2. confirm that the ContentProvider stuff continues to work for the sync

    • set the sync interval to one minute
    • see how often the sync actually happens
shankari commented 5 years ago

@deepalics0044 I am almost done, but there are still some minor pending issues that you don't use. In particular, the trip end notifications will not display, and I haven't tested the new push notification code extensively after the upgrade to the new version of the plugin.

But I think you don't use either of those, so once I finish testing the periodic sync, I am going to commit everything so that you can generate your apk

I will work on the final cleanup pieces next week.

shankari commented 5 years ago

ok so ContentProvider does not continue to work for the sync.

I see this error in the logs

03-13 00:05:21.462  1672  1685 D SyncManager: failed sync operation JobId: 107195, dummy_account u0 (eecs.berkeley.edu), edu.berkeley.eecs.emission.provider, SERVER, reason: AutoSync, SyncResult: databaseError: true stats []

Looking some more, I see this entire set of logs

03-13 00:05:08.071  1672  1672 W SyncManager: Failure adding authority: account=dummy_account auth=edu.berkeley.eecs.emission.provider enabled=true syncable=1
03-13 00:05:21.462  1672  1685 D SyncManager: failed sync operation JobId: 107195, dummy_account u0 (eecs.berkeley.edu), edu.berkeley.eecs.emission.provider, SERVER, reason: AutoSync, SyncResult: databaseError: true stats []
...
03-13 00:17:16.165  1672  1685 D SyncManager: failed sync operation JobId: 108886, dummy_account u0 (eecs.berkeley.edu), edu.berkeley.eecs.emission.provider, PERIODIC, reason: Periodic, period: 900000, flexMillis: 36000, SyncResult: databaseError: true stats []
...
03-13 00:31:59.321  1672  1685 D SyncManager: failed sync operation JobId: 108886, dummy_account u0 (eecs.berkeley.edu), edu.berkeley.eecs.emission.provider, PERIODIC, reason: Periodic, period: 900000, flexMillis: 36000, SyncResult: databaseError: true stats []
shankari commented 5 years ago

Testing on android 6.0 to start from first principles. Looks like it is launched every ~ 20 mins but errors out each time. Let's fix the provider name in AndroidManifest.xml.

03-13 08:46:46.249 22428 22448 I BackgroundTaskManager: Executing task InitializeCustome03-13 08:06:26.027 21436 21452 E ActivityThread: Failed to find provider info for edu.berkeley.eecs.emission.provider
03-13 08:20:43.262 21436 21885 E ActivityThread: Failed to find provider info for edu.berkeley.eecs.emission.provider
03-13 08:40:43.385 21436 22360 E ActivityThread: Failed to find provider info for edu.berkeley.eecs.emission.provider
shankari commented 5 years ago

Works now!

03-13 09:30:43.531 23334 23692 I ServerSyncAdapter: Starting sync with push
03-13 09:40:44.441 23334 23986 I ServerSyncAdapter: Starting sync with push
03-13 09:50:20.348 23334 25070 I ServerSyncAdapter: Starting sync with push

Ok, so now let's test this working code in a more recent version of android

shankari commented 5 years ago

Installing at 9:57

03-13 09:55:03.500  1673  1686 I ActivityManager: Start proc 2510:edu.berkeley.eecs.embase:sync/u0a83 for service edu.berkeley.eecs.embase/edu.berkeley.eecs.emission.cordova.serversync.SyncService
03-13 09:55:03.773  1673  2267 I ActivityManager: Start proc 2582:edu.berkeley.eecs.embase/u0a83 for content provider edu.berkeley.eecs.embase/edu.berkeley.eecs.emission.cordova.serversync.StubContentProvider
03-13 09:55:06.877  1673  1687 I ActivityManager: Killing 2582:edu.berkeley.eecs.embase/u0a83 (adj 700): stop edu.berkeley.eecs.embase
03-13 09:55:06.881  1673  1687 I ActivityManager: Killing 2510:edu.berkeley.eecs.embase:sync/u0a83 (adj 900): stop edu.berkeley.eecs.embase
...
i03-13 09:55:07.053  2356  2356 I Pm      : Package edu.berkeley.eecs.embase installed in 3694 ms

works on android 8 (oreo) as well

03-13 10:09:49.565  3990  4006 I ServerSyncAdapter: Starting sync with push
03-13 10:24:59.966  3990  4096 I ServerSyncAdapter: Starting sync with push
03-13 10:39:47.693  3990  4174 I ServerSyncAdapter: Starting sync with push
shankari commented 5 years ago

Final issue: need to deal with re-entrant calls. Thinking about it carefully, the main issue is that we share the same instance of the GoogleApiClient across multiple invocations. One fairly naive fix would be to launch a new thread for each action, and give it its own copy of the GoogleApiClient. Another would be to track if there are pending operations and wait to exit.

Let's try the second approach first briefly. We already have a list of pending operations, we just need to remove them from the list when we start an operation (check...) that can generate a new transition.

shankari commented 5 years ago

Finished second approach. Had to remove all notifications before the call to checkLocation... so that we don't get two (confusing) notifications. Re-testing....

    Start in `waiting for trip_start` state
    Turn location off: stays in `waiting_for_trip_start`
    Force sync: moves to `start` state
    Force start trip: stays in `start` state
    Force sync: generates prompt again (did not crash!)

Did some other random testing, nothing crashes. Going to merge this and update all the plugins so that @deepalics0044 is unblocked.

woo!

shankari commented 5 years ago

Plugins that are updated are: https://github.com/e-mission/e-mission-transition-notify/pull/19 https://github.com/e-mission/e-mission-data-collection/pull/170 https://github.com/e-mission/cordova-unified-logger/pull/29 https://github.com/e-mission/cordova-jwt-auth/pull/35 https://github.com/e-mission/cordova-server-sync/pull/35

shankari commented 5 years ago

@deepalics0044 in order to use dynamic permissions you can merge this PR locally https://github.com/e-mission/e-mission-phone/pull/533

There are no changes to the phone HTML code. Instead, the native plugins are updated. Make sure to remove and re-install the plugins and ensure that they are in fact at the correct versions.

Let me know if you run into any issues.

shankari commented 5 years ago

Because I updated the push plugin, you also need to generate and download google-services.json and Google-Services.plist from firebase; the app will not compile without it. https://github.com/phonegap/phonegap-plugin-push/blob/master/docs/INSTALLATION.md#installation-requirements