Bug 1534394 - Make WSRunObject::InsertText() set aPointAfterInsertedString by itself when HTMLEditor::InsertTextWithTransaction() returns error r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 23 May 2019 06:06:18 +0000
changeset 475137 0475e2855e5c4b03dfce9da151fd4ac2bb1367c3
parent 475136 4cd6d838796a453f9de5821b2e146f84174badfb
child 475138 8b8349028050387c536fd8c44afdba93c566832c
push id86156
push usermasayuki@d-toybox.com
push dateThu, 23 May 2019 06:49:18 +0000
treeherderautoland@0475e2855e5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1534394
milestone69.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 1534394 - Make WSRunObject::InsertText() set aPointAfterInsertedString by itself when HTMLEditor::InsertTextWithTransaction() returns error r=m_kato Oddly, `WSRunObject::InsertText()` returns `NS_OK` even when `HTMLEditor::InsertTextWithTransaction()` returns error. However, it fails if insertion point is not editable like `<noscript>` element. In such case, `aPointAfterInsertedString` isn't modified and its caller, `HTMLEditRules::WillInsertText()` keep handling inserting remaining text with non-positioned `EditorDOMPoint`. Therefore, at the next time, `WSRunObject` fails to do anything since it requires positioned `EditorDOMPoint`. For making uplift safer, this patch makes `WSRunObject::InsertText()` set `aPointAfterInsertedString` by itself when `HTMLEditor::InsertTextWithTransaction()` returns error. Differential Revision: https://phabricator.services.mozilla.com/D32131
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/WSRunObject.cpp
editor/libeditor/crashtests/1534394.html
editor/libeditor/crashtests/crashtests.list
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1536,16 +1536,17 @@ nsresult HTMLEditRules::WillInsertText(E
           rv = wsObj.InsertText(*doc, spacesStr, &pointAfterInsertedSpaces);
           if (NS_WARN_IF(!CanHandleEditAction())) {
             return NS_ERROR_EDITOR_DESTROYED;
           }
           if (NS_WARN_IF(NS_FAILED(rv))) {
             return rv;
           }
           pos++;
+          MOZ_ASSERT(pointAfterInsertedSpaces.IsSet());
           currentPoint = pointAfterInsertedSpaces;
           pointToInsert = pointAfterInsertedSpaces;
         }
         // is it a return?
         else if (subStr.Equals(newlineStr)) {
           RefPtr<Element> newBRElement =
               wsObj.InsertBreak(MOZ_KnownLive(*SelectionRefPtr()), currentPoint,
                                 nsIEditor::eNone);
@@ -1576,16 +1577,17 @@ nsresult HTMLEditRules::WillInsertText(E
           EditorRawDOMPoint pointAfterInsertedString;
           rv = wsObj.InsertText(*doc, subStr, &pointAfterInsertedString);
           if (NS_WARN_IF(!CanHandleEditAction())) {
             return NS_ERROR_EDITOR_DESTROYED;
           }
           if (NS_WARN_IF(NS_FAILED(rv))) {
             return rv;
           }
+          MOZ_ASSERT(pointAfterInsertedString.IsSet());
           currentPoint = pointAfterInsertedString;
           pointToInsert = pointAfterInsertedString;
         }
       }
     }
 
     // After this block, pointToInsert is updated by AutoTrackDOMPoint.
   }
--- a/editor/libeditor/WSRunObject.cpp
+++ b/editor/libeditor/WSRunObject.cpp
@@ -363,22 +363,28 @@ nsresult WSRunObject::InsertText(Documen
       } else {
         prevWS = true;
       }
     } else {
       prevWS = false;
     }
   }
 
-  // Ready, aim, fire!
+  // XXX If the point is not editable, InsertTextWithTransaction() returns
+  //     error, but we keep handling it.  But I think that it wastes the
+  //     runtime cost.  So, perhaps, we should return error code which couldn't
+  //     modify it and make each caller of this method decide whether it should
+  //     keep or stop handling the edit action.
   nsresult rv =
       MOZ_KnownLive(mHTMLEditor)
           ->InsertTextWithTransaction(aDocument, theString, pointToInsert,
                                       aPointAfterInsertedString);
   if (NS_WARN_IF(NS_FAILED(rv))) {
+    // XXX Temporarily, set new insertion point to the original point.
+    *aPointAfterInsertedString = pointToInsert;
     return NS_OK;
   }
   return NS_OK;
 }
 
 nsresult WSRunObject::DeleteWSBackward() {
   WSPoint point = GetPreviousCharPoint(mScanStartPoint);
   NS_ENSURE_TRUE(point.mTextNode, NS_OK);  // nothing to delete
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/crashtests/1534394.html
@@ -0,0 +1,29 @@
+<html>
+<head>
+    <script>
+
+      function start () {
+        const sel = document.getSelection()
+        const range_1 = new Range()
+        const noscript = document.getElementById('id_24')
+        const map = document.getElementById('id_26')
+        sel.selectAllChildren(noscript)
+        const range_2 = range_1.cloneRange()
+        range_2.selectNode(map)
+        const frag = range_2.extractContents()
+        range_2.insertNode(noscript)
+        document.documentElement.contentEditable = true
+        document.execCommand('removeFormat', false,)
+        noscript.contentEditable = false
+        document.execCommand('insertText', false, '�\n')
+      }
+
+      window.addEventListener('load', start)
+    </script>
+</head>
+<body>
+<big class="">
+    <map class="" id="id_26">
+        <noscript class="" id="id_24">
+</body>
+</html>
--- a/editor/libeditor/crashtests/crashtests.list
+++ b/editor/libeditor/crashtests/crashtests.list
@@ -103,9 +103,10 @@ needs-focus load 1424450.html
 load 1425091.html
 load 1441619.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
 pref(layout.accessiblecaret.enabled,true) load 1470926.html
 load 1525481.html
+load 1534394.html
 load 1547898.html