From 8b160783a8200b38d86b05c581a1f5b8114c7399 Mon Sep 17 00:00:00 2001 From: Jim Chen Date: Mon, 23 Jan 2017 14:35:04 -0500 Subject: [PATCH] Bug 1319660 - Fix possible crash when editing contentEditable; r=esawin r=masayuki r=smaug Bug 1319660 - 1. Don't take shortcut if old replacement ranges don't match; r=esawin 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 Bug 1319660 - 2. Use previous node instead of sibling when adjusting last node; r=masayuki r=smaug nsContentIterator in pre mode adjusts its last node if the node is a childless node like
. However, right now it's using GetPrevSibling, which can lead to error in some edge cases such as:


In this case, if the last node is
with offset 0, GetPrevSibling will return

because

is
's parent's previous sibling, and the last node will be set to

. However, the correct last node in this case is

, because
with offset 0 refers to the position to the left of
, which is
with offset 0. In this case, PrevNode returns the correct
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. Bug 1319660 - 3. Add test for correctly adjusting last node in content iterator; r=masayuki Add a test for the previous patch that makes sure querying selected text in an edge case works correctly. Bug 1319660 - 4. Add test for start node regression; r=me Add a new test case for the NextNode() regression. r=me for trivial test-only patch. Bug 1319660 - 5. Restore GetNextSibling call for first node of pre-content-iterator; r=smaug 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. --- dom/base/nsContentIterator.cpp | 20 ++++---- .../java/org/mozilla/gecko/GeckoEditable.java | 5 +- .../window_composition_text_querycontent.xul | 47 +++++++++++++++++-- 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/dom/base/nsContentIterator.cpp b/dom/base/nsContentIterator.cpp index 28448c0141eb..287de7722829 100644 --- a/dom/base/nsContentIterator.cpp +++ b/dom/base/nsContentIterator.cpp @@ -354,25 +354,25 @@ nsContentIterator::Init(nsIDOMRange* aDOMRange) nsIContent* cChild = nullptr; - if (!startIsData && startNode->HasChildren()) { + // 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()) { cChild = startNode->GetChildAt(startIndx); NS_WARNING_ASSERTION(cChild, "GetChildAt returned null"); } if (!cChild) { - // no children, must be a text node - // - // XXXbz no children might also just mean no children. So I'm not - // sure what that comment above is talking about. + // No children (possibly a
or text node), or index is after last child. if (mPre) { // XXX: In the future, if start offset is after the last // character in the cdata node, should we set mFirst to // the next sibling? - // If the node has no child, the child may be
or something. - // So, we shouldn't skip the empty node if the start offset is 0. - // In other words, if the offset is 1, the node should be ignored. + // Normally we would skip the start node because the start node is outside + // of the range in pre mode. However, if startIndx == 0, it means the node + // has no children, and the node may be
or something. We don't skip + // the node in this case in order to address bug 1215798. if (!startIsData && startIndx) { mFirst = GetNextSibling(startNode); NS_WARNING_ASSERTION(mFirst, "GetNextSibling returned null"); @@ -430,8 +430,8 @@ nsContentIterator::Init(nsIDOMRange* aDOMRange) // the last element should be the previous node (i.e., shouldn't // include the end node in the range). if (!endIsData && !endNode->HasChildren() && !endIndx) { - mLast = GetPrevSibling(endNode); - NS_WARNING_ASSERTION(mLast, "GetPrevSibling returned null"); + mLast = PrevNode(endNode); + NS_WARNING_ASSERTION(mLast, "PrevNode returned null"); if (NS_WARN_IF(!NodeIsInTraversalRange(mLast, mPre, startNode, startIndx, endNode, endIndx))) { diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java index 1b10ba015fd8..4be9597d7c95 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEditable.java @@ -1230,8 +1230,9 @@ final class GeckoEditable extends JNIObject // with Gecko here. mIgnoreSelectionChange = false; - } else if (indexInText == 0 && text.length() == action.mSequence.length()) { - // The new text exactly matches our sequence, so do a direct replace. + } else if (indexInText == 0 && text.length() == action.mSequence.length() && + oldEnd - start == action.mEnd - action.mStart) { + // The new change exactly matches our saved change, so do a direct replace. mText.currentReplace(start, oldEnd, action.mSequence); // Ignore the next selection change because the selection change is a diff --git a/widget/tests/window_composition_text_querycontent.xul b/widget/tests/window_composition_text_querycontent.xul index ecd936ac9eae..f345f549be7b 100644 --- a/widget/tests/window_composition_text_querycontent.xul +++ b/widget/tests/window_composition_text_querycontent.xul @@ -4020,14 +4020,53 @@ function runSetSelectionEventTest() contenteditable.innerHTML = "abc"; 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 + "\""); + + // #17 (bug 1319660 - incorrect adjustment of content iterator last node) + contenteditable.innerHTML = "
a

"; + + synthesizeSelectionSet(kLFLen, 1+kLFLen); + is(selection.anchorNode, contenteditable.firstChild, + "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first
element"); + 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 second
element"); + is(selection.focusOffset, 0, + "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\": selection focus offset should be 0"); + checkSelection(kLFLen, "a" + kLF, "runSetSelectionEventTest #17 (kLFLen, 1+kLFLen), \"" + contenteditable.innerHTML + "\""); + + synthesizeSelectionSet(1+2*kLFLen, 0); + is(selection.anchorNode, contenteditable.lastChild, + "runSetSelectionEventTest #17 (1+2*kLFLen, 0), \"" + contenteditable.innerHTML + "\": selection anchor node should be the second
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 second
element"); + is(selection.focusOffset, 0, + "runSetSelectionEventTest #17 (1+2*kLFLen, 0), \"" + contenteditable.innerHTML + "\": selection focus offset should be 0"); + checkSelection(1+2*kLFLen, "", "runSetSelectionEventTest #17 (1+2*kLFLen, 0), \"" + contenteditable.innerHTML + "\""); + + // #18 (bug 1319660 - content iterator start node regression) + contenteditable.innerHTML = "


"; + + synthesizeSelectionSet(2*kLFLen, kLFLen); + is(selection.anchorNode, contenteditable.firstChild, + "runSetSelectionEventTest #18 (2*kLFLen, kLFLen), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first
element"); + is(selection.anchorOffset, 1, + "runSetSelectionEventTest #18 (2*kLFLen, kLFLen), \"" + contenteditable.innerHTML + "\": selection anchor offset should be 1"); + is(selection.focusNode, contenteditable.lastChild, + "runSetSelectionEventTest #18 (2*kLFLen, kLFLen), \"" + contenteditable.innerHTML + "\": selection focus node should be the second
element"); + is(selection.focusOffset, 0, + "runSetSelectionEventTest #18 (2*kLFLen, kLFLen), \"" + contenteditable.innerHTML + "\": selection focus offset should be 0"); + checkSelection(2*kLFLen, kLF, "runSetSelectionEventTest #18 (2*kLFLen, kLFLen), \"" + contenteditable.innerHTML + "\""); } function runQueryTextContentEventTest()