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 <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.

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

@@ -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 <br> 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 <br> 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 <br> 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))) {

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

widget/tests/window_composition_text_querycontent.xul

@@ -4020,14 +4020,53 @@ function runSetSelectionEventTest()
   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 + "\"");
+
+  // #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 <div> 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 <div> 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 <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 second <div> 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 = "<div><br></div><div><br></div>";
+
+  synthesizeSelectionSet(2*kLFLen, kLFLen);
+  is(selection.anchorNode, contenteditable.firstChild,
+     "runSetSelectionEventTest #18 (2*kLFLen, kLFLen), \"" + contenteditable.innerHTML + "\": selection anchor node should be the first <div> 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 <div> 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()