Closed
Bug 1254629
Opened 8 years ago
Closed 8 years ago
When using filter on GitHub, crash in java.lang.IllegalArgumentException: invalid selection notification range
Categories
(Firefox for Android Graveyard :: Keyboards and IME, defect)
Tracking
(firefox45 affected, firefox46+ fixed, firefox47+ fixed, firefox48+ fixed)
VERIFIED
FIXED
Firefox 48
People
(Reporter: jchen, Assigned: jchen)
Details
(Keywords: crash, topcrash)
Crash Data
Attachments
(4 files, 1 obsolete file)
2.68 KB,
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
5.34 KB,
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
esawin
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.20 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
+++ 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
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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 7•8 years ago
|
||
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•8 years ago
|
||
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 9•8 years ago
|
||
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•8 years ago
|
Attachment #8728250 -
Flags: review?(esawin) → review+
Assignee | ||
Comment 10•8 years ago
|
||
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•8 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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(nchen)
Keywords: leave-open
Comment 13•8 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 15•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(nchen)
Keywords: leave-open
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cf76c73b91dd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Updated•8 years ago
|
Attachment #8728251 -
Attachment is obsolete: true
Assignee | ||
Comment 19•8 years ago
|
||
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?
Comment 20•8 years ago
|
||
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 21•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/2692deefd17f
Comment 23•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2f67185763b0
Comment 24•6 years ago
|
||
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)
Assignee | ||
Comment 25•6 years ago
|
||
Do you have steps to reproduce? Please file a new bug if that's causing any issue.
Flags: needinfo?(nchen)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•