aws-amplify / amplify-android

The fastest and easiest way to use AWS from your Android app.
https://docs.amplify.aws/lib/q/platform/android/
Apache License 2.0
246 stars 117 forks source link

ArrayIndexOutOfBoundsException caused by TransferStatusUpdater.unregisterListener$lambda$15 #2663

Open yaroslav-v opened 10 months ago

yaroslav-v commented 10 months ago

Before opening, please confirm:

Language and Async Model

Java

Amplify Categories

Storage

Gradle script dependencies

```groovy // Put output below this line implementation "com.amplifyframework:core:2.14.7" implementation "com.amplifyframework:aws-storage-s3:2.14.7" implementation "com.amplifyframework:aws-auth-cognito:2.14.7" ```

Environment information

``` # Put output below this line ------------------------------------------------------------ Gradle 8.1.1 ------------------------------------------------------------ Build time: 2023-04-21 12:31:26 UTC Revision: 1cf537a851c635c364a4214885f8b9798051175b Kotlin: 1.8.10 Groovy: 3.0.15 Ant: Apache Ant(TM) version 1.10.11 compiled on July 10 2021 JVM: 1.8.0_201 (Oracle Corporation 25.201-b09) OS: Mac OS X 10.16 x86_64 ```

Please include any relevant guides or documentation you're referencing

No response

Describe the bug

Hi there!

We have a few (3 at the moment) reports on Firebase with this crash. The issue appears on Android 13 only, affected devices include Samsung and Xiaomi.

The issue is quite rare and, unfortunately, there are no steps to reproduce it.

I believe you may have more clues why this might happen.

Reproduction steps (if applicable)

No response

Code Snippet

// Initialization in Application class

Amplify.Logging.disable();

Amplify.addPlugin(new AWSCognitoAuthPlugin());
Amplify.addPlugin(new AWSS3StoragePlugin());

AmplifyConfiguration amplifyConfig = AmplifyConfiguration.builder(context)
        .devMenuEnabled(false)
        .build();
Amplify.configure(amplifyConfig, context);

// Uploading, somewhere in the app

DateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ", Locale.US);
dateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));

Map<String, String> metadata = new ArrayMap<>(4);
metadata.put("date", dateFormat.format(date));
metadata.put("session_uuid", uid);
metadata.put("token", authToken);
metadata.put("service", serviceName);

StorageUploadFileOptions options =
        StorageUploadFileOptions.builder()
                .accessLevel(StorageAccessLevel.PUBLIC)
                .contentType(mimeType)
                .metadata(metadata)
                .build();

final File file = new File(path);
if (file.canRead()) {
    Amplify.Storage.uploadFile(
            file.getName(),
            file,
            options,
            result -> {
                // write success logs
            },
            failure -> {
                // write failure logs
            }
    );
}

Log output

``` // Put your logs below this line Fatal Exception: java.lang.ArrayIndexOutOfBoundsException length=1; index=1 java.util.ArrayList.remove (ArrayList.java:631) com.amplifyframework.storage.s3.transfer.TransferStatusUpdater.unregisterListener$lambda$15 (TransferStatusUpdater.kt:214) android.os.Handler.handleCallback (Handler.java:942) android.os.Handler.dispatchMessage (Handler.java:99) android.os.Looper.loopOnce (Looper.java:211) android.os.Looper.loop (Looper.java:300) android.app.ActivityThread.main (ActivityThread.java:8410) java.lang.reflect.Method.invoke (Method.java) com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:559) com.android.internal.os.ZygoteInit.main (ZygoteInit.java:954) ```

amplifyconfiguration.json

No response

GraphQL Schema

```graphql // Put your schema below this line ```

Additional information and screenshots

No response

tylerjroach commented 9 months ago

Thank you for the bug report. A member of our team will take a look.

tylerjroach commented 9 months ago
@Synchronized
fun unregisterListener(transferRecordId: Int, transferListener: TransferListener) {
    mainHandler.post {
        transferStatusListenerMap[transferRecordId]?.remove(transferListener)
    }
}

There appears to be a few faulty threading assumptions used here. This method is synchronized, but immediately calls mainHandler.post { } which means the actual work wont be done in the synchronized block.

There are some additional attempts at safeguard, such as transferStatusListenerMap being a ConcurrentHashMap. This area needs refactoring to ensure better thread safety.

tylerjroach commented 8 months ago

Wanted to add some additional research notes here.

Crash is java.util.ArrayList.remove (ArrayList.java:631) which shows the crash is coming from the remove(transferListener) call once a list is grabbed from the ConcurrentHashMap, transferStatusListenerMap.

Under the hood, this Kotlin helper method first does an indexOf(element), then a removeAt(index). My guess is that another area of code would have had to have shortened the list after indexOf(element) is called and before removeAt(index) is called.

public override fun remove(element: E): Boolean {
    val index = indexOf(element)
    if (index == -1) return false
    removeAt(index)
    return true
}

While this block in TransferStatusUpdater is a bit misguided because it attempts to synchronize and then goes out to a different thread, this is the only area of the code that removes items from the list. All the removes are posted to the same (main) thread so I would not expect any threading issues at this point.

@Synchronized
fun unregisterListener(transferRecordId: Int, transferListener: TransferListener) {
    mainHandler.post {
        transferStatusListenerMap[transferRecordId]?.remove(transferListener)
    }
}

Were still missing something since this crash is being observed, but I'm still not sure how this is happening.

xaluxs commented 7 months ago

I am getting the same error in firebase, here is the data

image

Fatal Exception: java.lang.ArrayIndexOutOfBoundsException: length=1; index=1
       at java.util.ArrayList.remove(ArrayList.java:631)
       at com.amplifyframework.storage.s3.transfer.TransferStatusUpdater.unregisterListener$lambda$15(TransferStatusUpdater.kt:214)
tylerjroach commented 7 months ago

@xaluxs Thanks for the additional detail. How many times have you seen this crash. Any other devices?

yaroslav-v commented 6 months ago

Hi guys!

I just want to give you some additional context. For the last 30 days this issue was registered for 13 users in Firebase.

The issue appears on Android 12-14, affected devices are Samsung (Galaxy A14, Galaxy S21 Ultra, Galaxy S21 FE etc.), Motorola (Moto G Stylus (2023)), Sony (Xperia 5 III), Ragentek - FIH Foxconn (WP23), Google.

The stack trace is the same for all cases.

AWS Amplify v2.14.11

yuhengshs commented 6 months ago

@yaroslav-v Thank you for your update, we will take a look and provide updates.

yaroslav-v commented 5 days ago

Hi guys! Do you have any news regarding this issue? I still see it in the logs from time to time even on the latest versions.

Here is a new log (almost identical to the original one):

Fatal Exception: java.lang.ArrayIndexOutOfBoundsException
length=1; index=1
java.util.ArrayList.remove (ArrayList.java:713)
com.amplifyframework.storage.s3.transfer.TransferStatusUpdater.unregisterListener$lambda$15 (TransferStatusUpdater.kt:207)
android.os.Handler.handleCallback (Handler.java:959)
android.os.Handler.dispatchMessage (Handler.java:100)
android.os.Looper.loopOnce (Looper.java:232)
android.os.Looper.loop (Looper.java:317)
android.app.ActivityThread.main (ActivityThread.java:8592)
java.lang.reflect.Method.invoke (Method.java)
com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:580)
com.android.internal.os.ZygoteInit.main (ZygoteInit.java:878)