Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WP-Android 8828] - Detect ArrayIndexOutOfBoundsException exception #861

Merged

Conversation

daniloercoli
Copy link
Contributor

This PR does add the ability to detect the ArrayIndexOutOfBoundsException that may happen on Android 8 at the following line of code android.text.DynamicLayout.getBlockIndex(DynamicLayout.java:646) due to a bug present in the Android Platform.
When this exact exception is detected, Aztec reports it to the parent app, by wrapping it into a custom Exception.

The reporting to the parent app happens by re-using the external logging capabilities of Aztec, instead of adding another layer of communication between Aztec and the parent app.
The parent app should just check the type of the exception in the "external" logger implementation passed to Aztec.

Make sure strings will be translated:

  • [ x ] If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O && Build.VERSION.SDK_INT <= Build.VERSION_CODES.O_MR1
&& ex is ArrayIndexOutOfBoundsException) {
val stackTrace = Log.getStackTraceString(ex)
if (stackTrace.contains("android.text.DynamicLayout.getBlockIndex(DynamicLayout.java:646)")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice checking for the line number string here. It makes it fine-grained so we avoid having false positives treated mistakingly 👍
Also, was wondering whether stackTrace could be null, so double checked getStackTraceString() and it seems it will never return null as per this so 👍 as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I detected an issue - this line will be different on Android 8.1 (API 27), see this #516

it will be java.lang.ArrayIndexOutOfBoundsException: length=39; index=-1 at android.text.DynamicLayout.getBlockIndex(DynamicLayout.java:648)

If you follow the steps to reproduce in the comment below on Android 8.1 (API 27) it will fail the string comparison @daniloercoli - can we add this other string for the case where Build.VERSION.SDK_INT == Build.VERSION_CODES.O_MR1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I've updated the code in 86a0000

Tried to be more verbose as possibile to let future devs to understand quickly why there are those if cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you 🙇

@@ -0,0 +1,5 @@
package org.wordpress.aztec.util
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about moving this class to a package of its own like org.wordpress.aztec.exception or org.wordpress.aztec.util.exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea. Addressed in 876ac04

…cLayoutGetBlockIndexOutOfBoundsException there
Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@mzorz
Copy link
Contributor

mzorz commented Oct 17, 2019

FYI just wanted to leave a note that this is easily reproduceable (so the code in this PR can be tested) by disabling the dynamicLayoutCrashPreventer InputFilter like this:

Comment out lines 528 in AztecText.kt like this:

//        if (Build.VERSION.SDK_INT == Build.VERSION_CODES.O || Build.VERSION.SDK_INT == Build.VERSION_CODES.O_MR1) {
//            // dynamicLayoutCrashPreventer needs to be first in array as these are going to be chained when processed
//            filters = arrayOf(dynamicLayoutCrashPreventer, emptyEditTextBackspaceDetector)
//        } else {
            filters = arrayOf(emptyEditTextBackspaceDetector)
//        }

Only leave filters = arrayOf(emptyEditTextBackspaceDetector) (line 532) uncommented.

Then follow these steps:

  1. start the demo app
  2. place the cursor at the beginning, right before the image
  3. enter anything
  4. observe the exception is thrown but caught by this code correctly

I've tested it and confirmed the crash is captured by the code in this branch and the call to the external logger is correctly made 👍

@jrcacd
Copy link

jrcacd commented Nov 7, 2019

How can we catch the ArrayIndexOutOfBoundsException exception?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants