Closed Bug 1254629 Opened 4 years ago Closed 4 years ago

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

Categories

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

All
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox45 --- affected
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed

People

(Reporter: jchen, Assigned: jchen)

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(4 files, 1 obsolete file)

+++ 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
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)
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)
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)
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)
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+
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)
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
Flags: needinfo?(nchen)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/cf76c73b91dd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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.
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+
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.
Flags: needinfo?(nchen)
Do you have steps to reproduce? Please file a new bug if that's causing any issue.
Flags: needinfo?(nchen)
You need to log in before you can comment on or make changes to this bug.