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

VERIFIED FIXED in Firefox 46

Status

()

Firefox for Android
Keyboards and IME
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

({crash, topcrash})

unspecified
Firefox 48
All
Android
crash, topcrash
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46+ fixed, firefox47+ fixed, firefox48+ fixed)

Details

(crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

+++ 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
Created attachment 8728248 [details] [diff] [review]
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.
Attachment #8728248 - Flags: review?(esawin)
Created attachment 8728249 [details] [diff] [review]
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.
Attachment #8728249 - Flags: review?(esawin)
Created attachment 8728250 [details] [diff] [review]
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.
Attachment #8728250 - Flags: review?(esawin)
Created attachment 8728251 [details] [diff] [review]
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.
Attachment #8728251 - Flags: review?(masayuki)
Created attachment 8728253 [details] [diff] [review]
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.
Attachment #8728253 - Flags: review?(masayuki)
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.
Attachment #8728253 - Flags: review?(masayuki) → review+
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 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
Attachment #8728248 - Flags: review?(esawin) → review+
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.
Attachment #8728249 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8728250 - Flags: review?(esawin) → review+
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.
Attachment #8728251 - Flags: review?(masayuki)

Comment 11

2 years ago
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
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
Flags: needinfo?(nchen)
Flags: needinfo?(nchen)
Keywords: leave-open

Comment 13

2 years ago
bugherder
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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0359d3b3dc55
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
Flags: needinfo?(nchen)

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf76c73b91dd
Flags: needinfo?(nchen)
Keywords: leave-open

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf76c73b91dd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Comment 18

2 years ago
STR are working fine now
Status: RESOLVED → VERIFIED
Attachment #8728251 - Attachment is obsolete: true
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
Attachment #8728250 - Flags: approval-mozilla-beta?
Attachment #8728250 - Flags: approval-mozilla-aurora?
this is a topcrash on release for android, marking affected and tracking for other affected branches.
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
tracking-firefox48: --- → +
Keywords: topcrash
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.
Attachment #8728250 - Flags: approval-mozilla-beta?
Attachment #8728250 - Flags: approval-mozilla-beta+
Attachment #8728250 - Flags: approval-mozilla-aurora?
Attachment #8728250 - Flags: approval-mozilla-aurora+

Comment 22

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2692deefd17f
status-firefox47: affected → fixed

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2f67185763b0
status-firefox46: affected → fixed
You need to log in before you can comment on or make changes to this bug.