Closed Bug 1319660 Opened 3 years ago Closed 3 years ago

textbox typing/deleting crashes fennec

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
fennec 52+ ---
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: bkelly, Assigned: jchen)

References

()

Details

(Keywords: crash)

Attachments

(1 file, 6 obsolete files)

I saw someone on twitter complaining http://telegra.ph was crashing on firefox for android.  I tried the site and, sure enough, it crashes.  All I did was start typing in the title space.

It crashed twice, but now won't crash for me.

Unfortunately my android aurora profile does not show me anything in about:crashes.  James, does this mean the crash happened in the java layer?
Flags: needinfo?(snorp)
I can reproduce this and there's not much in the log:

> GeckoEventDispatcher  W  No listeners for FormAssist:Hide in dispatchEvent
>           GeckoEditable  E  invalid selection notification range: 1 to 1, length: 0
>              System.err  W  java.lang.IllegalArgumentException: invalid selection notification range
>                          W      at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:1125)
>                          W      at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
>                          W      at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:513)
>         google-breakpad  W  ExceptionHandler::GenerateDump cloned child
>                          W  27637
>                          W  ExceptionHandler::SendContinueSignalToChild sent continue signal to child
tracking-fennec: --- → ?
So far I only see this in Aurora to Release and do not see that happening in Nightly.
No, Java crashes should be reported in about:crashes. I suspect about:crashes may have been broken somehow, as it wasn't showing anything in Nightly for me (but does appear to work now).
Flags: needinfo?(snorp)
Assignee: nobody → walkingice0204
tracking-fennec: ? → 51+
I can trigger this reliably on fennec aurora 52 on twitter:

1. Log in to twitter on fennec aurora
2. Have someone or another account send you a DM
3. Open twitter DM screen on fennec aurora
4. Try to start a message by typing "Still"
5. On my device I see a textbox with "StillS" for some reason
6. Try to delete that trailing bogus "S"
7. Crash

This 100% reproduces for me.

This is what ADB shows me:

2-06 11:10:55.286 31470 31487 E GeckoEditable: invalid selection notification range: 5 to 5, length
: 0
12-06 11:10:55.291 31470 31487 W System.err: java.lang.IllegalArgumentException: invalid selection n
otification range
12-06 11:10:55.291 31470 31487 W System.err:    at org.mozilla.gecko.GeckoEditable.onSelectionChange
(GeckoEditable.java:1125)
12-06 11:10:55.291 31470 31487 W System.err:    at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
12-06 11:10:55.291 31470 31487 W System.err:    at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:513)
12-06 11:10:55.320 31470 31487 W google-breakpad: ExceptionHandler::GenerateDump cloned child
12-06 11:10:55.320 31470 31487 W google-breakpad: 31770
12-06 11:10:55.320 31470 31487 W google-breakpad:
12-06 11:10:55.320 31770 31487 W google-breakpad: ExceptionHandler::WaitForContinueSignal waiting for continue signal...
12-06 11:10:55.320 31470 31487 W google-breakpad: ExceptionHandler::SendContinueSignalToChild sent continue signal to child
12-06 11:10:57.363 31771 31771 D AndroidRuntime: >>>>>> START com.android.internal.os.RuntimeInit uid 10110 <<<<<<
This seems related to our handling of typing, number of characters displayed, and trying to delete phantom characters.  Thats my theory anyway.

Adjusting the summary to indicate this is not specific to that one site.
Status: NEW → ASSIGNED
Summary: telegra.ph site crashes on fennec → textbox typing/deleting crashes fennec
I cannot reproduce in twitter way, but in another way

# In Beta 51.0

1. go to telegra.ph, in "Title" field type in "a", then type in "b"
2. But the field will response as "ba", and cursor places between b and a
3. Press Return key to give a line break, now it crash

# In Aurora 52.0a2

Cannot reproduce in this way

# In mozilla-central (hg revision 7652a58efa46f1c57c94bba26efc5d53b6184e83)

a little different way to reproduce

1. go to telegra.ph, in "Title" field type in "abc", press Return
2. the cursor is in field "Your name", now type in "def"
3. the cursor jumps back to the head of "Title" field, and "def" is inserted in front of "abc"
4. now the cursor places between "def" and "abc", press Return key will crash Fennec.
When to open telegra.ph page, inside GeckoEditable.java the mText will be initialized[1] with value "\n\n\n\n\n", and once I entered "abc", the mCurrentText becomes "\nabc\n\n\n\n\n". I will keep digging problem in this direction.

[1] https://dxr.mozilla.org/mozilla-central/rev/6dbc6e9f62a705d5f523cc750811bd01c8275ec6/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java#194
it seems in callback onTextChange[1] got a large number for `unboundedOldEnd`, then set `oldEnd` to `currentLength`. Then it replace whole `mText.mCurrentText` by empty string. So next time in callback `onSelectionChange` we get 0 length from `mText.getCurrentText()`.

No idea of the meaning of `unboundedOldEnd`, neither its expected value. Jim, could you give some hints?

[1] https://dxr.mozilla.org/mozilla-central/rev/6dbc6e9f62a705d5f523cc750811bd01c8275ec6/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java#1169
Flags: needinfo?(nchen)
unboundedOldEnd should only be larger than currentLength when focus is changing, and we want to replace all current text. Otherwise, when focus is not changing, unboundedOldEnd should be less than or equal to currentLength. So in this case it sounds like a bug.

It's strange though that Aurora and Nightly have different behaviors, because the code hasn't changed recently.
Flags: needinfo?(nchen)
Are there assertions we only have enabled in "non release" builds like aurora/nightly?
IIRC these exceptions are in all builds.

Julian, since I was already looking at this bug, I can work it if you want.
Flags: needinfo?(walkingice0204)
OK, Thanks! Let me re-assign it. Please let me know if I need do anything in front-end.
Assignee: walkingice0204 → nchen
Flags: needinfo?(walkingice0204)
The block at [1] is a shortcut we take when we reconcile Java text
changes with Gecko text changes. However, we only checked that the new
ranges are the same, i.e. that the new Gecko text is the same as the new
Java text. We should also be checking that the old ranges are the same,
i.e. that the replaced Gecko text is the same as the replaced Java text.

[1] https://dxr.mozilla.org/mozilla-central/rev/bbbd2f7539f224a482cc6d2dd10e6a5f31c8baf3/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java#1233
Attachment #8821854 - Flags: review?(esawin)
nsContentIterator in pre mode adjusts its last node if the node is a
childless node like <br>. However, right now it's using GetPrevSibling,
which can lead to error in some edge cases such as:

<p></p><div><br></div>

In this case, if the last node is <br> with offset 0, GetPrevSibling
will return <p> because <p> is <br>'s parent's previous sibling, and the
last node will be set to <p>. However, the correct last node in this
case is <div>, because <br> with offset 0 refers to the position to the
left of <br>, which is <div> with offset 0. In this case, PrevNode
returns the correct <div> value, so we should set the last node to the
result of PrevNode.

For the first node, for a childless node in pre mode, GetNextSibling and
NextNode are the same, so there is no bug in this case. Nevertheless,
this patch changes the call to NextNode to be consistent with calling
PrevNode for the last node.
Attachment #8821855 - Flags: review?(masayuki)
Add a test for the previous patch that makes sure querying selected text
in an edge case works correctly.
Attachment #8821856 - Flags: review?(masayuki)
Comment on attachment 8821856 [details] [diff] [review]
3. Add test for correctly adjusting last node in content iterator (v1)

>   // #16
>   contenteditable.innerHTML = "a<blink>b</blink>c";
>   synthesizeSelectionSet(0, 3);
>   is(selection.anchorNode, contenteditable.firstChild,
>-     "runSetSelectionEventTest #15 (0, 0), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first text node");
>+     "runSetSelectionEventTest #16 (0, 3), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first text node");
>   is(selection.anchorOffset, 0,
>-     "runSetSelectionEventTest #15 (0, 0), \"" + contenteditable.innerHTML + "\": selection anchor offset should be 0");
>+     "runSetSelectionEventTest #16 (0, 3), \"" + contenteditable.innerHTML + "\": selection anchor offset should be 0");
>   is(selection.focusNode, contenteditable.lastChild,
>-     "runSetSelectionEventTest #15 (0, 0), \"" + contenteditable.innerHTML + "\": selection focus node should be the last text node");
>+     "runSetSelectionEventTest #16 (0, 3), \"" + contenteditable.innerHTML + "\": selection focus node should be the last text node");
>   is(selection.focusOffset, contenteditable.lastChild.wholeText.length,
>-     "runSetSelectionEventTest #15 (0, 0), \"" + contenteditable.innerHTML + "\": selection focus offset should be the length of the last text node");
>+     "runSetSelectionEventTest #16 (0, 3), \"" + contenteditable.innerHTML + "\": selection focus offset should be the length of the last text node");
>   checkSelection(0, "abc", "runSetSelectionEventTest #16 (0, 3), \"" + contenteditable.innerHTML + "\"");

Oh, thanks!

>+
>+  // #17 (bug 1319660 - incorrect adjustment of content iterator last node)
>+  contenteditable.innerHTML = "<div>a</div><div><br></div>";
>+
>+  synthesizeSelectionSet(kLFLen, 1+kLFLen);
>+  is(selection.anchorNode, contenteditable.firstChild,
>+     "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first text node");
>+  is(selection.anchorOffset, 0,
>+     "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\": selection anchor offset should be 0");
>+  is(selection.focusNode, contenteditable.lastChild,
>+     "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\": selection focus node should be the last text node");
>+  is(selection.focusOffset, 0,
>+     "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\": selection focus offset should be the length of the last text node");
>+  checkSelection(kLFLen, "a" + kLF, "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\"");

Hmm, although I'm not sure if the focus point is good result, anyway, it's out of scope of this bug.

>+
>+  synthesizeSelectionSet(1+2*kLFLen, 0);
>+  is(selection.anchorNode, contenteditable.lastChild,
>+     "runSetSelectionEventTest #17 (1+2*kLFLen, 0), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first text node");

should be "the second <div> element"?

>+  is(selection.anchorOffset, 0,
>+     "runSetSelectionEventTest #17 (1+2*kLFLen, 0), \"" + contenteditable.innerHTML + "\": selection anchor offset should be 0");
>+  is(selection.focusNode, contenteditable.lastChild,
>+     "runSetSelectionEventTest #17 (1+2*kLFLen, 0), \"" + contenteditable.innerHTML + "\": selection focus node should be the last text node");

ditto.
Attachment #8821856 - Flags: review?(masayuki) → review+
Comment on attachment 8821855 [details] [diff] [review]
2. Use previous node instead of sibling when adjusting last node (v1)

Looks like that this should work. However, I'm not a peer of DOM. So, should be reviewed by smaug too.
Attachment #8821855 - Flags: review?(masayuki)
Attachment #8821855 - Flags: review?(bugs)
Attachment #8821855 - Flags: review+
# I wonder, we should have automated tests of nsIContentIterator in mochitest-chrome.
Comment on attachment 8821855 [details] [diff] [review]
2. Use previous node instead of sibling when adjusting last node (v1)

This is regression risky stuff. Is there perhaps anything less risky we could take on branches?
Attachment #8821855 - Flags: review?(bugs) → review+
Attachment #8821854 - Flags: review?(esawin) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7c2edd54b8
1. Don't take shortcut if old replacement ranges don't match; r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/93353b53a706
2. Use previous node instead of sibling when adjusting last node; r=masayuki r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d506d3c193c9
3. Add test for correctly adjusting last node in content iterator; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/9a7c2edd54b8
https://hg.mozilla.org/mozilla-central/rev/93353b53a706
https://hg.mozilla.org/mozilla-central/rev/d506d3c193c9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8821854 [details] [diff] [review]
1. Don't take shortcut if old replacement ranges don't match (v1)

Requesting uplift to 52 for all patches. Given the risk and the fact that one of the patches only applies on 52, it's probably best to limit the uplift to 52 and won't fix for previous versions.


Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Crash when using some sites that rely on html editors (e.g. telegra.ph)
[Is this code covered by automated tests?]: Yes
[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?]: Somewhat.
[Why is the change risky/not risky?]: Although the new behavior in nsContentIterator is more correct, it is nonetheless a change in behavior and can have wide impact on mostly IME/editor code.
[String changes made/needed]: None
Attachment #8821854 - Flags: approval-mozilla-aurora?
Depends on: 1329446
Hello,

Verified this issue in the latest nightly on a Nexus 5 (Android 6.0.1) and it's no longer reproducible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The last patch changed the `GetNextSibling()` call to `NextNode()`
because I assumed they're equivalent in this case. That turned out to
not be the case because we can reach this line even if the node has
children -- the index just has to be after the last child. So this patch
restores the `GetNextSibling` call to restore the correct behavior.

I also added some comment to clarify that we can reach this line due to
one of two conditions: 1) the node has no children; 2) the node has
children but the index is after the last child.

This patch also replaces the `HasChildren()` check when setting
`cChild`.  If the index is after the last child (i.e. index ==
childCount), `GetChildAt()` fails and we erroneously log an assertion
warning, even though the input was valid. The new check handles all
cases whether start node has children or not.
Attachment #8825212 - Flags: review?(bugs)
Comment on attachment 8825212 [details] [diff] [review]
Restore GetNextSibling call for first node of pre-content-iterator (v1)

Can we please get tests which would have captured the issues in the previous patch.


But I don't understand this. If startNode doesn't have children, the startOffset shouldn't be 1. startNode is after all range's 'startParent', in the spec terminology startContainer.
It is fine for the offset to be == GetChildCount(), which means offset points after the last child node. (in case of no child nodes, 0)

Maybe it is just the comment which is wrong, but at least that needs to be clarified.
Attachment #8825212 - Flags: review?(bugs) → review-
Add a new test case for the NextNode() regression. r=me for trivial
test-only patch.
Attachment #8825626 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8825212 [details] [diff] [review]
> Restore GetNextSibling call for first node of pre-content-iterator (v1)
> 
> But I don't understand this. If startNode doesn't have children, the
> startOffset shouldn't be 1. startNode is after all range's 'startParent', in
> the spec terminology startContainer.
> It is fine for the offset to be == GetChildCount(), which means offset
> points after the last child node. (in case of no child nodes, 0)
> 
> Maybe it is just the comment which is wrong, but at least that needs to be
> clarified.

I based that off of the comment block at [1], which seems to imply it's
possible for empty node to have offset 1. This was mentioned in bug 1215798
comment 7, but the next comment says apparently that's not true. So I guess
maybe [1] is just inaccurate. I changed that comment in this new patch.

I'm adding an IME test that covers this regression, but bug 1325850 is about
adding tests for nsContentIterator in general. 

[1] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/dom/base/nsContentIterator.cpp#373
Attachment #8825627 - Flags: review?(bugs)
Attachment #8825212 - Attachment is obsolete: true
oh, yeah, that comment is perhaps a bit unclear. If the child is <br> for example, then offset of course can be 1 and we're pointing to the end of <br>
So the regression was from changing GetNextSibling to NextNode only. Changing GetPrevSibling to PrevNode fixes the original bug and is not related to the regression.
Could you explain why we want to use Get*Sibling in one case but not in the other one.
Basically they're trying to accomplish different things. So in pre-mode the order of iteration follows the opening tag, e.g. iterating over

> <div> <br> </div> <p> </p>

outputs <div>, <br>, then <p>.


Now, in this example, if the start node is <div> and offset is 1, i.e.

> <div> <br> </div> <p> </p>
>           ^

The first output should be <p> because the next opening tag is <p>. Calling `NextNode` on <div> in this case gives you <br>, which is not what we want. Calling `GetNextSibling` gives you the correct result, <p>.


On the other hand, in this new example below, if the end node is <br> and offset is 0,

> <p> </p> <div> <br> </div>
>                 ^

We want to end right before <br> according to [1]. So the last output should be <div>. Calling `GetPrevSibling` on <br> gives you <p>, which is not what we want and results in the original bug. Calling `PrevNode` gives you the correct result, <div>

[1] http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/dom/base/nsContentIterator.cpp#429
Comment on attachment 8821854 [details] [diff] [review]
1. Don't take shortcut if old replacement ranges don't match (v1)

clearing aurora approval flag for now, it looks like we need a followup fix so feel free to re-request approval once that lands.
Attachment #8821854 - Flags: approval-mozilla-aurora?
Jim & James, I would like to backout this patch for causing bug 1329446.
It has been too long now as we are only a few days away from the merge.
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
(In reply to Sylvestre Ledru [:sylvestre] from comment #35)
> Jim & James, I would like to backout this patch for causing bug 1329446.
> It has been too long now as we are only a few days away from the merge.

Ok, makes sense.
Flags: needinfo?(nchen)
Carsten, could you take care of the backout please? Thanks
Flags: needinfo?(snorp) → needinfo?(cbook)
Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/35960153165a56e99e7bff5fe12f75bcb31df7c2
Status: REOPENED → ASSIGNED
Flags: needinfo?(cbook)
Target Milestone: Firefox 53 → ---
Comment on attachment 8825627 [details] [diff] [review]
5. Restore GetNextSibling call for first node of pre-content-iterator (v2)


>+  // Valid start indices are 0 <= startIndx <= childCount. That means if start
>+  // node has no children, only offset 0 is valid.
>+  if (!startIsData && uint32_t(startIndx) != startNode->GetChildCount()) {
Could you make the latter condition startIndx < startNode->GetChildCount()
Attachment #8825627 - Flags: review?(bugs) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0bd28150d4f
Fix possible crash when editing contentEditable; r=esawin r=masayuki r=smaug
https://hg.mozilla.org/mozilla-central/rev/a0bd28150d4f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Attached patch Combined patchSplinter Review
Requesting uplift again.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Some sites that rely on contentEditable editors (like telegra.ph) will not work correctly
[Is this code covered by automated tests?]: Yes
[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?]: Small risk
[Why is the change risky/not risky?]: The patch introduces a slight change in behavior on all platforms, and there was a regression the first time this bug landed. Nevertheless, we haven't seen any more regressions, and given we're early in the cycle, I think the risk is acceptable.
[String changes made/needed]: None
Attachment #8821854 - Attachment is obsolete: true
Attachment #8821855 - Attachment is obsolete: true
Attachment #8821856 - Attachment is obsolete: true
Attachment #8825626 - Attachment is obsolete: true
Attachment #8825627 - Attachment is obsolete: true
Attachment #8831790 - Flags: review+
Attachment #8831790 - Flags: approval-mozilla-beta?
Attachment #8831790 - Flags: approval-mozilla-aurora?
Comment on attachment 8831790 [details] [diff] [review]
Combined patch

Happy to try this on aurora. 
Julien, what do you think for beta 52? We might try it for beta 3, but I leave it to you...
Attachment #8831790 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(jcristau)
Comment on attachment 8831790 [details] [diff] [review]
Combined patch

Last time around the regression was caught in 3 days on nightly, and this one has stuck for a week, so let's try and include this in 52.0b3.
Flags: needinfo?(jcristau)
Attachment #8831790 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on all branches 54.0a1(07-02-2017), 53.0a2(07-02-2017) and 52.0b4 on both Samsung Galaxy S6 EDGE (Android 6.0) and Nexus 7 (Android 5.1.1)
tracking-fennec: ? → 52+
You need to log in before you can comment on or make changes to this bug.