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)

ARM
Android
defect
Not set
critical

Tracking

(fennec+, firefox52 wontfix, firefox53 wontfix, firefox54+ wontfix, firefox55 verified, firefox56 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
fennec + ---
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 + wontfix
firefox55 --- verified
firefox56 --- verified

People

(Reporter: ting, Assigned: jchen)

References

Details

(Keywords: crash)

Crash Data

Attachments

(5 files)

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.
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.
This reproduces in FF52 release as well.
Too late for firefox 52, mass-wontfix.
Got the issue in nightly 55. Not sure about the str but it was in telegram as well.
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)
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.
Flags: needinfo?(snorp)
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.
tracking-fennec: --- → ?
Hardware: Unspecified → ARM
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
Jim is on the case.
Flags: needinfo?(snorp)
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 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-
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.
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 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 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 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.)
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
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 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 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+
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
Please request Beta approval on this when you get a chance.
Flags: needinfo?(nchen)
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.
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 :)
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+
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?
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 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+
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.
Status: RESOLVED → VERIFIED
Depends on: 1383747
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.