Target Android 14 (set targetSdkVersion to 34), by 2024-10 deadline

[gnprice]

This is the successor to #5453 (and an annual series of previous issues linked from there). We should update our targetSdkVersion to 34, meaning Android 14.

The deadline is earlier this year than in some previous years: it's 2024-08-31. (Much like last year, we can request an extension to 2024-11-01.) is 2024-10-31, after I requested an extension.

The important steps for this upgrade are:

1. Read about the potentially-breaking changes, and identify those that might affect us. (edit: Also the changes not tied to targetSdkVersion, because for Android 13 last year a change that should have been in the first doc wasn't, and bit us, and the latter doc had an item that would have raised our suspicions if we'd read that doc closely.)
2. Make a WIP change to targetSdkVersion.
3. Test the WIP change thoroughly, especially in the areas highlighted in step 1. Fix things as needed, and repeat.

[chrisbobbe]

From that potentially-breaking changes article:

• [ ] Core functionality:
  • [x] Foreground service types are required
  • We don't have any foreground services AFAICT.
  • [x] Enforcement of BLUETOOTH_CONNECT permission in BluetoothAdapter
  • We don't use Bluetooth. @react-native-community/netinfo doesn't use the BluetoothAdapter API mentioned in the article.
  • [ ] OpenJDK 17 updates
  • Regexes are used by react-native-webview in a custom DownloadListener…I think we don't allow downloads directly from the WebView, so this should be moot?
  • A few regexes are used in react-native, in […]/animated/InterpolationAnimatedNode.java, […]/devsupport/BundleDownloader.java, […]/devsupport/StackTraceHelper.java, […]/util/JSStackTrace.java. Probably any issues will get discovered in manual testing?
  • expo-constants uses UUID.fromString but it looks like it might already handle IllegalArgumentException in that code?
  • We don't use ProGuard so I don't think the point about ProGuard is relevant.
  • [x] JobScheduler reinforces callback and network behavior
  • I don't think we use JobScheduler; irrelevant.
  • [x] Tiles launch API
  • I don't think we use TileService; irrelevant.
• [ ] Privacy:
  • [ ] Partial access to photos and videos
  • I'm getting confused about this one. It seems like as long as we use "the photo picker", then we don't need any image/video access permissions, because image selection is done through an external system that only gives our app access to specific media the user chooses. When we bumped targetSdkVersion to 33 in 6f44474ac, we wrote down that we do use "Android's PhotoPicker API", because we upgraded react-native-image-picker. That upgrade included a commit that said "add new PhotoPicker API for Android 13 Teramisu"…but no code with "PhotoPicker" was added; instead it started using something called MediaStore, which is still used in react-native-image-picker at main. And there's an open issue for "Using Android's Photo Picker instead of Media Store", which links to an Android doc to show how they're different.
  • So I think it's possible that this part in the behavior-changes doc will apply to us: "If your app doesn't use the new permission, the system runs your app in a compatibility mode."
  • In any case we should probably test our upload flows manually, on an Android 14 device and a non-Android 14 device (done; image upload from camera and library worked on the office Android device, and we don't support video uploads from either source on Android 9).
• [x] User experience:
  • [x] Secure full-screen Intent notifications
  • We don't use full-screen intents; irrelevant. (It's a feature for showing extremely high-priority notifications like phone calls and alarms.)
• [ ] Security:
  • I'd appreciate Greg's review of these.
  • [ ] Restrictions to implicit and pending intents
  • [ ] Runtime-registered broadcasts receivers must specify export behavior
  • [ ] Safer dynamic code loading
  • [ ] Additional restrictions on starting activities from the background
  • [ ] Zip path traversal
  • [x] User consent required for each MediaProjection capture session
  • We don't use MediaProjection; it says "A token granting applications the ability to capture screen contents and/or record system audio." Irrelevant.
• [x] Updated non-SDK restrictions
  • A linked doc says any issues of this kind would show up in the Play Console, and I checked and didn't see any.

[chrisbobbe]

Quick manual testing on the office Android device (Android 9) showed no problems, which I guess isn't surprising because it's not Android 14. The app opened without crashing; notifications worked, with the app foregrounded/backgrounded/closed; and image uploads worked, both from the camera and the media library. (We don't support video uploads from the media library prior to Android 13, and we don't support video uploads from the camera on Android at all.)

[gnprice]

showed no problems, which I guess isn't surprising because it's not Android 14.

Yeah — once targetSdkVersion is at least equal to the device's SDK version, it should be completely indifferent to the specific value. So a targetSdkVersion of 34 vs. 33 should mean exactly the same thing to any device with SDK version up through 33, i.e. up through Android 13.

[gnprice]

Thanks for the detailed read through the breaking-changes page! I'll do a separate read too, for a cross-check.

[gnprice]

OK, doing my own read-through. After I read each of these items I'm also going and reading your notes on them — thanks for the thorough validation of where some of these APIs are used!

• Core functionality

  • Foreground services — I agree, I believe we don't have any.

  • Bluetooth — agree, should be irrelevant

  • OpenJDK 17

  • Regexes — yeah, hopefully nothing has an invalid group reference anyway. If something does, I guess we'll either discover it in manual testing (or users later reporting it as a bug), or it probably isn't commonly enough triggered to worry about.

    FWIW that regex in react-native-webview does have a group reference (the \\1), but it looks perfectly valid (there's a group that comes before it, in fact two such groups; it'll refer to what the (\"?) captured).

  • UUIDs — similar story to the regexes, but UUIDs are less ubiquitous than regexes.

  • ProGuard — yeah agreed, irrelevant to us

  • JobScheduler — I looked in Android Studio at those onStartJob/onStopJob methods; nothing in our project implements the JobService interface they live on. So indeed definitely not using that.

  • Tiles — I looked in Android Studio at that deprecated method startActivityAndCollapse. The only call site was elsewhere in the Android SDK (in com.android.egg.neko.NekoTile); and that class in turn has no references. So indeed definitely not using.

• Privacy

  • Partial access to photos/videos — yeah, from that issue https://github.com/react-native-image-picker/react-native-image-picker/issues/2299 and your other notes on this point above, it kind of sounds like the maintainers of react-native-image-picker at the time may have not understood what they were doing when they merged a PR that said it was adding the use of the photo picker.

  … OTOH, when I try the app now on my Android 14 phone, using the "image" button under the compose box, I do get the photo picker UI.

  So it'll be informative to test #5879 on an Android 14 device and see if image/video uploads still work or not.

  Fortunately also, as you noted elsewhere, there is that PR https://github.com/react-native-image-picker/react-native-image-picker/pull/2304 which sounds like it may solve the problem (if indeed there is a problem?). Hopefully as the deadline approaches there'll be more impetus toward finishing that and merging it (or clarifying it's not needed?). If not, as a fallback plan we could try using the PR's version.

• User experience

  • Full-screen intents — agreed, irrelevant

• Security

  • [ ] Implicit intents — We have several intent filters. Some are on exported components (with android:exported="true"), our MainActivity and ShareToZulipActivity; so those are fine. Then some are on unexported components, so any intents targeting those will need to be explicit. Those components are FcmListenerService, matching action com.google.firebase.MESSAGING_EVENT; and com.google.firebase.iid.FirebaseInstanceIdService, matching action com.google.firebase.INSTANCE_ID_EVENT.

  I guess we can validate both of those by making a fresh install, with #5879, on an Android 14 device, and verifying that notifications work.

  • [x] Pending intents — I believe we don't ever create a mutable pending intent, so this doesn't affect us. (I looked for references to PendingIntent.FLAG_MUTABLE; there were just a few, I believe all in the Android SDK, and none looking like something we might be indirectly calling. And OTOH the PendingIntent in our notifications uses PendingIntent.FLAG_IMMUTABLE.)

  • [x] Runtime-registered broadcast receivers — we don't use these. Following the link on context-registered receivers, those require androidx.core version 1.9.0+; we're on 1.7.0 and so don't have the ContextCompat.registerReceiver API we'd use in order to make such a thing.

  • [x] Dynamic code loading — pretty sure we don't do this. There's no reason we should; and I also looked for references to the PathClassLoader constructors (the class mentioned in this docs section), and found a handful that were all in the Android SDK and all either looked harmless (because they're loading something from within the APK) or not particularly like something we'd transitively use.

  If we do somehow run into this, it'd probably crash the app, so we'd notice in manual testing.

  • [x] Starting activities from background — I don't think we ever do this.

  I don't think we ever call a method like PendingIntent#send. We do construct a PendingIntent that goes on our notifications, describing how to open the notification — but then IIUC it's the system's notification manager that invokes that PendingIntent, by calling PendingIntent#send or the like.

  I'm pretty sure we never bind a service of another app.

  • [x] Zip path traversal — I don't think we ever open a zip file, except our APK itself.

  • MediaProjection — agreed, we never use this

• Updated non-SDK restrictions — agreed, we seem to be in the clear

---

In short:

• The PhotoPicker situation might be fine after all (though those react-native-image-picker threads are puzzling, then). We'll find out from some manual testing.
• The restrictions on implicit intents are probably fine. We should confirm by manually testing notifications on a fresh install.
• The other security changes all appear to be fine.

[gnprice]

After our experience last year with the doc that's supposed to describe the effects of targetSdkVersion being deficient, I also read through the "Behavior changes: all apps" doc.

• [x] Core functionality

  • [x] Exact alarms — we don't use these.
  • [x] Context-registered broadcasts queued — I believe this is inconsistent terminology for the same concept as "context-registered receivers", which came up above and I determined we don't use.

  Specifically, the link here for "important broadcasts that are declared in the manifest" goes to a section headed "Manifest-declared receivers". So that demonstrates alternation in terminology between the "broadcast" being declared in place X and the broadcast receiver being declared in place X. I believe the same thing is going on with the idea of a "context-registered broadcast" — partly because I don't have any other ideas what it would mean for a broadcast to be "context-registered".

  • [x] Apps can kill only their own background processes — we don't attempt this.
  • [x] "MTU is set to 517 for the first GATT client requesting an MTU" (yes, that's the heading verbatim) — this is deep inside Bluetooth, which we don't use.
  • [x] New reason an app can be placed in the restricted standby bucket — this looks like potentially it could prevent notifications from appearing if we have repeated ANRs. (Which was already true for some ANRs, but I guess not for Service#onBind ANRs.) Not actionable for us, though. We do have some ANRs for some users; if those start causing notification trouble for users where they weren't before, the realistic fix is to get the new Flutter-based app launched to those users.
  • [x] mlock — It'd be weird for us to be using this, and I doubt we are.
  • [x] cached-app resource usage — I think we're using framework-supported lifecycle APIs, and I doubt we're running afoul of this.
• [x] User experience
  • [x] Non-dismissable notifications — our notifications are all dismissable.
  • [x] Data safety information — we're already content with the answers we've supplied here.
• [x] Accessibility
  • [x] Non-linear font scaling — I tried a quick test just now, and generally everything works fine. The only glitch I noticed was the count badge on the DMs icon in the bottom nav bar; but that's a marginal enough feature that I'm OK with it being glitched in this legacy app for some users.

• [x] Security

  • [x] Minimum installable target API level — As a user I certainly noticed this affecting some other apps, which had been unmaintained for years. Doesn't affect us, though. We've met this threshold since 53970d46d57f9188286ca1e26b0ec5761a712e8f back in 2018, when the Play Store started requiring it.
  • [x] Media owner package names — Hmmm. We do actually use this column, when installing notification sounds, to check if a file was put there by our own app.

  Taking the doc literally, the value would be redacted so we can't see it: our app isn't always visible to other apps (that's true only of some system packages), and we don't request QUERY_ALL_PACKAGES permission.

  OTOH, what they probably mean is actually that it's redacted unless the package is visible to our app. That'd make a lot more sense… and it's what the OWNER_PACKAGE_NAME doc itself says. And an app is always visible to itself, so we're in the clear.

  So I guess we could double-check that with manual testing.

  But… the impact of this is pretty minor at worst, and I'm not sure it has any real user impact at all. It can only matter on first install (or first upgrade from an ancient version with a different notification channel ID); only if there's already a sound on the system with the name of our default notification sound, "Zulip - Chime.m4a"; and only if that sound was in fact placed by a previous install of our app.

  Even then, it'll just cause us to set the notification sound to the copy from within our APK rather than to the copy in the device's shared media storage. So the user should still get the sound — I think the only effect is that if they go to the system's notification settings for the app, and consider changing the sound, they'll miss out on seeing the name of the sound as a hint to make discoverable that we have a couple of other sounds with similar names.

  So in the end I'm content to not investigate.

In short: I think there's nothing further to investigate from that doc.

[gnprice]

The deadline for this is now just over six weeks away (2024-10-31). The next steps are to try the upgrade and do some manual testing.

• For this we're blocked at the moment by #5888. Hopefully we'll resolve that shortly.
• Then the change to test is just a couple of lines; it starts with #5879 and then updates targetSdkVersion too.
• Then the things to test (on an Android 14+ device) are, based on my comment above at https://github.com/zulip/zulip-mobile/issues/5877#issuecomment-2279511937 :
  • The photo picker.
  • Notifications, on a fresh install of the app.
• If those all work, then we can merge the upgrade and make a release.
  • Making a release eagerly is good to ensure that if that turns up a showstopping bug, we make that discovery at a time when we can still revert on Google Play to a release without the upgrade, while we investigate.
  • This should also probably get a bit of a longer period in beta than many releases, like most of a week.

[gnprice]

I tested the targetSdkVersion 34 bump in a release build on my Pixel 8 running Android 14. Both of the areas above work fine: the photo picker, and notifications, in a fresh install of the app.

The photo picker working is consistent with what I found above at https://github.com/zulip/zulip-mobile/issues/5877#issuecomment-2279511937 in testing the current released version of the app. It's a bit puzzling how it's working — there's that issue https://github.com/react-native-image-picker/react-native-image-picker/issues/2299 discussed above, and it sure does look in the code like it's using the MediaStore API. But I don't see the symptoms reported in that issue; I get just one modal for picking an image, and the image I pick there successfully gets uploaded.

So that all looks good.

---

However. When I try to launch a debug build, it crashes. The stack trace says:

Caused by: java.lang.SecurityException: com.zulipmobile.debug:
  One of RECEIVER_EXPORTED or RECEIVER_NOT_EXPORTED should be specified
  when a receiver isn't being registered exclusively for system broadcasts
[…]
    at android.app.IActivityManager$Stub$Proxy.registerReceiverWithFeature(IActivityManager.java:5860)
    at android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1853)
    at android.app.ContextImpl.registerReceiver(ContextImpl.java:1793)
    at android.app.ContextImpl.registerReceiver(ContextImpl.java:1781)
    at android.content.ContextWrapper.registerReceiver(ContextWrapper.java:757)
    at com.facebook.react.devsupport.DevSupportManagerBase.reload(DevSupportManagerBase.java:1110)

Full stack trace

``` FATAL EXCEPTION: main Process: com.zulipmobile.debug, PID: 19362 java.lang.RuntimeException: Unable to create application com.zulipmobile.MainApplication: java.lang.RuntimeException: Requested enabled DevSupportManager, but BridgeDevSupportManager class was not found or could not be created at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7403) at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2379) at android.os.Handler.dispatchMessage(Handler.java:107) at android.os.Looper.loopOnce(Looper.java:232) at android.os.Looper.loop(Looper.java:317) at android.app.ActivityThread.main(ActivityThread.java:8592) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878) Caused by: java.lang.RuntimeException: Requested enabled DevSupportManager, but BridgeDevSupportManager class was not found or could not be created at com.facebook.react.devsupport.DefaultDevSupportManagerFactory.create(DefaultDevSupportManagerFactory.java:96) at com.facebook.react.ReactInstanceManager.(ReactInstanceManager.java:260) at com.facebook.react.ReactInstanceManagerBuilder.build(ReactInstanceManagerBuilder.java:358) at com.facebook.react.ReactNativeHost.createReactInstanceManager(ReactNativeHost.java:95) at expo.modules.ReactNativeHostWrapperBase.createReactInstanceManager(ReactNativeHostWrapperBase.kt:33) at com.facebook.react.ReactNativeHost.getReactInstanceManager(ReactNativeHost.java:42) at com.zulipmobile.MainApplication.onCreate(MainApplication.java:66) at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1386) at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7398) at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2379)  at android.os.Handler.dispatchMessage(Handler.java:107)  at android.os.Looper.loopOnce(Looper.java:232)  at android.os.Looper.loop(Looper.java:317)  at android.app.ActivityThread.main(ActivityThread.java:8592)  at java.lang.reflect.Method.invoke(Native Method)  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)  Caused by: java.lang.reflect.InvocationTargetException at java.lang.reflect.Constructor.newInstance0(Native Method) at java.lang.reflect.Constructor.newInstance(Constructor.java:343) at com.facebook.react.devsupport.DefaultDevSupportManagerFactory.create(DefaultDevSupportManagerFactory.java:85) at com.facebook.react.ReactInstanceManager.(ReactInstanceManager.java:260)  at com.facebook.react.ReactInstanceManagerBuilder.build(ReactInstanceManagerBuilder.java:358)  at com.facebook.react.ReactNativeHost.createReactInstanceManager(ReactNativeHost.java:95)  at expo.modules.ReactNativeHostWrapperBase.createReactInstanceManager(ReactNativeHostWrapperBase.kt:33)  at com.facebook.react.ReactNativeHost.getReactInstanceManager(ReactNativeHost.java:42)  at com.zulipmobile.MainApplication.onCreate(MainApplication.java:66)  at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1386)  at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7398)  at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2379)  at android.os.Handler.dispatchMessage(Handler.java:107)  at android.os.Looper.loopOnce(Looper.java:232)  at android.os.Looper.loop(Looper.java:317)  at android.app.ActivityThread.main(ActivityThread.java:8592)  at java.lang.reflect.Method.invoke(Native Method)  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)  Caused by: java.lang.SecurityException: com.zulipmobile.debug: One of RECEIVER_EXPORTED or RECEIVER_NOT_EXPORTED should be specified when a receiver isn't being registered exclusively for system broadcasts at android.os.Parcel.createExceptionOrNull(Parcel.java:3183) at android.os.Parcel.createException(Parcel.java:3167) at android.os.Parcel.readException(Parcel.java:3150) at android.os.Parcel.readException(Parcel.java:3092) at android.app.IActivityManager$Stub$Proxy.registerReceiverWithFeature(IActivityManager.java:5860) at android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1853) at android.app.ContextImpl.registerReceiver(ContextImpl.java:1793) at android.app.ContextImpl.registerReceiver(ContextImpl.java:1781) at android.content.ContextWrapper.registerReceiver(ContextWrapper.java:757) at com.facebook.react.devsupport.DevSupportManagerBase.reload(DevSupportManagerBase.java:1110) at com.facebook.react.devsupport.DevSupportManagerBase.reloadSettings(DevSupportManagerBase.java:735) at com.facebook.react.devsupport.DevSupportManagerBase.setDevSupportEnabled(DevSupportManagerBase.java:612) at com.facebook.react.devsupport.DevSupportManagerBase.(DevSupportManagerBase.java:205) at com.facebook.react.devsupport.BridgeDevSupportManager.(BridgeDevSupportManager.java:77) at java.lang.reflect.Constructor.newInstance0(Native Method)  at java.lang.reflect.Constructor.newInstance(Constructor.java:343)  at com.facebook.react.devsupport.DefaultDevSupportManagerFactory.create(DefaultDevSupportManagerFactory.java:85)  at com.facebook.react.ReactInstanceManager.(ReactInstanceManager.java:260)  at com.facebook.react.ReactInstanceManagerBuilder.build(ReactInstanceManagerBuilder.java:358)  at com.facebook.react.ReactNativeHost.createReactInstanceManager(ReactNativeHost.java:95)  at expo.modules.ReactNativeHostWrapperBase.createReactInstanceManager(ReactNativeHostWrapperBase.kt:33)  at com.facebook.react.ReactNativeHost.getReactInstanceManager(ReactNativeHost.java:42)  at com.zulipmobile.MainApplication.onCreate(MainApplication.java:66)  at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1386)  at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7398)  at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2379)  at android.os.Handler.dispatchMessage(Handler.java:107)  at android.os.Looper.loopOnce(Looper.java:232)  at android.os.Looper.loop(Looper.java:317)  at android.app.ActivityThread.main(ActivityThread.java:8592)  at java.lang.reflect.Method.invoke(Native Method)  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)  Caused by: android.os.RemoteException: Remote stack trace: at com.android.server.am.ActivityManagerService.registerReceiverWithFeature(ActivityManagerService.java:14469) at android.app.IActivityManager$Stub.onTransact(IActivityManager.java:2648) at com.android.server.am.ActivityManagerService.onTransact(ActivityManagerService.java:2822) at android.os.Binder.execTransactInternal(Binder.java:1500) at android.os.Binder.execTransact(Binder.java:1444) ```

So the key line is here:

        mApplicationContext.registerReceiver(mReloadAppBroadcastReceiver, filter);

That's exactly the "runtime-registered broadcast receivers" or "context-registered receivers" item that was mentioned in the release notes. I concluded above we didn't use it because our androidx.core was too old… but that bit of RN code is using the registerReceiver method on Context directly, from the Android SDK. (So another instance of Android docs being too glib, particularly in release notes.)

So we'll need to deal with that — it's debug-only, but it completely breaks debug builds, and we do need the ability to run debug builds so we can develop changes when necessary.

Presumably RN dealt with this upstream sometime in the last year or two. It's unlikely we'll upgrade RN to that version, because upgrading RN has always been a lot of work and for this legacy codebase that's no longer worth it. It's possible that the solution will look like running our own little fork of RN that just backports one small patch atop v0.68.7, which is (still) the latest v0.68.x and is the version we're using.

[chrisbobbe]

Thanks for all that investigation and writeup! As discussed in the office just now, we should be able to apply the patch using patch-package, and I expect that'll be pretty straightforward.