1051556 - crash in java.lang.IllegalArgumentException: invalid selection notification range at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java)

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-66c714af-d11f-433a-835e-fe5fb2140808.
=============================================================

java.lang.IllegalArgumentException: invalid selection notification range: 116466 to 116466, length: 71
	at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:815)
	at org.mozilla.gecko.GeckoAppShell.notifyIMEChange(GeckoAppShell.java:474)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:353)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:178)

Comment 2

[Aaron Train [:aaronmt]]

Oddly only two reports from this crash signature are using 34.0 (20140807) builds.

The common URL's are Etherpad again.

Comment 3

[Robert Kaiser]

(In reply to Aaron Train [:aaronmt] from comment #2)
> Oddly only two reports from this crash signature are using 34.0 (20140807)
> builds.

Actually, there are many reports with different numbers: https://crash-stats.mozilla.com/search/?signature=~invalid+selection+notification&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform

Comment 4

[Aaron Train [:aaronmt]]

Still seeing signatures on 35

Comment 5

[Aaron Train [:aaronmt]]

Still seeing these reports on 36

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

Possible str https://support.mozilla.org/en-US/questions/1040934#answer-679203

Comment 12

[Aaron Train [:aaronmt]]

I'm sick of these duplicate reports. This is probably a simple fix, can we get this looked at?

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1051556-Simplify-invalid-selection-exception-mes.patch

Let's unify the crash signatures first to get a better overview on affected URLs and devices by moving the crash details to the log.

Comment 14

[Aaron Train [:aaronmt]]

(In reply to Eugen Sawin [:esawin] from comment #13)
> Created attachment 8572627 [details] [diff] [review]
> 0001-Bug-1051556-Simplify-invalid-selection-exception-mes.patch
> 
> Let's unify the crash signatures first to get a better overview on affected
> URLs and devices by moving the crash details to the log.

This might be unnecessary, kbrosnan or kairo might know a soccoro trick

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1051556-Simplify-GeckoEditable-exception-message.patch

Just extending the previous patch to all exceptions with dynamic values with high ranges, let's keep reports sane.

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1051556-Simplify-GeckoEditable-exception-message.patch

Fixed syntax error.

Comment 17

[Eugen Sawin [:esawin]]

Discussed on IRC.

Comment 18

[Eugen Sawin [:esawin]]

https://hg.mozilla.org/integration/fx-team/rev/0ca8525ca4ec

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ca8525ca4ec

Comment 20

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1051556-Simplify-GeckoEditable-exception-message.patch

Looks like I've attached the old version twice, here is the corrected version (just a syntax fix).

Comment 21

[Eugen Sawin [:esawin]]

Comment on attachment 8573572 [details] [diff] [review]
0001-Bug-1051556-Simplify-GeckoEditable-exception-message.patch

I would like to uplift this to beta for maximum exposure.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: the crash signature contains highly dynamic values which makes report analysis difficult
[Describe test coverage new/current, TreeHerder]: none required
[Risks and why]: none, this is just a change in the text message of the thrown exception
[String/UUID change made/needed]: none

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8573572 [details] [diff] [review]
0001-Bug-1051556-Simplify-GeckoEditable-exception-message.patch

OK. Let's take this patch that only impacts logging and exception messages to get better information about the crash. Beta+ Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/359e3680014f
https://hg.mozilla.org/releases/mozilla-beta/rev/73c7414f883d

Comment 24

[Robert Kaiser]

(In reply to Aaron Train [:aaronmt] from comment #14)
> (In reply to Eugen Sawin [:esawin] from comment #13)
> > Created attachment 8572627 [details] [diff] [review]
> > 0001-Bug-1051556-Simplify-invalid-selection-exception-mes.patch
> > 
> > Let's unify the crash signatures first to get a better overview on affected
> > URLs and devices by moving the crash details to the log.
> 
> This might be unnecessary, kbrosnan or kairo might know a soccoro trick

You'll need a bug filed against Socorro and a regex to match as well as what to replace it with, like in bug 726385 or bug 865142

Comment 25

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: signature consolidation, no direct user impact but would help the Android team prioritize this bug. Ride along if there is a 36.0.2

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8573572 [details] [diff] [review]
0001-Bug-1051556-Simplify-GeckoEditable-exception-message.patch

[Triage Comment]

Taking it for release. Cf comment #25

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f91f9947247a

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

Don't need the old signature examples now that this is on release.

Comment 29

[Kevin Brosnan [Ex-Mozilla]]

This ends up around number 15 crash in 36.0.x

Comment 30

[Eugen Sawin [:esawin]]

Bug 1148590 handles the crash caused by out-of-editor selection, but there might be other triggers, too.

Comment 31

[Eugen Sawin [:esawin]]

It looks like we still get crash reports with bug 1148590 fixed, so there must be additional causes for this.

Comment 32

[Flaviu Cos]

I can reproduce this crash with this signature:
java.lang.IllegalArgumentException: invalid selection notification range at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java)

Environment: 
Device: Nexus 5 (Android 6.0);
Build: Nightly 45.0a1 (2015-11-16);


Steps to reproduce:
1. Go to http://www-archive.mozilla.org/quality/browser/front-end/testcases/wallet/login.html (short URL: http://goo.gl/vi7z6);
2. Login with random username/passowrd;
3. Tap on the android back button;
4. Tap on the username field and delete one or more characters from the current username;
5. Tap on the saved form history.


Notes:
For further details please check the following video:
https://youtu.be/Q5Pgrv7kFF0

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Snorp do you think this is worth a look? Maybe we have a chance to fix this a bit more, now that we can reproduce it. 
This signature is fairly high volume on 43 beta (around 140 crashes per version)

Comment 34

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

-> jchen?

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I looked at both the crash signatures and 44.0a2 hits and I was unable to get any data on the 2nd signature. For the first one, there have been < 10 crashes in last 28 days, so I do not feel the need to track this for 44. Please re-nominate if there is a crash spike or if you disagree with my assessment.

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

I think it's fine not to track it; we don't get a lot of Firefox for Android data on the aurora channel, so it's often hard to tell which crashes are important there until that version moves to beta.

Comment 37

[Wellington Torrejais da Silva]

Crash in login pages with saved data(auto complete): https://crash-stats.mozilla.com/report/index/61d2af66-2e60-41cb-9b65-ff6e12151129

Comment 38

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

Attachment: Convert testInputConnection to a JS test (v1)

A test for this bug requires the Robocop test call some JS functions.
This patch gives testInputConnection the ability to make calls to
content JS. It also converts the existing tests to use JS instead of
using previous workarounds (e.g. using URL hash to set the initial text,
using tab key to change input focus).

Comment 39

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

Attachment: Add test for re-flushing changes (v1)

This patch adds a test for this bug by ending an existing composition followed
by setting the value of the input field.

Comment 40

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

Attachment: Make GeckoEditable.onTextChange more efficient (v1)

This patch simplifies the onTextChange method, makes it more efficient
by avoiding unnecessary text copying, and fixes some small bugs.

Comment 41

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

Attachment: Re-flush IME changes when querying text triggers more changes (v1)

We send query text events when flushing IME changes, and sometimes these
events make Gecko commit more pending changes. In that case, we should
try flushing again, so we pick up the new changes.

This patch also makes the process of flushing text changes
transactional, so that if we have to bail due to more pending changes,
nothing will be committed.

Comment 42

[Teodora Vermesan (:TeoVermesan)]

I can reproduce this crash on latest Nightly using Nexus 4 (Android 5.1.1)
1. Go to twitter.com
2. Type "teo" in the username text field
3. From the autocomplete pop-up choose "teovermesan"

=> Nightly crashes

Comment 43

[David Weir (satdav)]

Also got this on nighly today would be great for a fix asap

Comment 44

[Jack W]

Getting this on Firefox 44a2, reproducible every time when content is injected into an instance of CKEditor by a specific script. However, I haven't been able to reproduce it on FF42 release for some reason.

Comment 45

[Eugen Sawin [:esawin]]

Comment on attachment 8695471 [details] [diff] [review]
Add test for re-flushing changes (v1)

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

::: mobile/android/tests/browser/robocop/robocop_input.html
@@ +23,5 @@
>            input.focus();
>          },
>  
> +        test_reflush_changes: function() {
> +          let inputEditable = SpecialPowers.wrap(input).QueryInterface(SpecialPowers.Ci.nsIDOMNSEditableElement);

Indentation

@@ +24,5 @@
>          },
>  
> +        test_reflush_changes: function() {
> +          let inputEditable = SpecialPowers.wrap(input).QueryInterface(SpecialPowers.Ci.nsIDOMNSEditableElement);
> +          let inputIme = inputEditable.editor.QueryInterface(SpecialPowers.Ci.nsIEditorIMESupport);

Indentation

Comment 46

[Eugen Sawin [:esawin]]

Comment on attachment 8695473 [details] [diff] [review]
Re-flush IME changes when querying text triggers more changes (v1)

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

::: mobile/android/base/GeckoEditable.java
@@ +1004,5 @@
>          /* For the "end" parameters, Gecko can pass in a large
>             number to denote "end of the text". Fix that here */
>          final int oldEnd = unboundedOldEnd > mText.length() ? mText.length() : unboundedOldEnd;
>          // new end should always match text
> +        if (unboundedOldEnd < Integer.MAX_VALUE / 2 && unboundedNewEnd != (start + text.length())) {

Can you break that up into two lines for improved readability?

::: widget/android/nsWindow.cpp
@@ +240,5 @@
>          , mIMERanges(new TextRangeArray())
>          , mIMEMaskEventsCount(1) // Mask IME events since there's no focus yet
>          , mIMEUpdatingContext(false)
>          , mIMESelectionChanged(false)
> +        , mIMETextChangedDuringFlush(false)

Can we use an enum for IME state keeping instead to avoid all the boolean parameters?

@@ +321,4 @@
>  
>      void SendIMEDummyKeyEvents();
>      void AddIMETextChange(const IMETextChange& aChange);
> +    void PostFlushIMEChanges(bool aRetry = false);

Passing an enum with, e.g., RETRY, NO_RETRY states, would be more descriptive than a boolean argument.

@@ +2078,5 @@
> +        int32_t start;
> +        int32_t oldEnd;
> +        int32_t newEnd;
> +    };
> +    nsAutoTArray<TextRecord, 4> mTextTransaction;

This is not a member variable (m-prefix).

@@ +2124,3 @@
>      mIMETextChanges.Clear();
>  
> +    for (uint32_t i = 0; i < mTextTransaction.Length(); i++) {

We could use iterators here instead.

@@ +2124,4 @@
>      mIMETextChanges.Clear();
>  
> +    for (uint32_t i = 0; i < mTextTransaction.Length(); i++) {
> +        TextRecord& record = mTextTransaction[i];

Can be const reference.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3d880efce8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a83a8594ddc
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f4c2d01bad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c89683497d23

Comment 48

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8d22eea111d9 for https://treeherder.mozilla.org/logviewer.html#?job_id=18431250&repo=mozilla-inbound

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/983e2c0c80a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/41cfa4292121
https://hg.mozilla.org/integration/mozilla-inbound/rev/362320a47dbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea9c07b65bd

Comment 50

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/983e2c0c80a8
https://hg.mozilla.org/mozilla-central/rev/41cfa4292121
https://hg.mozilla.org/mozilla-central/rev/362320a47dbb
https://hg.mozilla.org/mozilla-central/rev/eea9c07b65bd

Comment 52

[Sylvestre Ledru [:Sylvestre]]

[Tracking Requested - why for this release]:
Ritu, in Firefox 43, this is top crash  #5. See: https://crash-stats.mozilla.com/topcrashers/?product=FennecAndroid&version=43.0

Jim, are you planning to request the uplift to aurora and/or beta?
Thanks

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF44 as this has occurred ~400 times in last 28 days.

Comment 54

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Snorp, this is a top crash on Beta44. Would you be able to help nominate patches for uplift to m-b? Thanks!

Comment 55

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

The fix actually already made it into Aurora. For Beta, the fix doesn't really apply unless we uplift some other fixes. I think the risk for that is big enough that we should let this ride the trains.

Comment 56

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jim, Brad: The first crash signature associated with this top is currently ranked at #11 on 44.0b2 and it has jumped up 8 spots to that rank. This kind of data is not reassuring. Worse yet, on FF43, in the past 7 days, this crash has occurred 6000 times (!!!). I see this becoming a top crasher on FF44 once it's on the release channel. Can we find a way to uplift this to Beta44 unless we are absolutely certain that uplifting would hurt rather than help the current situation? Thanks!

Comment 57

[Brad Lassey [:blassey] (use needinfo?)]

Jim, can you list the bugs that would also need to be uplifted? Or is there a simpler "paper over" patch we could land on beta?

Comment 58

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

Attachment: Workaround patch for beta

This patch ignores the error instead of crashing. It will get rid of this crash and hopefully won't trigger other errors.

Comment 59

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

Comment on attachment 8703199 [details] [diff] [review]
Workaround patch for beta

Approval Request Comment

[Feature/regressing bug #]: N/A

[User impact if declined]: Top-crash for Beta

[Describe test coverage new/current, TreeHerder]: None. This is a simple band-aid patch for Beta only

[Risks and why]: Small; the patch itself is a one-line patch to ignore the error, although it may lead to other errors popping up.

[String/UUID change made/needed]: None

Comment 60

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8703199 [details] [diff] [review]
Workaround patch for beta

band-aid fix for a top crash, taking it.

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/14659c4b4fec

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/14659c4b4fec

Comment 63

[Flaviu Cos]

Verified as fixed in Firefox 44  Beta 8;
Device: Nexus 5 (Android 6.0.1).

Comment 64

[Flaviu Cos]

Verified as fixed in builds:
- 45 Beta 9;
- 46.0a2 2016-02-26;
Device: Nexus 5 (Android 6.0.1).

Comment 65

[Jack W]

I'm still experiencing this bug in the latest android Aurora build (47.0a2, 2016-04-02). adb logcat is showing this (not sure how to make it formatted as code):

04-02 20:03:01.601 19341 19341 I Keyboard: setKeyboardWidth newWidth=1080, mTotalWidth=1080
04-02 20:03:01.607 19521 20577 D GeckoEditable: reply: Action(3)
04-02 20:03:01.607 19521 20577 D GeckoEditable: reply: Action(3)
04-02 20:03:01.617 19521 20913 D GeckoEditable: offer: Action(6)
04-02 20:03:01.617 19521 20913 D GeckoEditable: icSendComposition(): no composition
04-02 20:03:01.621 19521 20577 D GeckoEditable: onSelectionChange(17319, 17319)
04-02 20:03:01.621 19521 20577 E GeckoEditable: invalid selection notification range: 17319 to 17319, length: 6
04-02 20:03:01.624 19521 20577 E GeckoCrashHandler: >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 4921 ("Gecko")
04-02 20:03:01.624 19521 20577 E GeckoCrashHandler: java.lang.IllegalArgumentException: invalid selection notification range
04-02 20:03:01.624 19521 20577 E GeckoCrashHandler:     at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:981)
04-02 20:03:01.624 19521 20577 E GeckoCrashHandler:     at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
04-02 20:03:01.624 19521 20577 E GeckoCrashHandler:     at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:443)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler: Main thread (1) stack:
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     android.os.MessageQueue.nativePollOnce(Native Method)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     android.os.MessageQueue.next(MessageQueue.java:323)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     android.os.Looper.loop(Looper.java:135)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     android.app.ActivityThread.main(ActivityThread.java:5417)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     java.lang.reflect.Method.invoke(Native Method)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
04-02 20:03:01.625 19521 20577 E GeckoCrashHandler:     com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
04-02 20:03:01.658 19521 20577 W google-breakpad: ExceptionHandler::GenerateDump cloned child
04-02 20:03:01.658 19521 20577 W google-breakpad: 20969(�@(�X(��(��H(��H(�
04-02 20:03:01.658 19521 20577 W google-breakpad:
04-02 20:03:01.658 19521 20577 W google-breakpad: ExceptionHandler::SendContinueSignalToChild sent continue signal to child
04-02 20:03:01.659 20969 20577 W google-breakpad: ExceptionHandler::WaitForContinueSignal waiting for continue signal...
04-02 20:03:03.399 20998 20998 D AndroidRuntime: >>>>>> START com.android.internal.os.RuntimeInit uid 10131 <<<<<<


To reproduce, go to https://linustechtips.com/main/contact/ and, in the editor, type @slick (or any username on the site), then tap one of the results that appears in the dropdown. It is converted to a non-content-editable span, then the browser crashes. The tagging feature that this uses is very temperamental though, so if the dropdown isn't appearing, try refreshing the browser or using a different keyboard (Google keyboard works sometimes).

Comment 66

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

(In reply to Jack W from comment #65)
> I'm still experiencing this bug in the latest android Aurora build (47.0a2,
> 2016-04-02). adb logcat is showing this (not sure how to make it formatted
> as code):

Thanks for the report! I believe this crash is fixed in the latest Nightly build. Can you check that? Although the tagging feature still doesn't work as intended, because you can't enter text after tagging someone.

Comment 67

[Jack W]

(In reply to Jim Chen [:jchen] [:darchons] from comment #66)
> (In reply to Jack W from comment #65)
> > I'm still experiencing this bug in the latest android Aurora build (47.0a2,
> > 2016-04-02). adb logcat is showing this (not sure how to make it formatted
> > as code):
> 
> Thanks for the report! I believe this crash is fixed in the latest Nightly
> build. Can you check that? Although the tagging feature still doesn't work
> as intended, because you can't enter text after tagging someone.

Yeah, it does seem to be fixed in the latest nightly. I was able to escape from the mention and carry on typing if there was more text on the line, but I'm not sure what's causing that.