Closed
Bug 1351170
Opened 7 years ago
Closed 7 years ago
Crash in java.lang.IllegalArgumentException: invalid selection notification range at org.mozilla.gecko.GeckoEditableChild.onSelectionChange(GeckoEditableChild.java)
Categories
(Firefox for Android Graveyard :: Keyboards and IME, defect)
Tracking
(fennec+, firefox52 wontfix, firefox53 wontfix, firefox54+ wontfix, firefox55 verified, firefox56 verified)
VERIFIED
FIXED
Firefox 56
People
(Reporter: ting, Assigned: jchen)
References
Details
(Keywords: crash)
Crash Data
Attachments
(5 files)
4.72 MB,
image/gif
|
Details | |
59 bytes,
text/x-review-board-request
|
masayuki
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
esawin
:
review+
|
Details |
1.28 KB,
patch
|
jchen
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-b336fd3a-c6bb-4a54-a43f-28f7e2170327. ============================================================= Top #2 of Aurora 20170326084757 on Android, 8 crashes from 5 installations. There are 17 reports in the past week.
Comment 1•7 years ago
|
||
I think I have exact STR for this: 1. Login into www.telegram.org in fennec 2. Send a message using the telegram webapp's emoji keyboard (this is different from the android OS emoji keyboard) 3. Crash will occur shortly thereafter. Note, sending a message using the android OS emoji keyboard works fine.
Comment 2•7 years ago
|
||
This reproduces in FF52 release as well.
Comment 3•7 years ago
|
||
Too late for firefox 52, mass-wontfix.
Comment 4•7 years ago
|
||
Got the issue in nightly 55. Not sure about the str but it was in telegram as well.
Comment 5•7 years ago
|
||
James, is there someone who can look at this? I have 100% STR in comment 1.
Flags: needinfo?(snorp)
Yup, Jim can look.
Assignee: nobody → nchen
Flags: needinfo?(snorp) → needinfo?(nchen)
Comment 7•7 years ago
|
||
Hi :snorp, Jim is on PTO, can you help find someone else to look at this issue? There are lots of crash in 54 now.
tracking-firefox54:
--- → +
Flags: needinfo?(snorp)
Comment 8•7 years ago
|
||
Hi, I found another scenario where this happens. Environment: Samsung Galaxy S7 Edge (Android 7.0) - 1440 x 2560 pixels (~534 ppi pixel density) Firefox Mobile Nightly 56.0a1 (2017-06-25) (Mobile) Prerequisites: Create an account on www.tutorialspoint.com. Steps to reproduce: 1. Login. 2. Navigate to https://www.tutorialspoint.com/tutor_connect/online_user_profile.php#UserBasic 3. Scroll down to the Education and Experience text area. 4. Tap the Indent button. 5. Type in some text. 6. Tap Enter. 7. Tap Backspace. Expected results: The text is deleted. Actual results: Nightly crashes. Notes: 1. The issue is also reproducible on Firefox 54.0 Release (Mobile). 2. Screen capture attached.
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
Jim is on the case.
Flags: needinfo?(snorp)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8885895 [details] Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; https://reviewboard.mozilla.org/r/156668/#review161910 ::: dom/events/ContentEventHandler.cpp:2847 (Diff revision 1) > + // For example, for this content: <br>abc, and range (<br>, 0)-("abc", 1), the > + // range includes the linebreak from <br>, so the start offset should _not_ > + // include <br>, and the start offset should be 0. > + // > + // However, for this content: <p>"abc"</p>, and range (<p>, 0)-("abc", 1), the > + // range does _not_ include the linebreak from <p>, so the start offset > + // _should_ include <p>, and the start offset should be 1. How about "<p></p><p>abc</p>" and range (<p>, 0)-("abc", 1) case? I don't think that it should include the break by <p>. I think that you should use IsContentBR() because this is a special case only for <br>. ::: dom/events/ContentEventHandler.cpp:2855 (Diff revision 1) > + const NodePosition& startPos = !aRange->GetStartContainer()->GetChildCount() > + ? NodePositionBefore(aRange->GetStartContainer(), aRange->StartOffset()) > + : NodePosition(aRange->GetStartContainer(), aRange->StartOffset()); Feels odd indent. Could you use: const NodePosition& startPos = !aRange->GetStartContainer()->GetChildCount() ? NodePositionBefore(aRange->GetStartContainer(), aRange->StartOffset()) : NodePosition(aRange->GetStartContainer(), aRange->StartOffset()); ? And in this case, we should use IsContentBR() though, if you need to check if the node has children, you should use nsINode::HasChildren() instead of nsINode::GetChildCount(). ::: widget/tests/window_composition_text_querycontent.xul:4289 (Diff revision 1) > + // #17 > + contenteditable.innerHTML = "<br/>a"; > + selection.setBaseAndExtent(contenteditable.firstChild, 0, contenteditable.lastChild, 1); > + checkSelection(0, kLF + "a", "runQueryTextContentEventTest #17, \"" + contenteditable.innerHTML + "\""); I don't understand why do you add this test here. How about to create runQuerySelectionEventTest? (FYI: The numbers are synced with the numbers in runSetSelectionEventTest). And perhaps, you should test the case "<p>[</p><p>a]bc</p>" and "<p>[abc</p><p>d]ef</p>".
Attachment #8885895 -
Flags: review?(masayuki) → review-
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8885896 [details] Bug 1351170 - 2. Notify selection listeners after adjusting range offsets; https://reviewboard.mozilla.org/r/156670/#review161994 ::: dom/base/nsRange.cpp:653 (Diff revision 1) > nsINode* container = NODE_FROM(aContainer, aDocument); > > // Adjust position if a sibling was inserted. > if (container == mStartContainer && aIndexInContainer < mStartOffset && > !mStartOffsetWasIncremented) { > ++mStartOffset; Why are we then changing the offset here and not by calling DoSetRange? Same with end offset. If we try to make this all work more consistently, would be nice if DoSetRange indeed would be the only thing to set start/end boundary points ::: dom/base/nsRange.cpp:705 (Diff revision 1) > > // Adjust position if a sibling was removed... > if (container == mStartContainer) { > if (aIndexInContainer < mStartOffset) { > --mStartOffset; > + rangeChanged = true; Same here
Attachment #8885896 -
Flags: review?(bugs) → review-
tracking-fennec: ? → +
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8885895 [details] Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; https://reviewboard.mozilla.org/r/156668/#review161910 > How about "<p></p><p>abc</p>" and range (<p>, 0)-("abc", 1) case? I don't think that it should include the break by <p>. > > I think that you should use IsContentBR() because this is a special case only for <br>. The bug also affects tags like `<img>`, so I don't think `IsContentBR()` is enough. For `<p></p><p>abc</p>` and range `(<p>, 0)-("abc", 1)`, the current result is offset=1 text="\n\na", so the offset doesn't match the text; with the patch, the result becomes offset=0 and text="\n\na", so at least the offset matches the text. > Feels odd indent. Could you use: > > const NodePosition& startPos = > !aRange->GetStartContainer()->GetChildCount() > ? NodePositionBefore(aRange->GetStartContainer(), aRange->StartOffset()) > : NodePosition(aRange->GetStartContainer(), aRange->StartOffset()); > > ? > > And in this case, we should use IsContentBR() though, if you need to check if the node has children, you should use nsINode::HasChildren() instead of nsINode::GetChildCount(). Fixed indent and `HasChildren()`. > I don't understand why do you add this test here. > > How about to create runQuerySelectionEventTest? > > (FYI: The numbers are synced with the numbers in runSetSelectionEventTest). > > And perhaps, you should test the case "<p>[</p><p>a]bc</p>" and "<p>[abc</p><p>d]ef</p>". Added `runQuerySelectionEventTest` and the two testcases.
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8885896 [details] Bug 1351170 - 2. Notify selection listeners after adjusting range offsets; https://reviewboard.mozilla.org/r/156670/#review161994 > Why are we then changing the offset here and not by calling DoSetRange? Same with end offset. > If we try to make this all work more consistently, would be nice if DoSetRange indeed would be the only thing to set start/end boundary points Changed to go through `DoSetRange` for all cases.
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8886298 [details] Bug 1351170 - 3. Do more to synchronize spans to shadow text; https://reviewboard.mozilla.org/r/157046/#review162178
Attachment #8886298 -
Flags: review?(esawin) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8885896 [details] Bug 1351170 - 2. Notify selection listeners after adjusting range offsets; https://reviewboard.mozilla.org/r/156670/#review162198
Attachment #8885896 -
Flags: review?(bugs) → review+
Comment 21•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8885895 [details] Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; https://reviewboard.mozilla.org/r/156668/#review161910 > The bug also affects tags like `<img>`, so I don't think `IsContentBR()` is enough. For `<p></p><p>abc</p>` and range `(<p>, 0)-("abc", 1)`, the current result is offset=1 text="\n\na", so the offset doesn't match the text; with the patch, the result becomes offset=0 and text="\n\na", so at least the offset matches the text. No, it shouldn't behave so because IME may try to set caret between the two \n. Then, returning 0 when it checks selection range is bad. I found nsHTMLElement::IsContainer(), cannot use this? (I think ContentEventHandler should have a method which wraps its call since it needs to get tag ID.)
Assignee | ||
Comment 22•7 years ago
|
||
Ok, I think we should also fix the hack in nsContentIterator [1]? The current behavior returns incorrect results (offset=1 text="\n\na" in the above example). [1] http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/dom/base/nsContentIterator.cpp#373
Comment 23•7 years ago
|
||
If removing the hack doesn't cause new orange, I guess it's reasonable, pretty risky though. # I think that such low level class shouldn't have any hack unless specifying to do hacks explicitly because users who depend on such hack will cause the source code to be made spaghetti.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8885895 [details] Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; https://reviewboard.mozilla.org/r/156668/#review161910 > No, it shouldn't behave so because IME may try to set caret between the two \n. Then, returning 0 when it checks selection range is bad. I found nsHTMLElement::IsContainer(), cannot use this? (I think ContentEventHandler should have a method which wraps its call since it needs to get tag ID.) The new patch uses `IsContainer()` for both `ContentEventHandler` and `nsContentIterator` so we get consistent behavior.
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8885895 [details] Bug 1351170 - 1. Correctly calculate start offset for non-text nodes; https://reviewboard.mozilla.org/r/156668/#review163406 ::: widget/tests/window_composition_text_querycontent.xul:4304 (Diff revision 3) > + > + // #2 > + contenteditable.innerHTML = "<p></p><p>abc</p>"; > + selection.setBaseAndExtent(contenteditable.firstChild, 0, contenteditable.lastChild.firstChild, 1); > + // XXX special behavior for childless non-text nodes. > + checkSelection(1, kLF + "a", "runQuerySelectionEventTest #2, \"" + contenteditable.innerHTML + "\""); I guess, you have not run this test on Windows. The first argument should be kLFLen rather than 1. ::: widget/tests/window_composition_text_querycontent.xul:4309 (Diff revision 3) > + checkSelection(1, kLF + "a", "runQuerySelectionEventTest #2, \"" + contenteditable.innerHTML + "\""); > + > + // #3 > + contenteditable.innerHTML = "<p>abc</p><p>def</p>"; > + selection.setBaseAndExtent(contenteditable.firstChild, 0, contenteditable.lastChild.firstChild, 1); > + checkSelection(1, "abc" + kLF + "d", "runQuerySelectionEventTest #3, \"" + contenteditable.innerHTML + "\""); Same here.
Attachment #8885895 -
Flags: review?(masayuki) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
Pushed by nchen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7773fb9e0bf3 1. Correctly calculate start offset for non-text nodes; r=masayuki https://hg.mozilla.org/integration/autoland/rev/b7909eddfe26 2. Notify selection listeners after adjusting range offsets; r=smaug https://hg.mozilla.org/integration/autoland/rev/9fc3f64f2583 3. Do more to synchronize spans to shadow text; r=esawin
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7773fb9e0bf3 https://hg.mozilla.org/mozilla-central/rev/b7909eddfe26 https://hg.mozilla.org/mozilla-central/rev/9fc3f64f2583
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 34•7 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: needinfo?(nchen)
Assignee | ||
Comment 35•7 years ago
|
||
Hm the patches are pretty risky as they affect all platforms (despite this crash being Android-only), so we may not want to uplift the patches directly. I can see if I can prepare a workaround patch just for Beta that's less risky.
Comment 36•7 years ago
|
||
OK, there's only a week left in the cycle and we've only got 2 Android betas left, so anything you can do soon would be great :)
Assignee | ||
Comment 37•7 years ago
|
||
Don't crash when we encounter a selection exception when in Beta. Still crash when in Nightly so we can investigate the source of the crash. r=me for trivial patch.
Attachment #8889668 -
Flags: review+
Assignee | ||
Comment 38•7 years ago
|
||
Comment on attachment 8889668 [details] [diff] [review] Patch for Beta (v1) This at least makes us not crash in the Telegram testcase. Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: Possible crash when using certain sites like Telegram. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Simple crash prevention fix; no change in functionality. [String changes made/needed]: None
Flags: needinfo?(nchen)
Attachment #8889668 -
Flags: approval-mozilla-beta?
Comment 39•7 years ago
|
||
Pushed by nchen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d134c8d7cc6 Don't crash on selection exception in Beta; r=me
Comment 40•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d134c8d7cc6
Comment 41•7 years ago
|
||
Comment on attachment 8889668 [details] [diff] [review] Patch for Beta (v1) avoid a crash in fennec, beta55+ afaict this is #10 top crash in 54, seems worth the workaround
Attachment #8889668 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 42•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3b36f2863e5c
Comment 43•7 years ago
|
||
Device: - Galaxy S7 Edge (Android 7.0); Builds: - Beta - 55.0b14; - Nightly - 56.0a1 (2017-07-31); Hello, I've verified this following the instructions provided in Comment 1 and Comment 8 and the issue is no longer reproducible.
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
•