Bug 1424450 - Don't set selection on ClearStyle. r?masayuki draft
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Wed, 18 Apr 2018 16:13:24 +0900
changeset 784866 576c700055e357a893a8fe9ba5067dc9e8fc1d48
parent 783966 789e30ff2e3d6e1fcfce1a373c1e5635488d24da
push id107063
push userbmo:m_kato@ga2.so-net.ne.jp
push dateThu, 19 Apr 2018 08:52:36 +0000
reviewersmasayuki
bugs1424450
milestone61.0a1
Bug 1424450 - Don't set selection on ClearStyle. r?masayuki This crash is that HTMLEditor::ClearStyle returns nullptr for aNode even if successful. When current start node and offset isn't within ancestor limiter, HTMLEditor::ClearStyle will return nullptr for split node even if successful. Because SplitNodeTransation returns error since Selection::Collapse is failed. Then, SplitNodeDeep in HTMLEditor::SplitStyleAvovePoint returns error. But this error is ignored. So node will becomes null even if successful. CreateStyleForInsertText will set new selection when there is split node, so we shouldn't set selection on ClearStyle. Also, InsertNodeTransation is ignored for error when Collapse is failed, but SplitNodeTransaction isn't ignored. We should create a rule when collapse is failed on transaction. And at feature, we shouldn't set selection in CreateStyleForInsertText, and then, it should return new insertion point for InsertText instead of setting new selection. MozReview-Commit-ID: BRKWLqTfrvC
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/crashtests/1424450.html
editor/libeditor/crashtests/crashtests.list
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -4916,94 +4916,98 @@ HTMLEditRules::ConvertListType(Element* 
 /**
  * CreateStyleForInsertText() takes care of clearing and setting appropriate
  * style nodes for text insertion.
  */
 nsresult
 HTMLEditRules::CreateStyleForInsertText(Selection& aSelection,
                                         nsIDocument& aDoc)
 {
-  MOZ_ASSERT(mHTMLEditor->mTypeInState);
+  if (NS_WARN_IF(!mHTMLEditor)) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+  RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
+
+  MOZ_ASSERT(htmlEditor->mTypeInState);
 
   bool weDidSomething = false;
   NS_ENSURE_STATE(aSelection.GetRangeAt(0));
   nsCOMPtr<nsINode> node = aSelection.GetRangeAt(0)->GetStartContainer();
   int32_t offset = aSelection.GetRangeAt(0)->StartOffset();
 
   nsCOMPtr<Element> rootElement = aDoc.GetRootElement();
   NS_ENSURE_STATE(rootElement);
 
   // process clearing any styles first
   UniquePtr<PropItem> item =
-    Move(mHTMLEditor->mTypeInState->TakeClearProperty());
-  while (item && node != rootElement) {
-    NS_ENSURE_STATE(mHTMLEditor);
-    // XXX If we redesign ClearStyle(), we can use EditorDOMPoint in this
-    //     method.
-    nsresult rv =
-      mHTMLEditor->ClearStyle(address_of(node), &offset,
-                              item->tag, item->attr);
-    NS_ENSURE_SUCCESS(rv, rv);
-    item = Move(mHTMLEditor->mTypeInState->TakeClearProperty());
-    weDidSomething = true;
+    Move(htmlEditor->mTypeInState->TakeClearProperty());
+
+  {
+    // Transactions may set selection, but we will set selection if necessary.
+    AutoTransactionsConserveSelection dontChangeMySelection(htmlEditor);
+
+    while (item && node != rootElement) {
+      // XXX If we redesign ClearStyle(), we can use EditorDOMPoint in this
+      //     method.
+      nsresult rv =
+        htmlEditor->ClearStyle(address_of(node), &offset,
+                               item->tag, item->attr);
+      NS_ENSURE_SUCCESS(rv, rv);
+      item = Move(htmlEditor->mTypeInState->TakeClearProperty());
+      weDidSomething = true;
+    }
   }
 
   // then process setting any styles
-  int32_t relFontSize = mHTMLEditor->mTypeInState->TakeRelativeFontSize();
-  item = Move(mHTMLEditor->mTypeInState->TakeSetProperty());
+  int32_t relFontSize = htmlEditor->mTypeInState->TakeRelativeFontSize();
+  item = Move(htmlEditor->mTypeInState->TakeSetProperty());
 
   if (item || relFontSize) {
     // we have at least one style to add; make a new text node to insert style
     // nodes above.
     if (RefPtr<Text> text = node->GetAsText()) {
-      if (NS_WARN_IF(!mHTMLEditor)) {
-        return NS_ERROR_FAILURE;
-      }
       // if we are in a text node, split it
       SplitNodeResult splitTextNodeResult =
-        mHTMLEditor->SplitNodeDeep(*text, EditorRawDOMPoint(text, offset),
+        htmlEditor->SplitNodeDeep(*text, EditorRawDOMPoint(text, offset),
                                    SplitAtEdges::eAllowToCreateEmptyContainer);
       if (NS_WARN_IF(splitTextNodeResult.Failed())) {
         return splitTextNodeResult.Rv();
       }
       EditorRawDOMPoint splitPoint(splitTextNodeResult.SplitPoint());
       node = splitPoint.GetContainer();
       offset = splitPoint.Offset();
     }
-    if (!mHTMLEditor->IsContainer(node)) {
+    if (!htmlEditor->IsContainer(node)) {
       return NS_OK;
     }
     OwningNonNull<Text> newNode =
       EditorBase::CreateTextNode(aDoc, EmptyString());
-    NS_ENSURE_STATE(mHTMLEditor);
     nsresult rv =
-      mHTMLEditor->InsertNode(*newNode, EditorRawDOMPoint(node, offset));
+      htmlEditor->InsertNode(*newNode, EditorRawDOMPoint(node, offset));
     NS_ENSURE_SUCCESS(rv, rv);
     node = newNode;
     offset = 0;
     weDidSomething = true;
 
     if (relFontSize) {
       // dir indicated bigger versus smaller.  1 = bigger, -1 = smaller
       HTMLEditor::FontSize dir = relFontSize > 0 ?
         HTMLEditor::FontSize::incr : HTMLEditor::FontSize::decr;
       for (int32_t j = 0; j < DeprecatedAbs(relFontSize); j++) {
-        NS_ENSURE_STATE(mHTMLEditor);
-        rv = mHTMLEditor->RelativeFontChangeOnTextNode(dir, newNode, 0, -1);
+        rv = htmlEditor->RelativeFontChangeOnTextNode(dir, newNode, 0, -1);
         NS_ENSURE_SUCCESS(rv, rv);
       }
     }
 
     while (item) {
-      NS_ENSURE_STATE(mHTMLEditor);
-      rv = mHTMLEditor->SetInlinePropertyOnNode(*node->AsContent(),
-                                                *item->tag, item->attr,
-                                                item->value);
+      rv = htmlEditor->SetInlinePropertyOnNode(*node->AsContent(),
+                                               *item->tag, item->attr,
+                                               item->value);
       NS_ENSURE_SUCCESS(rv, rv);
-      item = mHTMLEditor->mTypeInState->TakeSetProperty();
+      item = htmlEditor->mTypeInState->TakeSetProperty();
     }
   }
   if (weDidSomething) {
     return aSelection.Collapse(node, offset);
   }
 
   return NS_OK;
 }
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/crashtests/1424450.html
@@ -0,0 +1,29 @@
+<script>
+function go() {
+  let selection = window.getSelection();
+  selection.setPosition(htmlvar00007, 1);
+  selection.setBaseAndExtent(htmlvar00011, 0, htmlvar00043, 0);
+  svgvar00014.before(svgvar00008.previousElementSibling);
+
+  document.execCommand("removeFormat", false);
+  document.execCommand("hiliteColor", false, "-moz-buttondefault");
+  document.execCommand("insertText", false, "");
+}
+function eh1() {
+  svgvar00007.appendChild(htmlvar00011);
+  htmlvar00003.appendChild(htmlvar00035);
+}
+</script>
+<body onload=go()>
+<span id="htmlvar00003">
+<pre id="htmlvar00007" contenteditable="true">
+<fieldset id="htmlvar00011"></fieldset>
+<iframe srcdoc="H"></iframe>
+<a id="htmlvar00035" hidden="hidden" contenteditable="true">
+<svg>
+<set onbegin="eh1()"/>
+<use id="htmlvar00043">
+<desc id="svgvar00007"></desc>
+</use>
+<font-face-uri id="svgvar00008"/>
+<feComponentTransfer id="svgvar00014">
--- a/editor/libeditor/crashtests/crashtests.list
+++ b/editor/libeditor/crashtests/crashtests.list
@@ -91,12 +91,13 @@ load 1393171.html
 needs-focus load 1402196.html
 load 1402469.html
 load 1402526.html
 load 1402904.html
 load 1405747.html
 load 1408170.html
 load 1414581.html
 load 1415231.html
+needs-focus load 1424450.html
 load 1425091.html
 load 1443664.html
 skip-if(Android) needs-focus load 1444630.html
 load 1446451.html