Bug 1464251 - SplitNodeDeepWithTransaction might create orphan node. r=masayuki
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Thu, 09 Aug 2018 08:22:50 +0000
changeset 486267 147cb3ed71f8dc1f24653b4486cecc54c52ad5a1
parent 486266 7717c47bd4ed667fa6dd0148f8653a2a57692754
child 486268 b11bd5b080a2cd7feac3c7be175bd4e4b7b0dd2a
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmasayuki
bugs1464251
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1464251 - SplitNodeDeepWithTransaction might create orphan node. r=masayuki SplitStyleAbovePoint calls SplitNodeDeepWithTransaction repeatedly. If SplitNodeDeepWithTransaction creates orphan node like this test case, this crash occurs. So we should check whether node becomes orphan node. Differential Revision: https://phabricator.services.mozilla.com/D1993
editor/libeditor/HTMLStyleEditor.cpp
editor/libeditor/crashtests/1464251.html
editor/libeditor/crashtests/crashtests.list
--- a/editor/libeditor/HTMLStyleEditor.cpp
+++ b/editor/libeditor/HTMLStyleEditor.cpp
@@ -544,17 +544,17 @@ HTMLEditor::SplitStyleAbovePoint(nsCOMPt
   if (aOutLeftNode) {
     *aOutLeftNode = nullptr;
   }
   if (aOutRightNode) {
     *aOutRightNode = nullptr;
   }
 
   // Split any matching style nodes above the node/offset
-  OwningNonNull<nsIContent> node = *(*aNode)->AsContent();
+  nsCOMPtr<nsIContent> node = (*aNode)->AsContent();
 
   bool useCSS = IsCSSEnabled();
 
   bool isSet;
   while (!IsBlockNode(node) && node->GetParent() &&
          IsEditable(node->GetParent())) {
     isSet = false;
     if (useCSS && CSSEditUtils::IsCSSEditableProperty(node, aProperty,
@@ -567,17 +567,17 @@ HTMLEditor::SplitStyleAbovePoint(nsCOMPt
                 node, aProperty, aAttribute, firstValue,
                 CSSEditUtils::eSpecified);
     }
     if (// node is the correct inline prop
         (aProperty && node->IsHTMLElement(aProperty)) ||
         // node is href - test if really <a href=...
         (aProperty == nsGkAtoms::href && HTMLEditUtils::IsLink(node)) ||
         // or node is any prop, and we asked to split them all
-        (!aProperty && NodeIsProperty(node)) ||
+        (!aProperty && NodeIsProperty(*node)) ||
         // or the style is specified in the style attribute
         isSet) {
       // Found a style node we need to split
       SplitNodeResult splitNodeResult =
         SplitNodeDeepWithTransaction(*node, EditorRawDOMPoint(*aNode, *aOffset),
                                      SplitAtEdges::eAllowToCreateEmptyContainer);
       NS_WARNING_ASSERTION(splitNodeResult.Succeeded(),
         "Failed to split the node");
@@ -588,16 +588,19 @@ HTMLEditor::SplitStyleAbovePoint(nsCOMPt
       if (aOutLeftNode) {
         NS_IF_ADDREF(*aOutLeftNode = splitNodeResult.GetPreviousNode());
       }
       if (aOutRightNode) {
         NS_IF_ADDREF(*aOutRightNode = splitNodeResult.GetNextNode());
       }
     }
     node = node->GetParent();
+    if (NS_WARN_IF(!node)) {
+      return NS_ERROR_FAILURE;
+    }
   }
 
   return NS_OK;
 }
 
 nsresult
 HTMLEditor::ClearStyle(nsCOMPtr<nsINode>* aNode,
                        int32_t* aOffset,
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/crashtests/1464251.html
@@ -0,0 +1,21 @@
+<script>
+var count = 0;
+
+function go() {
+  document.execCommand("delete", false);
+}
+function eh() {
+  count++;
+  if (count >= 3) {
+    return;
+  }
+  window.addEventListener("DOMNodeInserted", eh);
+  document.execCommand("removeFormat", false);
+  document.execCommand("insertText", false, "1");
+}
+</script>
+<body onload=go()>
+<ol oninput="eh()" contenteditable="true">
+<!-- x -->
+<s>
+<!-- x -->
--- a/editor/libeditor/crashtests/crashtests.list
+++ b/editor/libeditor/crashtests/crashtests.list
@@ -99,8 +99,9 @@ asserts(0-1) load 1414581.html
 load 1415231.html
 load 1423767.html
 needs-focus load 1423776.html
 needs-focus load 1424450.html
 load 1425091.html
 load 1443664.html
 skip-if(Android) needs-focus load 1444630.html
 load 1446451.html
+asserts(0-2) load 1464251.html # assertion is that mutation event listener modifies content