1217669 - Android widget might have a bug to merge multiple text changes

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Widget: Android, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: nsWindow for Android should use TextChangeDataBase::MergeWith() for merging multiple text changes

Now, nsWindow for Android stores multiple text changes with computing the range's offsets. However, if I use IMENotifiction::TextChangeData::MergeWith(), I see different result.

For example, with current code, I hit this assertion with my patches for bug 1213589:

>  05:57:44     INFO -  47 INFO TEST-START | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html
>  05:58:39     INFO -  INFO | automation.py | Application ran for: 0:08:58.526686
>  05:58:39     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpQnyz5opidlog
>  05:58:40     INFO -  /data/tombstones does not exist; tombstone check skipped
>  05:58:40  WARNING -  PROCESS-CRASH | java-exception | java.lang.IllegalArgumentException: invalid selection notification range at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:910)

>  06:43:47     INFO -  231 INFO TEST-START | editor/libeditor/tests/test_bug795785.html
>  06:44:09     INFO -  INFO | automation.py | Application ran for: 0:21:38.180603
>  06:44:09     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpv51U3gpidlog
>  06:44:10     INFO -  /data/tombstones does not exist; tombstone check skipped
>  06:44:11  WARNING -  PROCESS-CRASH | java-exception | java.lang.IllegalArgumentException: invalid selection notification range at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:910)

However, with the attached patch, I don't see these assertions. However, I hit this assertion:

>  11:12:37     INFO -  198 INFO TEST-START | editor/libeditor/tests/test_bug795785.html
>  11:13:22     INFO -  INFO | automation.py | Application ran for: 0:14:18.648331
>  11:13:22     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp2wpwWIpidlog
>  11:13:22     INFO -  /data/tombstones does not exist; tombstone check skipped
>  11:13:23  WARNING -  PROCESS-CRASH | java-exception | java.lang.IndexOutOfBoundsException: replace (5 ... 0) has end before start at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:1009)


So, I guess that the computing range offsets in nsWindow for Android might be buggy. And TextChangeData::MergeWith() might be so too.


Jim, do we have some tests about nsWindow::AddIMETextChange()? TextChangeData::MergeWith() has a lot of tests in:
http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#2178
Therefore, I trust TextChangeData::MergeWith() result better.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54fcd1f6d7d6

Comment 2

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0)
> 
> However, with the attached patch, I don't see these assertions. However, I
> hit this assertion:
> 
> >  11:12:37     INFO -  198 INFO TEST-START | editor/libeditor/tests/test_bug795785.html
> >  11:13:22     INFO -  INFO | automation.py | Application ran for: 0:14:18.648331
> >  11:13:22     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp2wpwWIpidlog
> >  11:13:22     INFO -  /data/tombstones does not exist; tombstone check skipped
> >  11:13:23  WARNING -  PROCESS-CRASH | java-exception | java.lang.IndexOutOfBoundsException: replace (5 ... 0) has end before start at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:1009)

This assertion happens before the other assertion in the same code path, so I think it's caused by the same problem.

> Jim, do we have some tests about nsWindow::AddIMETextChange()?
> TextChangeData::MergeWith() has a lot of tests in:
> http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#2178
> Therefore, I trust TextChangeData::MergeWith() result better.

We have a test [1] that tests the entire IME system on Android, but it's not very extensive. However, in actual usage, nsWindow::AddIMETextChange has been reliable for a long time, so I still suspect the root cause is that content is sending widget invalid offsets through NOTIFY_IME_OF_TEXT_CHANGE.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testInputConnection.java

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I still suspect the root cause is that content is sending widget invalid offsets through
> NOTIFY_IME_OF_TEXT_CHANGE.

Of course, it's possible. However, NOTIFY_IME_OF_SELECTION_CHANGE also uses same methods in ContentEventHandler... I'd like to create texts of NOTIFY_IME_OF_SELECTION_CHANGE and NOTIFY_IME_OF_TEXT_CHANGE, but I don't have much time due to TPAC, sigh...

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: With the patch, we don't hit the new assertion at running test_browserElement_inproc_CopyPaste.html.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And additional odd case:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=551bf2fb1bd9

I added assertions to start < mOldEnd and start < mNewEnd in TextChangeData::MergeWith() but without hitting the assertions, still we hit the |java.lang.IndexOutOfBoundsException: replace (5 ... 0) has end before start at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:1009)|. So, anyway, looks like Android specific code has some bugs.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ba88ffadbd9

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ba88ffadbd9

This try build only tested the patch. So, the new assertion hit may detect a regression of my patch. (but I'm still confused that these tests are not crashed in TextChangeDataase::MergeWith().) However, avoiding the old assertion hits may indicate AddIMETextChange() having a bug.

How do you think that attached patch should be landed?
# AddIMETextChange() looks simpler than TextChangeDataBase::MergeWith(). Therefore, I guess that it doesn't have code for some edge cases.

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

this is totally out of my knowledge so you might have to find the right person to ping again. Thanks

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Joe Cheng [:jcheng] from comment #8)
> this is totally out of my knowledge so you might have to find the right
> person to ping again. Thanks

Wow, I'm sorry.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jim:

Could you check the comment 7?

Comment 11

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

Attachment: Bug 1213859 part 5

I think the patches in bug 1213859 have some edge case bugs. With this patch applied on top of the other patches, I can get the Android tests to pass [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=52772a3c0caa

One bug was that when a block element is added, the text change offsets don't include the extra newline at the beginning. For example, when adding a <div/> to the editor, the text change reports 0 added length but it should be 1 because the <div/> now has a newline in front.

Many places in ContentEventHandler assume positions inside and outside an opening HTML tag represent the same offset. For example, the code assumed that the following two positions have the same offset,

>  <div>|<p>abc</p></div>
>  <div><p>|abc</p></div>

This was true because there was no newline before <p>, but now there is an implicit newline before <p>, the two positions no longer have the same offset, so we have to fix the code to not assume that. This patch adds a aRootOffset parameter to GetFlatTextOffsetOfRange, so we can distinguish between positions that are inside or outside of the opening tag.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #11)
> Created attachment 8678427 [details] [diff] [review]
> Bug 1213859 part 5
> 
> I think the patches in bug 1213859 have some edge case bugs. With this patch
> applied on top of the other patches, I can get the Android tests to pass [1].
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=52772a3c0caa

Wow, thank you for your investigation.

I found a bug of GetNativeTextLength(), but still have some problems. The patch really helps me!

> One bug was that when a block element is added, the text change offsets
> don't include the extra newline at the beginning. For example, when adding a
> <div/> to the editor, the text change reports 0 added length but it should
> be 1 because the <div/> now has a newline in front.

Right.

> Many places in ContentEventHandler assume positions inside and outside an
> opening HTML tag represent the same offset. For example, the code assumed
> that the following two positions have the same offset,
> 
> >  <div>|<p>abc</p></div>
> >  <div><p>|abc</p></div>
> 
> This was true because there was no newline before <p>, but now there is an
> implicit newline before <p>, the two positions no longer have the same
> offset, so we have to fix the code to not assume that. This patch adds a
> aRootOffset parameter to GetFlatTextOffsetOfRange, so we can distinguish
> between positions that are inside or outside of the opening tag.

Yeah, I'm now straggling with this issue!

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I'm still not sure if there is actually a bug, but at least we can fix bug 1213589 without any changes in widget for Android. Let's close this bug for now.