MarquisLP / World-Scribe

An Android app for fictional world-building
MIT License
41 stars 7 forks source link

App crashes after selecting a storage location on some Android 10 devices #43

Open MarquisLP opened 4 years ago

MarquisLP commented 4 years ago

Several users have said that the app crashes after they've selected the app's storage location on the Permissions page.

I've checked the Play Console logs and it would appear this is the culprit:

java.lang.RuntimeException: 

  at android.app.ActivityThread.deliverResults (ActivityThread.java:5097)

  at android.app.ActivityThread.handleSendResult (ActivityThread.java:5138)

  at android.app.servertransaction.ActivityResultItem.execute (ActivityResultItem.java:51)

  at android.app.servertransaction.TransactionExecutor.executeCallbacks (TransactionExecutor.java:135)

  at android.app.servertransaction.TransactionExecutor.execute (TransactionExecutor.java:95)

  at android.app.ActivityThread$H.handleMessage (ActivityThread.java:2147)

  at android.os.Handler.dispatchMessage (Handler.java:107)

  at android.os.Looper.loop (Looper.java:237)

  at android.app.ActivityThread.main (ActivityThread.java:7811)

  at java.lang.reflect.Method.invoke (Native Method)

  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:493)

  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1076)

Caused by: java.lang.NullPointerException: 

  at com.balda.flipper.DocumentFileCompat.peekSubFolder (DocumentFileCompat.java:85)

  at com.averi.worldscribe.utilities.ExternalReader.worldAlreadyExists (ExternalReader.java:74)

  at com.averi.worldscribe.activities.PermissionActivity.goToNextActivity (PermissionActivity.java:183)

  at com.averi.worldscribe.activities.PermissionActivity.onActivityResult (PermissionActivity.java:141)

  at android.app.Activity.dispatchActivityResult (Activity.java:8292)

  at android.app.ActivityThread.deliverResults (ActivityThread.java:5090)

So this is an issue either with Flipper, how the Permissions page calls it, or the way we retrieve the root tree URI on Android 10+ devices. Given that the error is a NullPointerException, it seems the latter might be true and causing us to get a null tree URI. As such, it might be worth looking into tree URIs in more detail.

MarquisLP commented 4 years ago

634947ef17592a38ff23c191bb6d8467e71faeef - I've edited the problematic locations to throw RuntimeExceptions that log the device's root tree URI. This will help us start diagnosing the issue by letting us see if there's a specific kind of URI / file system that's being affected.

MarquisLP commented 4 years ago

Upon further inspection in the Play Console, it seems this crash specifically affects Samsung devices running Android 10. As such, it probably has something to do with Samsung's file system.

I've had the chance to speak to one such user and send them a modified APK that prints their file tree URI at the time of a crash. The URI was:

file://storage/emulated/0

The prefix is significant because that means the device is serving a file URI, rather than a content URI as needed for API Level 24 and above. According to this StackOverflow answer, the issue may be resolved if we use FileProvider to convert the file URI to a content URI.

MarquisLP commented 4 years ago

Reopening this issue because another user contacted me yesterday saying that the fix didn't work for them. So there's still a subset of users affected by this crash.

However, the first user I contacted said that the fix worked for them, so it will be included in 1.6.1 in the hopes that it will help at least some of the affected users.

MarquisLP commented 4 years ago

This issue is still occurring for some users, so I've conducted further investigation yielding the following insights:

Since this issue seems to mostly affect existing users after updating the app, it's more likely that the app directory cannot be read for some reason. But to make sure it's not a creation error, I've requested a few users to try uninstalling and reinstalling the app, then report back to me on whether the app directory was created or not. If you're an affected user reading this, then please do the same and let me know your results in this thread.

Further investigation will occur once I've received responses on this matter.

MarquisLP commented 4 years ago

Thanks to the help of several diligent users, I was able to pinpoint the exact cause of this issue as well as develop a fix for it.

The cause originates in the constructor for the Flipper utility class, StorageManagerCompat. For devices that have an Android SDK earlier than Q, this constructor automatically calls Environment.getExternalStorageDirectory() and adds its resulting URI to the StorageManagerCompat's list of device roots.

In most cases, Environment.getExternalStorageDirectory() will return 'file:///storage/emulated/0', which is the URI that appeared in the error messages sent by users. However, [Environment.getExternalStorageDirectory() is now deprecated](https://developer.android.com/reference/android/os/Environment#getExternalStorageDirectory()) because it uses the older file system model from pre-Android 10. So when the app tries to use that URI on Android 10 devices, it fails because the app is not allowed to access '/storage/emulated/0' directly.

In PermissionsActivity, we have code that overwrites this invalid root URI with the URI for the location that users grant permission to. This overwrite is accomplished by calling StorageManagerCompat.addRoot(). The problem, however, is that StorageManagerCompat's root list uses the Java HashSet data structure, whose add() method is defensive against duplicates. This means that if you call StorageManagerCompat.addRoot() with a Root that already seems to exist in the HashSet, it will not be added.

Adding onto this is the fact that in Flipper's Root class, the equality override only checks the 'name' attribute and not the 'uri' attribute. So although we're trying to do StorageManagerCompat.addRoot(new Root(DEF_MAIN_ROOT, uri)), where uri is different from the one currently stored, the HashSet sees that there already exists a Root with the name 'DEF_MAIN_ROOT' and discards uri.

I confirmed this was happening by asking users to take screenshots of the file root URI at two different points in the setup process: when the URI is returned directly from ACTION_OPEN_DOCUMENT_TREE, transmitted to onActivityResult as the 'Intent data' parameter; and later, after the addRoot() call, when FileRetriever attempts to use the stored URI. In the former, the URI was a valid 'content:///' URI; in the latter, it suddenly became the invalid '/storage/emulated/0' URI.

The fix, then, is to call StorageManagerCompat.deleteRoot(DEF_MAIN_ROOT) to ensure that there is no overlap before calling addRoot(). It's a simple, single-line fix, but it was obfuscated by all of the above complications as well as the inconsistency in behaviour across devices.

Now, the remaining question is: why was this issue so inconsistent across Android 10 devices? I'm not able to confirm this 100%, but I suspect the affected devices still had Build.VERSION.SDK_INT set to a value older than Q, causing StorageManagerCompat to store that invalid root URI. Other Android 10 devices where Build.VERSION.SDK_INT is correctly set to Q or higher avoid this issue because then StorageManagerCompat never stores that invalid URI in the first place, allowing the StorageManagerCompat.addRoot() call to succeed.


With all of this now sorted out, this fix is slated to be released in 1.6.2 once we receive testing results from a few more users. If you are reading this and own an affected device, please consider helping us with testing by sending an email to averistudios@gmail.com asking for a test APK.