klaviyo/klaviyo-android-sdk

Klaviyo.setPushToken() may cause app crash

Closed this issue · 5 comments

Jenkyn commented

Description

We have observed a significant number of IllegalThreadStateException exceptions for our app in the Google Play Store Console. Upon examining the stack trace, it appears that these exceptions are triggered by the invocation of Klaviyo.setPushToken().

Checklist

  • I have determined whether this bug is also reproducible in a vanilla Android project
  • If possible, I've reproduced the issue using the main branch of this package.
  • This issue hasn't been addressed in an existing GitHub issue or pull request.

Expected behavior

The app run normally and no crash

Actual behavior

The app performs well in most cases, but on some case, it may lead to crashes (about 1% probability).

Steps to reproduce

1、Integrate the SDK within the app and initialize it in the Application class. Code snippet as below:
     
Klaviyo.initialize(“klaviyo_public_key”, context)
FirebaseMessaging.getInstance().token.addOnSuccessListener { token ->
    Klaviyo.setPushToken(token)
}

2、Configure the KlaviyoPushService in the manifest。
3、Run the app, the app may throw IllegalThreadStateException. The stack trace as below.

Fatal Exception: java.lang.IllegalThreadStateException:
       at java.lang.Thread.start(Thread.java:872)
       at com.klaviyo.analytics.networking.KlaviyoApiClient.initBatch(KlaviyoApiClient.kt:205)
       at com.klaviyo.analytics.networking.KlaviyoApiClient.enqueueRequest(KlaviyoApiClient.kt:113)
       at com.klaviyo.analytics.networking.KlaviyoApiClient.enqueuePushToken(KlaviyoApiClient.kt:81)
       at com.klaviyo.analytics.Klaviyo.setPushToken(Klaviyo.kt:153)
       at com.klaviyo.pushFcm.KlaviyoPushService.onNewToken(KlaviyoPushService.kt:29)
       at com.google.firebase.messaging.FirebaseMessagingService.handleIntent(FirebaseMessagingService.java:163)
       at com.google.firebase.messaging.EnhancedIntentService.lambda$processIntent$0(EnhancedIntentService.java:78)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
       at com.google.android.gms.common.util.concurrent.zza.run(com.google.android.gms:play-services-basement@@18.1.0:2)
       at java.lang.Thread.run(Thread.java:923)

or 

Fatal Exception: java.lang.IllegalThreadStateException:
       at java.lang.Thread.start(Thread.java:960)
       at com.klaviyo.analytics.networking.KlaviyoApiClient.initBatch(KlaviyoApiClient.kt:205)
       at com.klaviyo.analytics.networking.KlaviyoApiClient.enqueueRequest(KlaviyoApiClient.kt:113)
       at com.klaviyo.analytics.networking.KlaviyoApiClient.enqueuePushToken(KlaviyoApiClient.kt:81)
       at com.klaviyo.analytics.Klaviyo.setPushToken(Klaviyo.kt:153)
       at com.myapp.app.startup.KlaviyoInitializer.create$lambda-0(KlaviyoInitializer.kt:17)
       at com.google.android.gms.tasks.zzm.run(com.google.android.gms:play-services-tasks@@18.0.1:1)
       at android.os.Handler.handleCallback(Handler.java:942)
       at android.os.Handler.dispatchMessage(Handler.java:99)
       at android.os.Looper.loopOnce(Looper.java:226)
       at android.os.Looper.loop(Looper.java:313)
       at android.app.ActivityThread.main(ActivityThread.java:8757)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:571)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1067)


From the stack trace, we learn that this issue arises because line 205 of KlaviyoApiClient.kt is executed twice, indicating that handlerThread.start() is being called twice. This is the root cause of the IllegalThreadStateException being thrown.


From the code analysis, it's clear that Klaviyo.setPushToken() is being called in both the callback registered in the Application's onCreate() and in KlaviyoPushService's onNewToken(). If you check the thread id in the logs, you will find that Klaviyo.setPushToken() is running on the main thread within the Application's callback, while in KlaviyoPushService, it's executed on a worker thread. Since Klaviyo.setPushToken() lacks synchronization and is not thread-safe, in a multi-threaded scenario, it's possible for the handlerThread.start() to be executed twice due to a race condition. This leads to the occurrence of the error.

The Klaviyo Android SDK version information

1.3.1

Device Information

No specific

Android Studio Version

No specific

Android API Level

No specific

Jenkyn commented

Seems that jordan has fix it: #93

jordan-griffin commented

Hey, @Jenkyn thanks for opening the issue. We just wanted to gather a bit more information if possible. Though we do feel that the open PR #93 will address the issue, we have struggled to reproduce the issue locally in a multi-threaded setup.

Any more information you can provide on your own setup, how your onCreate looks, would help us dig into this problem further.

We're also curious to know if you guys encountered this locally or if its just occurring in your production release - though sounds like with the 1% probability crash rate you are seeing, this has likely not been easy for you to replicate locally either.

Jenkyn commented

The code snippet above in step 1 is similar to the code in the onCreate method.

We encountered this issue in the production release and can't reproduce it locally. The 1% crash rate represents our app's crash rate, which is mostly caused by this exception.

jordan-griffin commented

Gotcha, thanks. We've already tried replicating the case from the code snippets you provided before but sounds like this is something that has not been replicated by either side locally. We'll hopefully have the fix released after the holiday weekend here.

jordan-griffin commented

v1.3.3 of the SDK has been released containing a potential fix for this issue. It should handle the following cases:

  • Apps with multithreading that may generate a race condition initializing a threaded network request to the SDK on startup.
  • Apps where the SDK's networking thread has terminated, we now will initialize a new thread in its place instead of generating a crash trying to re-use the original thread.

If you continue to encounter problems, please feel free to open another issue against the SDK and we'll look into it.

  • 👍1