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

JSApplicationIllegalArgumentException: Error while updating property 'text' of a view managed by: RCTAztecView #9832

Closed
sentry-io bot opened this issue May 10, 2019 · 32 comments

Comments

@sentry-io
Copy link

sentry-io bot commented May 10, 2019

Sentry Issue: https://sentry.io/share/issue/c467f624ddc244c593cf6442ae7d3cc2/ https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/ (broken link fixed 1/29/20)

One way to reproduce the issue in comments below.

IndexOutOfBoundsException: setSpan (22 ... 22) ends beyond length 5
    at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:1327)
    at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:688)
    at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:680)
    at android.text.Selection.setSelection(Selection.java:93)
    at android.text.Selection.setSelection(Selection.java:77)
...
(29 additional frame(s) were not displayed)

InvocationTargetException: None
    at java.lang.reflect.Method.invoke(Method.java)
    at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:83)
    at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132)
    at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51)
    at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37)
...
(20 additional frame(s) were not displayed)

JSApplicationIllegalArgumentException: Error while updating property 'text' of a view managed by: RCTAztecView
    at com.facebook.react.uimanager.ViewManagersPropertyCache$PropSetter.updateViewProp(ViewManagersPropertyCache.java:95)
    at com.facebook.react.uimanager.ViewManagerPropertyUpdater$FallbackViewManagerSetter.setProperty(ViewManagerPropertyUpdater.java:132)
    at com.facebook.react.uimanager.ViewManagerPropertyUpdater.updateProps(ViewManagerPropertyUpdater.java:51)
    at com.facebook.react.uimanager.ViewManager.updateProperties(ViewManager.java:37)
    at com.facebook.react.uimanager.NativeViewHierarchyManager.updateProperties(NativeViewHierarchyManager.java:136)
...
(19 additional frame(s) were not displayed)

Error while updating property 'text' of a view managed by: RCTAztecView
@designsimply
Copy link
Contributor

designsimply commented Aug 9, 2019

30-day impact: ~31 per day
Users affected: 449 in the last 30d
Last seen in: 12.4 (latest official release at the time of this comment)

https://sentry.io/share/issue/c467f624ddc244c593cf6442ae7d3cc2/

~Approx. number of crashes went from ~10/day to 1 or 2/day on 2019-07-31 when 12.9 was released. Recommend closing after a check from a Groundskeeper to confirm. Ignore this note, there were a lot more reports but they hadn't yet been combined into one Sentry issue.

@mkevins
Copy link
Contributor

mkevins commented Aug 14, 2019

@malinajirka certainly! 👍 I'll look into it.

@mkevins
Copy link
Contributor

mkevins commented Aug 15, 2019

There are a few IndexOutOfBoundsException-related crashes with various root causes, however, this one looks familiar. What these all seem to have in common is that they occur during a call from ReactAztecManager to the underlying EditText methods (notably, setSpan on android.text.SpannableStringBuilder).

There is a comment here that may be relevant: https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/component/index.native.js#L800 as it describes a workaround used in Gutenberg code to avoid setting the selection out of bounds. It works by anticipating AztecText's mutation of the text buffer, in it's "clean-up" of html. When it detects that spaces will be trimmed, it safely avoids setting the selection.

@mkevins
Copy link
Contributor

mkevins commented Aug 15, 2019

Another potential clue on this crash is that many of the exceptions are "off-by-one". I have been using the following jq script to parse the sentry json data, to try to find a pattern:

 #!/bin/bash
jq '
( 
 .exception.values[]
   | select( .type | contains("IndexOutOfBoundsException") )
   | { 
       value,
       offBy: ( [.value | capture("(?<n>\\d+)"; "g")]
         | map(.n|tonumber)
         | .[2]-.[1]
       ),
       modules: (.stacktrace.frames[-7:]
         #| map(.function) 
         #| map(.abs_path) 
         | map(.module) 
         #| map(.in_app) 
         #| map(.lineno) 
         #| map(.filename) 
         #|join(",") 
       )
     }
) +
( { breadcrumbs: 
 [
   .breadcrumbs.values[]
     #| select( .message | contains("WordPress-EDITOR") )
     | select( .message | contains("will be trimmed for spaces") )
     #| select( .message | contains("Processed HTML") )
     | .message
 ][-1]

} )

'

Via cat sentry-file-*|./parse-9832.

@mkevins
Copy link
Contributor

mkevins commented Aug 15, 2019

8 of the 14 cases listed have an IndexOutOfBoundsException caused by the index being off by 1:

Parsed output
{
  "value": "setSpan (29 ... 29) ends beyond length 28",
  "offBy": -1
}
{
  "value": "setSpan (426 ... 426) ends beyond length 425",
  "offBy": -1
}
{
  "value": "setSpan (481 ... 481) ends beyond length 480",
  "offBy": -1
}
{
  "value": "setSpan (345 ... 345) ends beyond length 319",
  "offBy": -26
}
{
  "value": "setSpan (52 ... 52) ends beyond length 31",
  "offBy": -21
}
{
  "value": "setSpan (126 ... 126) ends beyond length 125",
  "offBy": -1
}
{
  "value": "setSpan (187 ... 187) ends beyond length 186",
  "offBy": -1
}
{
  "value": "setSpan (335 ... 335) ends beyond length 327",
  "offBy": -8
}
{
  "value": "setSpan (326 ... 326) ends beyond length 325",
  "offBy": -1
}
{
  "value": "setSpan (4 ... 4) ends beyond length 1",
  "offBy": -3
}
{
  "value": "setSpan (57 ... 57) ends beyond length 56",
  "offBy": -1
}
{
  "value": "setSpan (143 ... 143) ends beyond length 132",
  "offBy": -11
}
{
  "value": "setSpan (664 ... 664) ends beyond length 663",
  "offBy": -1
}
{
  "value": "setSpan (341 ... 341) ends beyond length 339",
  "offBy": -2
}

@mkevins
Copy link
Contributor

mkevins commented Aug 15, 2019

I noticed that in some cases, the modules listed in the stacktrace leading up to android.text.SpannableStringBuilder were not always the same. In some cases, android.widget.TextView showed up, and in other cases, not. This may be a clue to what code paths are being touched, however, it did not appear to correlate to the differences "off-by-one" cases.

Also, it is a bit puzzling to see that this crash sometimes occurs, even when the breadcrumbs show that willTrimSpaces returned true:

Parsed output
{
  "value": "setSpan (29 ... 29) ends beyond length 28",
  "offBy": -1,
  "modules": [
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (426 ... 426) ends beyond length 425",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (481 ... 481) ends beyond length 480",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (345 ... 345) ends beyond length 319",
  "offBy": -26,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": "w/WordPress-EDITOR: RichText value will be trimmed for spaces! Avoiding setting the caret position manually."
}
{
  "value": "setSpan (52 ... 52) ends beyond length 31",
  "offBy": -21,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (126 ... 126) ends beyond length 125",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (187 ... 187) ends beyond length 186",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (335 ... 335) ends beyond length 327",
  "offBy": -8,
  "modules": [
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": "w/WordPress-EDITOR: RichText value will be trimmed for spaces! Avoiding setting the caret position manually."
}
{
  "value": "setSpan (326 ... 326) ends beyond length 325",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (4 ... 4) ends beyond length 1",
  "offBy": -3,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (57 ... 57) ends beyond length 56",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (143 ... 143) ends beyond length 132",
  "offBy": -11,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (664 ... 664) ends beyond length 663",
  "offBy": -1,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.widget.TextView",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}
{
  "value": "setSpan (341 ... 341) ends beyond length 339",
  "offBy": -2,
  "modules": [
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "org.wordpress.mobile.ReactNativeAztec.ReactAztecManager",
    "android.widget.EditText",
    "android.text.Selection",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder",
    "android.text.SpannableStringBuilder"
  ],
  "breadcrumbs": null
}

@mkevins
Copy link
Contributor

mkevins commented Aug 15, 2019

I also noticed that some, but not all of the cases had a breadcrumb with a message containing Processed HTML, which is logged from within the pasteHandler. Unfortunately, there was no obvious pattern or correlation to the "off-by-one" cases. As a next step, I will use the underlying content to try to reproduce the crash on the demo app. Perhaps there is something particular about the content itself, or the way it is parsed by Aztec that is leading to this exception.

@designsimply
Copy link
Contributor

I merged all the issues to one in Sentry. Here are the updated numbers for reference.

90-day impact: ~111 per day
Users affected in the last 90d: 3.2k
Last seen in: 12.9

https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

@designsimply
Copy link
Contributor

90-day impact: ~200 per day
Users affected in the last 90 days: 5200

https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

@hypest
Copy link
Contributor

hypest commented Sep 19, 2019

👋 @mkevins , let me know if you're still working on this ticket, thanks!

@mkevins
Copy link
Contributor

mkevins commented Sep 23, 2019

Hi @hypest 👋

I tried this next step, but was unable to reproduce using a few samples of content (from the crash reports):

As a next step, I will use the underlying content to try to reproduce the crash on the demo app.

Since then, I have unassigned myself.

@designsimply
Copy link
Contributor

90-day impact: ~333 per day
Users affected in the last 90 days: 8900

https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

Looks like a widespread crash happening for a lot of users. Let's raise the priority on this one.

@daniloercoli
Copy link
Contributor

My first attempt on fixing this issue was based on the following idea:

  1. Catch this specific crash in ReactNativeAztec updateSelectionIfNeeded method
  2. Throw a new Custom exception
  3. Catch the exception in the GB Editor Fragment
  4. Log the exception in Sentry, DO NOT crash the app

I've made this branch here: https://github.com/wordpress-mobile/WordPress-Android/tree/issue/9832-Aztec-crash-Span-Out-of-bounds
and the GB-mobile counter part here: https://github.com/wordpress-mobile/gutenberg-mobile/tree/issue/wp-android-9832-crash-Span-Out-of-bounds

The key point is calling the setNativeModuleCallExceptionHandler method of ReactInstanceManagerBuilder, and introduce a special version of the DefaultNativeModuleCallExceptionHandler used in non-debug mode.

Unfortunately when the error does happen, the ReactInstanceManager does properly call the custom handler and log the error in Sentry, but the ReactInstance does remain in a broken state, and the current block is froze. The UI is not responding anymore to the taps...

I'm not going to update this proposed solution above, since it's already a bit too convoluted, and not working fine.
I think we should find another way to give to the Aztec Wrapper the ability to log exception in the main app, without crashing it. This feature is already available in Aztec via the "external logger." So that Aztec doesn't know the actual implementation of the logger.

Reporting here a suggestion by @mzorz
I would it be possible to make a Sentry ReactNative wrapper component (like we have our Aztec ReactNative wrapper) and so the exception caught from Aztec RN can be sent to this other component? Just a thought, do you think this could work?

@cameronvoell cameronvoell self-assigned this Jan 31, 2020
@designsimply
Copy link
Contributor

Events in the last 90 days: 93,000
Users affected in the last 90 days: 15,000
https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

@designsimply designsimply removed the Aztec label Mar 5, 2020
@designsimply
Copy link
Contributor

Events in the last 90 days: 100,000
Users affected in the last 90 days: 16,000
Devices affected: looks to be mainly Samsung and Redmi (Xiaomi) devices
https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

@geriux
Copy link
Contributor

geriux commented Mar 30, 2020

I'll take a look at this since I have a Redmi (Xiaomi) device, I'll start by trying to reproduce the crash.

@geriux
Copy link
Contributor

geriux commented Apr 3, 2020

So I had some time to test this issue reviewing the reports from Sentry.

Found three cases that make the editor crash, I replaced the content with placeholder texts.

[cp]Text wihtin a tag​​​[/cp]
<em>Text within a tag​​​</em>.
<mark class="hl_yellow">"My text”,&nbsp;</mark>other text.

Block snippet example

<!-- wp:paragraph -->
<p><mark>"My text”, </mark>other text.</p>
<!-- /wp:paragraph -->

Also, some of the crashes I saw were with languages other than English so maybe there's an issue with some characters.

The steps to reproduce with the cases from above:

  • Create a post from the web editor with the block snippet example
  • Open the post from the app
  • Without selecting anything, add a Paragraph block from the inserter menu.
  • Once the block is added tap on the backspace key to merge the blocks
  • Crash

My knowledge of Aztec is pretty limited still but maybe someone else has more experience with it to know what could be happening?

@designsimply
Copy link
Contributor

1,656 events have been tracked for this crash in 14.6.1 since it was released 9d ago on Apr 22.

Events in the last 90 days: 127,000
Users affected in the last 90 days: 19,000
https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

@hypest
Copy link
Contributor

hypest commented May 6, 2020

@mchowning, can you put this one at the top of your attention list? Thanks!

@designsimply
Copy link
Contributor

designsimply commented May 19, 2020

3,236 events have been tracked for this crash in 14.7 since it was released 15d ago on May 4.

Events in the last 90d: 137,000
Users affected in the last 90d: 20,000
https://sentry.io/share/issue/91a4511e98264a9b97a16db7b5486a36/

Update to add a graph and note that events per day looks to be steadily rising for this one:

Screen Shot 2020-05-19 at May 19 5 50 46 PM

@designsimply
Copy link
Contributor

@geriux can you help with a code review on this one? Nice find on the steps to reproduce btw!! Thank you for the PR @mchowning. 🙇

@mchowning
Copy link
Contributor

So far I only opened a related issue in the gb-mobile repository, but I'm hoping to open a PR today! So there's nothing to review (quite) yet @geriux . 🙂

@mchowning
Copy link
Contributor

With the 14.9 release, this issue should no longer be causing a crash for our users. We have not resolved the underlying issue though, we've just stopped it from causing a crash. Any time the crash would have occurred, we are now sending a non-crashing IllegalSelectionIndexException like this to Sentry, so I would expect a lot of those exceptions to start showing up. This exception does not crash the app, and it includes more information about the state of the app when the issue arises. I'm hoping we can use that information to help us address the underlying issue.

@designsimply
Copy link
Contributor

Thank you @mchowning!

Noting that 14.9 was released on 2020-06-01 and this 90d graph will be a good one to watch over the next few weeks. Currently, in terms of crashes in that issue in Sentry, it has gone from over 500 events per day in May to only ~100 per day in the last few days.

I see that wordpress-mobile/gutenberg-mobile#2348 was opened to track the non-crashing error, @mchowning should we close this issue in favor of the new one with a link back to here for reference or do we need to watch both for the next few weeks to see what falls out?

@mchowning
Copy link
Contributor

mchowning commented Jun 8, 2020

I see that wordpress-mobile/gutenberg-mobile#2348 was opened to track the non-crashing error, @mchowning should we close this issue in favor of the new one with a link back to here for reference or do we need to watch both for the next few weeks to see what falls out?

I don't think we need to keep this one open since there are no crashes in 14.9. I went ahead and added a comment linking to this issue in wordpress-mobile/gutenberg-mobile#2348, so I'll go ahead and close this.

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

Successfully merging a pull request may close this issue.

9 participants