1254629 - When using filter on GitHub, crash in java.lang.IllegalArgumentException: invalid selection notification range

Status: VERIFIED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #1248459 +++

(In reply to Christian Ascheberg from bug 1248459 comment #20)
> I can still reproduce a crash in today's Nightly that links to this bug:
> bp-1bfb2476-44bf-48c1-954f-ca4e22160306
> 
> STR:
> 1. open a Github project site, issues or pull requests tab:
> https://github.com/mozilla/MozStumbler/issues
> 2. scroll to the bottom and choose "Desktop version"
> 3. click "Author" filter
> 4. input character or text into "Filter users" input
> 5. tapping enter on the keyboard crashes Fennec
> 
> Nexus 4 running latest stock Android

Comment 1

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

Attachment: Make onKeyEvent more efficient (v1)

When handling synthetic keys, don't remove existing compositions and
don't check for certain early exit conditions, because they are not
applicable.

Comment 2

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

Attachment: Add "hide on input" test case (v1)

The bug happens when an input is hidden (e.g. through 'display' style)
inside an input or key event handler.

Comment 3

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

Attachment: Ensure content root is in document when flushing IME changes (v1)

One work-around for the bug is to ensure that our content root is in a
document when flushing IME changes, because when an input becomes
hidden, the previous content root is removed from the document.

Comment 4

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

Attachment: Add test for letting query events fail for wrong content root (v1)

Add a test for the bug fixed by the next patch. When an input is hidden
(through display = none) during an input or key event, IME query events
after that can return invalid data.

Comment 5

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

Attachment: Let query events fail when content root is wrong (v1)

Make query events fail (including when caching selection) if the queried
content root is different from what we expected.

Comment 6

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

Comment on attachment 8728253 [details] [diff] [review]
Let query events fail when content root is wrong (v1)

I worry about this change on the other platforms but let's check regression reports.

And could you add comments before checking |mReply.mContentsRoot != mRootContent|, why the check is necessary.

Comment 7

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

Comment on attachment 8728251 [details] [diff] [review]
Add test for letting query events fail for wrong content root (v1)

>+  var input = document.getElementById("input");
>+
>+  input.addEventListener("input", function() {
>+    this.value = "";
>+    this.style.display = "none";
>+  });
>+
>+  input.focus();
>+  input.select();
>+
>+  var result = synthesizeQuerySelectedText();
>+  ok(result.succeeded, "Query selected text should succeed");
>+  is(result.offset, 0, "Selected text should be at offset 0");
>+  is(result.text, "foo", "Selected text should match");
>+
>+  synthesizeComposition({ type: "compositioncommit", data: "bar" });
>+
>+  result = synthesizeQuerySelectedText();
>+  ok(!result.succeeded, "Query hidden selection should fail");

Hmm, in this case, looks like that the query event should be succeeded with the whole contents of the document.

If you call SpecialPowers.getDOMWindowUtils(window).updateLayerTree() immediately after the synthesizeComposition() call, isn't it succeeded?

If so, this test depends on current IMEContentObserver's life time and doesn't test what you are fixing with the patch. In this case, don't you want to check if IMEContentObserver notifies selection change with wrong content root? If so, I think that you should test it actually.

syntehsizeComposition() takes callback function for listening notifications from IMEContentObserver. Unfortunately, nsITextInputProcessorNotification doesn't support "notify-selection-change" yet but you can support it easy. Currently, all widgets request NOTIFY_IME_OF_SELECTION_CHANGE (http://mxr.mozilla.org/mozilla-central/search?string=NOTIFY_SELECTION_CHANGE&filter=[Nn]OTIFY_SELECTION_CHANGE). Therefore, you can remove NOTIFY_SELECTION_CHANGE and WantSelectionChange() from IMEData.h and add a case for NOTIFY_IME_OF_SELECTION_CHANGE here (http://mxr.mozilla.org/mozilla-central/source/dom/base/TextInputProcessor.cpp?rev=03fc467e1002#643). Then, you can listen NOTIFY_IME_OF_SELECTION_CHANGE in the test.

If you think that this is really out of scope of this bug, please file a follow up bug and let's take this test for now. I'll try to fix it later.

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8728248 [details] [diff] [review]
Make onKeyEvent more efficient (v1)

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

::: widget/android/nsWindow.cpp
@@ +2494,5 @@
>          // array until the next IME_REPLACE_TEXT event, at which point
>          // these keys are dispatched in sequence.
>          mIMEKeyEvents.AppendElement(
>                  mozilla::UniquePtr<WidgetEvent>(event.Duplicate()));
> +

whitespace

Comment 9

[Eugen Sawin [:esawin]]

Comment on attachment 8728249 [details] [diff] [review]
Add "hide on input" test case (v1)

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

::: mobile/android/tests/browser/robocop/robocop_input.html
@@ +19,5 @@
> +      });
> +
> +      // An input that hides on input.
> +      let hiding_input = document.getElementById("hiding-input");
> +      hiding_input.addEventListener('input', function (e) {

Please be consistent with function definition styles (function(e) vs. function (e)), I see both styles used in this file.

@@ +20,5 @@
> +
> +      // An input that hides on input.
> +      let hiding_input = document.getElementById("hiding-input");
> +      hiding_input.addEventListener('input', function (e) {
> +        if (this.value === "foo") {

This test file needs a bit of documentation on where this input comes from (make the connection to testInputConnection.java) and what the input sequence is.

Comment 10

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

Comment on attachment 8728251 [details] [diff] [review]
Add test for letting query events fail for wrong content root (v1)

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> Comment on attachment 8728251 [details] [diff] [review]
> Add test for letting query events fail for wrong content root (v1)
> 
> If you think that this is really out of scope of this bug, please file a
> follow up bug and let's take this test for now. I'll try to fix it later.

Ok, I will leave the test alone for now.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0c3fcc410b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a21940e73836
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef38f6f97d2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd7564bd9998

Comment 12

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

I had to back out fd7564bd9998 because test_windowless_ime.html was failing on Windows:
https://treeherder.mozilla.org/logviewer.html#?job_id=23580996&repo=mozilla-inbound


https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa9f00493a9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b0c3fcc410b
https://hg.mozilla.org/mozilla-central/rev/a21940e73836
https://hg.mozilla.org/mozilla-central/rev/ef38f6f97d2f

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0359d3b3dc55

Comment 15

[Carsten Book [:Tomcat]]

backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9de09b9ee931

since one of this changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=23881111&repo=mozilla-inbound

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf76c73b91dd

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf76c73b91dd

Comment 18

[Christian Ascheberg]

STR are working fine now

Comment 19

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

Comment on attachment 8728250 [details] [diff] [review]
Ensure content root is in document when flushing IME changes (v1)

Uplift request for *this patch only*. I think it's the safest minimal set of patches to uplift that fixes the bug.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Possible crash when entering text on sites like GitHub
[Describe test coverage new/current, TreeHerder]: Locally, m-c
[Risks and why]: Very small; patch is Android-only and avoids crash
[String/UUID change made/needed]: None

Comment 20

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

this is a topcrash on release for android, marking affected and tracking for other affected branches.

Comment 21

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

Comment on attachment 8728250 [details] [diff] [review]
Ensure content root is in document when flushing IME changes (v1)

Fix for android topcrash, verified on m-c. 
This should make it into the beta 4 build.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2692deefd17f

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f67185763b0

Comment 24

[Jorg K (CEST = GMT+2)]

I'm seeing
[2784, Main Thread] WARNING: 'aEvent->mReply.mContentsRoot != mRootContent', file c:/mozilla-source/comm-central/mozilla/dom/events/IMEContentObserver.cpp, line 839
quite a bit in Thunderbird now.

Comment 25

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

Do you have steps to reproduce? Please file a new bug if that's causing any issue.