wordpress-mobile / AztecEditor-Android

A reusable native Android rich text editor component.
Mozilla Public License 2.0
675 stars 112 forks source link

Add a character before the picture in demo app crashes Aztec #729

Closed daniloercoli closed 5 years ago

daniloercoli commented 5 years ago

This is weird. I tried to add some character before the first picture in demo app and Aztec crashed with the following stack trace:

                  java.lang.ArrayIndexOutOfBoundsException: length=39; index=-1
                      at android.text.DynamicLayout.getBlockIndex(DynamicLayout.java:646)
                      at android.widget.Editor.drawHardwareAccelerated(Editor.java:1787)
                      at android.widget.Editor.onDraw(Editor.java:1756)
                      at android.widget.TextView.onDraw(TextView.java:7723)
                      at android.view.View.draw(View.java:20338)
                      at android.view.View.updateDisplayListIfDirty(View.java:19283)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.updateDisplayListIfDirty(View.java:19274)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.draw(View.java:20341)
                      at android.widget.ScrollView.draw(ScrollView.java:2731)
                      at android.view.View.updateDisplayListIfDirty(View.java:19283)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.updateDisplayListIfDirty(View.java:19274)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.updateDisplayListIfDirty(View.java:19274)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.updateDisplayListIfDirty(View.java:19274)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.updateDisplayListIfDirty(View.java:19274)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.updateDisplayListIfDirty(View.java:19274)
                      at android.view.View.draw(View.java:20061)
                      at android.view.ViewGroup.drawChild(ViewGroup.java:4421)
                      at android.view.ViewGroup.dispatchDraw(ViewGroup.java:4207)
                      at android.view.View.draw(View.java:20341)
                      at com.android.internal.policy.DecorView.draw(DecorView.java:979)
                      at android.view.View.updateDisplayListIfDirty(View.java:19283)
                      at android.view.ThreadedRenderer.updateViewTreeDisplayList(ThreadedRenderer.java:686)
                      at android.view.ThreadedRenderer.updateRootDisplayList(ThreadedRenderer.java:692)
                      at android.view.ThreadedRenderer.draw(ThreadedRenderer.java:800)
                      at android.view.ViewRootImpl.draw(ViewRootImpl.java:3451)
                      at android.view.ViewRootImpl.performDraw(ViewRootImpl.java:3238)
                      at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2773)
                      at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1742)
                      at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:7763)
                      at android.view.Choreographer$CallbackRecord.run(Choreographer.java:911)
                      at android.view.Choreographer.doCallbacks(Choreographer.java:723)
                      at android.view.Choreographer.doFrame(Choreographer.java:658)
                      at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897)
                      at android.os.Handler.handleCallback(Handler.java:789)
                      at android.os.Handler.dispatchMessage(Handler.java:98)
                      at android.os.Looper.loop(Looper.java:164)
                      at android.app.ActivityThread.main(ActivityThread.java:6938)
                      at java.lang.reflect.Method.invoke(Native Method)
                      at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:327)
                      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1374)

I've been able to reproduce the issue on Android 8.0 on both emulators, and real devices.

Steps to repro:

Would be cool if someone else can verify this report, since looking at crash report it seems something internal to Android.

Tested on develop running on Android 8.0.

koke commented 5 years ago

Tested on a Nexus 5 with 10.1 and it didn't crash

daniloercoli commented 5 years ago

Reproduced on Aztec 1.3.6.

daniloercoli commented 5 years ago

This is related to https://github.com/wordpress-mobile/AztecEditor-Android/issues/516

Fabric.io reports: 5a04a91461b02d480d160132 , 5a86f7f48cb3c2fa632d5149 , and 59b55f70be077a4dcc699371

daniloercoli commented 5 years ago

We can try to follow the suggestion reported in the Google bug report and disable Hardware acceleration. editText.setLayerType(View.LAYER_TYPE_SOFTWARE, null); Not quite sure if disabling HW Acceleration could open other potential problems.

hypest commented 5 years ago

Just commenting on the potential fix by using software rendering, I'd consider dropping hardware acceleration quite an extreme measure, one that would most probably harm Aztec's performance even more.

daniloercoli commented 5 years ago

Unfortunately doesn't seem other options are available to fix the issue. Except waiting until Google fixes it in the platform. We're waiting since 2017 anyway. In the meantime the number of crashes is raising (if you sum all the HardwareAccelerated crashes in fabric.io the number is going to be consistent).

hypest commented 5 years ago

Will assign this issue to myself and try to investigate as well.

koke commented 5 years ago

Even if there is no other solution, this crash seems to be affecting less than 0.07% of our users (if I counted this right). I'd imagine disabling hardware acceleration could potentially have worse effects on a much bigger set of users.

daniloercoli commented 5 years ago

I'm counting 11.22% of crashes in 10.2 due to this problem. Didn't count the #of users.

koke commented 5 years ago

That's right, but 10.2 has had 99.63% of crash-free sessions, so that's 11% of the 0.4% of crashed sessions, or ~0.04% of the total number of sessions.

It is one of the top Android crashes, and if we can find a way to fix it let's do it. Just not at the expense of degrading performance for the other 99%.

daniloercoli commented 5 years ago

Well, if we want to have a clear picture we should only count those sessions that involved open_editor event. Without that, those numbers don't have much meaning IMO.

Or we can deduct it in another way...calculate the % of all the total sessions that called open_editor and use it to properly calculate the free-sessions with editor.

khaykov commented 5 years ago

Based on the discussions about similar (same?) same issue in Aztec repo this might be the OS that was tracked by Google and fixed in newer OS version.

If someone else can confirm this we can probably close this issue (I assmue we are not going to disable HWA).

drakeet commented 5 years ago

The Oreo bug I also encountered this problem in my application, I tried to follow the "turn off hardware acceleration" method discussed in the Google issue, but it will cause the page to not render completely (it seems to be blank), so that it is a method that does not work for me.

drakeet commented 5 years ago

I found a better workaround: if we remove all values < 0 of DynamicLayout before onDraw(), the crash will be fixed, and the text view can perform well. I can reproduce it on my app Pure Writer, so I am sure the workaround works for it.

drakeet commented 5 years ago

If it is acceptable, I can create a PR or gist for it.

hypest commented 5 years ago

👋 @drakeet !

values < 0 of DynamicLayout

I'm kinda confused about what that means, can you elaborate? Also, are those an indication that there's a bug somewhere else? It would be awesome if you could continue investigating towards finding the root cause of the crash here or maybe offer some insight if you have it from your experience with your own app.

I'd say that the suggestion in https://github.com/wordpress-mobile/AztecEditor-Android/issues/729#issuecomment-430105339 feels a bit more hacky than we'd ideally want so, perhaps a PR with that won't get enough traction. That said, since I don't quite understand the suggestion so far, happy to revise after you expand it. Thanks!

mzorz commented 5 years ago

Closing via #801