1236705 - Regression in GeckoEditable.onTextChange when handling certain changes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This is a regression from bug 1051556, which had a patch to optimize GeckoEditable.onTextChange. However, that patch introduced a regression where data loss can occur if the changed text has a larger range than expected.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Correctly handle all text change scenarios (v1)

Handle all text change scenarios efficiently but correctly. In
particular, when we have a pending "replace text" action, make sure we
preserve old and new spans as much as we can, and make sure we merge the
text from the pending action with the actual changed text.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Will this fix bug 1236715?

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8703860 [details] [diff] [review]
Correctly handle all text change scenarios (v1)

Review of attachment 8703860 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoEditable.java
@@ +1058,3 @@
>                  // The new text exactly matches our sequence, so do a direct replace.
>                  geckoReplaceText(start, oldEnd, action.mSequence);
> +

Blank line.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Jim, could you fill the uplift request to aurora? Thanks

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed62b03205f

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ed62b03205f

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8703860 [details] [diff] [review]
Correctly handle all text change scenarios (v1)

Approval Request Comment

[Feature/regressing bug #]: Bug 1051556

[User impact if declined]: Crash on some pages

[Describe test coverage new/current, TreeHerder]: Locally, m-c

[Risks and why]: Small, patch introduces some new code, but it should be more robust than before

[String/UUID change made/needed]: None

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703860 [details] [diff] [review]
Correctly handle all text change scenarios (v1)

Fix a crash, taking it.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a919bac5d72

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

This is still happening in 45.0b3

https://crash-stats.mozilla.com/report/index/8ba5d6eb-ce2c-4221-a39e-f443a2160206

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

I'll open a new bug for the recent crashes.