Bug 1467794 - Split TextEditor::DeleteSelectionAsAction() to itself and TextEditor::DeleteSelectionAsSubAction() r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 29 Jun 2018 20:16:50 +0900
changeset 814139 7efe662a874d20c9b7747c55f15900dc5da8545e
parent 814053 3d7f2fdc5bf7ca7521013b28e0d8be0785d2b58a
child 814140 e4190bcbd4ef1b4dc8bb21c265c667cd007326d1
push id115123
push userjdescottes@mozilla.com
push dateWed, 04 Jul 2018 17:42:29 +0000
reviewersm_kato
bugs1467794, 1465702, 1405747
milestone63.0a1
Bug 1467794 - Split TextEditor::DeleteSelectionAsAction() to itself and TextEditor::DeleteSelectionAsSubAction() r=m_kato TextEditor::DeleteSelectionAsAction() is called even if it's a part of edit action. For example, it's called to prepare for inserting text. For bug 1465702, editor itself and edit rules classes should not call public DeleteSelectionAsAction() directly. Therefore, this patch creates DeleteSelectionAsSubAction() for internal use. Note that this patch adds NS_ASSERTION() to detect wrong caller. However, it cannot distinguish if the call is valid, for example, it's allowed to call DeleteSelectionAsSelection() even if it's handling an edit action but the method is called via mutation event listener. So, we need to allow some assertions with some tests. But unfortunately, 1405747.html uses mutation event listener too many times (about 1,000 times) and the number of assertion isn't stable. Therefore, this patch makes the test stop using the mutation event listener 2nd time since I can reproduce the crash with ESR 52 at the 2nd time. MozReview-Commit-ID: 1TWaypmnoCC
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditorDataTransfer.cpp
editor/libeditor/HTMLTableEditor.cpp
editor/libeditor/TextEditRules.cpp
editor/libeditor/TextEditor.cpp
editor/libeditor/TextEditor.h
editor/libeditor/TextEditorDataTransfer.cpp
editor/libeditor/crashtests/1405747.html
editor/libeditor/crashtests/crashtests.list
editor/libeditor/tests/test_bug1425997.html
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1345,18 +1345,18 @@ HTMLEditRules::WillInsertText(EditSubAct
 
   // initialize out param
   *aCancel = false;
   *aHandled = true;
   // If the selection isn't collapsed, delete it.  Don't delete existing inline
   // tags, because we're hopefully going to insert text (bug 787432).
   if (!SelectionRef().IsCollapsed()) {
     nsresult rv =
-      HTMLEditorRef().DeleteSelectionAsAction(nsIEditor::eNone,
-                                              nsIEditor::eNoStrip);
+      HTMLEditorRef().DeleteSelectionAsSubAction(nsIEditor::eNone,
+                                                 nsIEditor::eNoStrip);
     if (NS_WARN_IF(!CanHandleEditAction())) {
       return NS_ERROR_EDITOR_DESTROYED;
     }
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
@@ -1702,18 +1702,18 @@ HTMLEditRules::WillInsertBreak(bool* aCa
 
   MOZ_ASSERT(aCancel && aHandled);
   *aCancel = false;
   *aHandled = false;
 
   // If the selection isn't collapsed, delete it.
   if (!SelectionRef().IsCollapsed()) {
     nsresult rv =
-      HTMLEditorRef().DeleteSelectionAsAction(nsIEditor::eNone,
-                                              nsIEditor::eStrip);
+      HTMLEditorRef().DeleteSelectionAsSubAction(nsIEditor::eNone,
+                                                 nsIEditor::eStrip);
     if (NS_WARN_IF(!CanHandleEditAction())) {
       return NS_ERROR_EDITOR_DESTROYED;
     }
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -1158,17 +1158,17 @@ HTMLEditor::InsertBrElementAtSelectionWi
   AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
                                       *this, EditSubAction::eInsertText,
                                       nsIEditor::eNext);
 
   RefPtr<Selection> selection = GetSelection();
   NS_ENSURE_STATE(selection);
 
   if (!selection->IsCollapsed()) {
-    nsresult rv = DeleteSelectionAsAction(eNone, eStrip);
+    nsresult rv = DeleteSelectionAsSubAction(eNone, eStrip);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
   EditorRawDOMPoint atStartOfSelection(GetStartPoint(selection));
   if (NS_WARN_IF(!atStartOfSelection.IsSet())) {
     return NS_ERROR_FAILURE;
@@ -1582,17 +1582,17 @@ HTMLEditor::InsertElementAtSelection(Ele
 
   if (!handled) {
     if (aDeleteSelection) {
       if (!IsBlockNode(aElement)) {
         // E.g., inserting an image.  In this case we don't need to delete any
         // inline wrappers before we do the insertion.  Otherwise we let
         // DeleteSelectionAndPrepareToCreateNode do the deletion for us, which
         // calls DeleteSelection with aStripWrappers = eStrip.
-        rv = DeleteSelectionAsAction(eNone, eNoStrip);
+        rv = DeleteSelectionAsSubAction(eNone, eNoStrip);
         if (NS_WARN_IF(NS_FAILED(rv))) {
           return rv;
         }
       }
 
       nsresult rv = DeleteSelectionAndPrepareToCreateNode();
       NS_ENSURE_SUCCESS(rv, rv);
     }
--- a/editor/libeditor/HTMLEditorDataTransfer.cpp
+++ b/editor/libeditor/HTMLEditorDataTransfer.cpp
@@ -112,17 +112,17 @@ HTMLEditor::LoadHTML(const nsAString& aI
   }
   if (cancel) {
     return NS_OK; // rules canceled the operation
   }
 
   if (!handled) {
     // Delete Selection, but only if it isn't collapsed, see bug #106269
     if (!selection->IsCollapsed()) {
-      rv = DeleteSelectionAsAction(eNone, eStrip);
+      rv = DeleteSelectionAsSubAction(eNone, eStrip);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
     }
 
     // Get the first range in the selection, for context:
     RefPtr<nsRange> range = selection->GetRangeAt(0);
     NS_ENSURE_TRUE(range, NS_ERROR_NULL_POINTER);
@@ -236,17 +236,17 @@ HTMLEditor::DoInsertHTMLWithContext(cons
   // also occur later; this block is intended to cover the various
   // scenarios where we are dropping in an editor (and may want to delete
   // the selection before collapsing the selection in the new destination)
   if (aDestNode) {
     if (aDeleteSelection) {
       // Use an auto tracker so that our drop point is correctly
       // positioned after the delete.
       AutoTrackDOMPoint tracker(mRangeUpdater, &targetPoint);
-      rv = DeleteSelectionAsAction(eNone, eStrip);
+      rv = DeleteSelectionAsSubAction(eNone, eStrip);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
     }
 
     ErrorResult error;
     selection->Collapse(targetPoint, error);
     if (NS_WARN_IF(error.Failed())) {
@@ -265,17 +265,17 @@ HTMLEditor::DoInsertHTMLWithContext(cons
                            streamStartOffset,
                            streamEndParent,
                            streamEndOffset);
 
   if (nodeList.IsEmpty()) {
     // We aren't inserting anything, but if aDeleteSelection is set, we do want
     // to delete everything.
     if (aDeleteSelection) {
-      nsresult rv = DeleteSelectionAsAction(eNone, eStrip);
+      nsresult rv = DeleteSelectionAsSubAction(eNone, eStrip);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
     }
     return NS_OK;
   }
 
   // Are there any table elements in the list?
--- a/editor/libeditor/HTMLTableEditor.cpp
+++ b/editor/libeditor/HTMLTableEditor.cpp
@@ -676,17 +676,17 @@ HTMLEditor::DeleteTable2(Element* aTable
   // Select the table
   nsresult rv = ClearSelection();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
   rv = AppendNodeToSelectionAsRange(aTable);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  rv = DeleteSelectionAsAction(eNext, eStrip);
+  rv = DeleteSelectionAsSubAction(eNext, eStrip);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 HTMLEditor::DeleteTable()
@@ -975,25 +975,26 @@ HTMLEditor::DeleteTableColumn(int32_t aN
                                &startRowIndex, &startColIndex);
   NS_ENSURE_SUCCESS(rv, rv);
   // Don't fail if no cell found
   NS_ENSURE_TRUE(table && cell, NS_SUCCESS_EDITOR_ELEMENT_NOT_FOUND);
 
   rv = GetTableSize(table, &rowCount, &colCount);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  AutoPlaceholderBatch beginBatching(this);
+
   // Shortcut the case of deleting all columns in table
   if (!startColIndex && aNumber >= colCount) {
     return DeleteTable2(table, selection);
   }
 
   // Check for counts too high
   aNumber = std::min(aNumber,(colCount-startColIndex));
 
-  AutoPlaceholderBatch beginBatching(this);
   // Prevent rules testing until we're done
   AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
                                       *this, EditSubAction::eDeleteNode,
                                       nsIEditor::eNext);
 
   // Test if deletion is controlled by selected cells
   RefPtr<Element> firstCell;
   rv = GetFirstSelectedCell(nullptr, getter_AddRefs(firstCell));
--- a/editor/libeditor/TextEditRules.cpp
+++ b/editor/libeditor/TextEditRules.cpp
@@ -466,18 +466,18 @@ TextEditRules::WillInsertBreak(bool* aCa
       *aCancel = true;
       return NS_OK;
     }
 
     *aCancel = false;
 
     // if the selection isn't collapsed, delete it.
     if (!SelectionRef().IsCollapsed()) {
-      rv = TextEditorRef().DeleteSelectionAsAction(nsIEditor::eNone,
-                                                   nsIEditor::eStrip);
+      rv = TextEditorRef().DeleteSelectionAsSubAction(nsIEditor::eNone,
+                                                      nsIEditor::eStrip);
       if (NS_WARN_IF(!CanHandleEditAction())) {
         return NS_ERROR_EDITOR_DESTROYED;
       }
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
     }
 
@@ -720,18 +720,18 @@ TextEditRules::WillInsertText(EditSubAct
   if (IsPasswordEditor()) {
     nsContentUtils::GetSelectionInTextControl(&SelectionRef(),
                                               TextEditorRef().GetRoot(),
                                               start, end);
   }
 
   // if the selection isn't collapsed, delete it.
   if (!SelectionRef().IsCollapsed()) {
-    rv = TextEditorRef().DeleteSelectionAsAction(nsIEditor::eNone,
-                                                 nsIEditor::eStrip);
+    rv = TextEditorRef().DeleteSelectionAsSubAction(nsIEditor::eNone,
+                                                    nsIEditor::eStrip);
     if (NS_WARN_IF(!CanHandleEditAction())) {
       return NS_ERROR_EDITOR_DESTROYED;
     }
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -651,34 +651,51 @@ TextEditor::DeleteSelection(EDirection a
   return NS_OK;
 }
 
 nsresult
 TextEditor::DeleteSelectionAsAction(EDirection aDirection,
                                     EStripWrappers aStripWrappers)
 {
   MOZ_ASSERT(aStripWrappers == eStrip || aStripWrappers == eNoStrip);
+  // Showing this assertion is fine if this method is called by outside via
+  // mutation event listener or something.  Otherwise, this is called by
+  // wrong method.
+  NS_ASSERTION(!mPlaceholderBatch,
+    "Should be called only when this is the only edit action of the operation "
+    "unless mutation event listener nests some operations");
+
+  // delete placeholder txns merge.
+  AutoPlaceholderBatch batch(this, nsGkAtoms::DeleteTxnName);
+  nsresult rv = DeleteSelectionAsSubAction(aDirection, aStripWrappers);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+  return NS_OK;
+}
+
+nsresult
+TextEditor::DeleteSelectionAsSubAction(EDirection aDirection,
+                                       EStripWrappers aStripWrappers)
+{
+  MOZ_ASSERT(aStripWrappers == eStrip || aStripWrappers == eNoStrip);
+  MOZ_ASSERT(mPlaceholderBatch);
 
   if (!mRules) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   // Protect the edit rules object from dying
   RefPtr<TextEditRules> rules(mRules);
 
-  // delete placeholder txns merge.
-  AutoPlaceholderBatch batch(this, nsGkAtoms::DeleteTxnName);
-  AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
-                                      *this,
-                                      EditSubAction::eDeleteSelectedContent,
-                                      aDirection);
-
   // pre-process
   RefPtr<Selection> selection = GetSelection();
-  NS_ENSURE_TRUE(selection, NS_ERROR_NULL_POINTER);
+  if (NS_WARN_IF(!selection)) {
+    return NS_ERROR_FAILURE;
+  }
 
   // If there is an existing selection when an extended delete is requested,
   //  platforms that use "caret-style" caret positioning collapse the
   //  selection to the  start and then create a new selection.
   //  Platforms that use "selection-style" caret positioning just delete the
   //  existing selection without extending it.
   if (!selection->IsCollapsed()) {
     switch (aDirection) {
@@ -697,16 +714,20 @@ TextEditor::DeleteSelectionAsAction(EDir
         }
         break;
       }
       default:
         break;
     }
   }
 
+  AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
+                                      *this,
+                                      EditSubAction::eDeleteSelectedContent,
+                                      aDirection);
   EditSubActionInfo subActionInfo(EditSubAction::eDeleteSelectedContent);
   subActionInfo.collapsedAction = aDirection;
   subActionInfo.stripWrappers = aStripWrappers;
   bool cancel, handled;
   nsresult rv =
     rules->WillDoAction(selection, subActionInfo, &cancel, &handled);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
@@ -859,17 +880,17 @@ TextEditor::DeleteSelectionAndPrepareToC
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   if (NS_WARN_IF(!selection->GetAnchorFocusRange())) {
     return NS_OK;
   }
 
   if (!selection->GetAnchorFocusRange()->Collapsed()) {
-    nsresult rv = DeleteSelectionAsAction(eNone, eStrip);
+    nsresult rv = DeleteSelectionAsSubAction(eNone, eStrip);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
     MOZ_ASSERT(selection->GetAnchorFocusRange() &&
                selection->GetAnchorFocusRange()->Collapsed(),
                "Selection not collapsed after delete");
   }
 
@@ -1137,17 +1158,17 @@ TextEditor::SetText(const nsAString& aSt
         return NS_ERROR_FAILURE;
       }
       rv = selection->Collapse(rootElement, 0);
     } else {
       rv = EditorBase::SelectEntireDocument(selection);
     }
     if (NS_SUCCEEDED(rv)) {
       if (aString.IsEmpty()) {
-        rv = DeleteSelectionAsAction(eNone, eStrip);
+        rv = DeleteSelectionAsSubAction(eNone, eStrip);
         NS_WARNING_ASSERTION(NS_FAILED(rv), "Failed to remove all text");
       } else {
         rv = InsertTextAsAction(aString);
         NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to insert the new text");
       }
     }
   }
   // post-process
@@ -1626,17 +1647,20 @@ TextEditor::FireClipboardEvent(EventMess
   return !mDidPreDestroy;
 }
 
 NS_IMETHODIMP
 TextEditor::Cut()
 {
   bool actionTaken = false;
   if (FireClipboardEvent(eCut, nsIClipboard::kGlobalClipboard, &actionTaken)) {
-    DeleteSelectionAsAction(eNone, eStrip);
+    // XXX This transaction name is referred by PlaceholderTransaction::Merge()
+    //     so that we need to keep using it here.
+    AutoPlaceholderBatch batch(this, nsGkAtoms::DeleteTxnName);
+    DeleteSelectionAsSubAction(eNone, eStrip);
   }
   return actionTaken ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 TextEditor::CanCut(bool* aCanCut)
 {
   NS_ENSURE_ARG_POINTER(aCanCut);
--- a/editor/libeditor/TextEditor.h
+++ b/editor/libeditor/TextEditor.h
@@ -112,17 +112,18 @@ public:
    *
    * @param aStringToInsert     The string to insert.
    */
   nsresult InsertTextAsAction(const nsAString& aStringToInsert);
 
   /**
    * DeleteSelectionAsAction() removes selection content or content around
    * caret with transactions.  This should be used for handling it as an
-   * edit action.
+   * edit action.  If you'd like to remove selection for preparing to insert
+   * something, you probably should use DeleteSelectionAsSubAction().
    *
    * @param aDirection          How much range should be removed.
    * @param aStripWrappers      Whether the parent blocks should be removed
    *                            when they become empty.
    */
   nsresult DeleteSelectionAsAction(EDirection aDirection,
                                    EStripWrappers aStripWrappers);
 
@@ -194,16 +195,28 @@ protected: // May be called by friends.
   virtual nsresult SetAttributeOrEquivalent(Element* aElement,
                                             nsAtom* aAttribute,
                                             const nsAString& aValue,
                                             bool aSuppressTransaction) override;
   using EditorBase::RemoveAttributeOrEquivalent;
   using EditorBase::SetAttributeOrEquivalent;
 
   /**
+   * DeleteSelectionAsSubAction() removes selection content or content around
+   * caret with transactions.  This should be used for handling it as an
+   * edit sub-action.
+   *
+   * @param aDirection          How much range should be removed.
+   * @param aStripWrappers      Whether the parent blocks should be removed
+   *                            when they become empty.
+   */
+  nsresult DeleteSelectionAsSubAction(EDirection aDirection,
+                                      EStripWrappers aStripWrappers);
+
+  /**
    * DeleteSelectionWithTransaction() removes selected content or content
    * around caret with transactions.
    *
    * @param aDirection          How much range should be removed.
    * @param aStripWrappers      Whether the parent blocks should be removed
    *                            when they become empty.
    */
   virtual nsresult
--- a/editor/libeditor/TextEditorDataTransfer.cpp
+++ b/editor/libeditor/TextEditorDataTransfer.cpp
@@ -77,17 +77,17 @@ TextEditor::InsertTextAt(const nsAString
 
     nsCOMPtr<nsINode> targetNode = aDestinationNode;
     int32_t targetOffset = aDestOffset;
 
     if (aDoDeleteSelection) {
       // Use an auto tracker so that our drop point is correctly
       // positioned after the delete.
       AutoTrackDOMPoint tracker(mRangeUpdater, &targetNode, &targetOffset);
-      nsresult rv = DeleteSelectionAsAction(eNone, eStrip);
+      nsresult rv = DeleteSelectionAsSubAction(eNone, eStrip);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
     }
 
     ErrorResult error;
     selection->Collapse(RawRangeBoundary(targetNode, targetOffset), error);
     if (NS_WARN_IF(error.Failed())) {
--- a/editor/libeditor/crashtests/1405747.html
+++ b/editor/libeditor/crashtests/1405747.html
@@ -1,14 +1,22 @@
 <script>
+var target;
 function jsfuzzer() {
-try { htmlvar00017.addEventListener("DOMSubtreeModified", eventhandler5); } catch(e) { }
-try { htmlvar00017.align = ""; } catch(e) { }
+target = htmlvar00017; // Cache the target for removing the event handler.
+try { target.addEventListener("DOMSubtreeModified", eventhandler5); } catch(e) { }
+try { target.align = ""; } catch(e) { }
 }
+var count = 0;
 function eventhandler5() {
+if (count++ == 1) {
+  // If we didn't stop testing this, this event handler would be called too
+  // many times.  It's enough to run twice to reproduce the bug report.
+  target.removeEventListener("DOMSubtreeModified", eventhandler5);
+}
 try { document.execCommand("selectAll", false); } catch(e) { }
 try { document.execCommand("justifyCenter", false); } catch(e) { }
 try { document.execCommand("forwardDelete", false); } catch(e) { }
 }
 </script>
 <body onload=jsfuzzer()>
 <table contenteditable="">
 <th id="htmlvar00017"></th>
--- a/editor/libeditor/crashtests/crashtests.list
+++ b/editor/libeditor/crashtests/crashtests.list
@@ -86,21 +86,21 @@ load 1383747.html
 load 1383755.html
 load 1383763.html
 load 1384161.html
 load 1388075.html
 load 1393171.html
 needs-focus load 1402196.html
 load 1402469.html
 load 1402526.html
-load 1402904.html
-load 1405747.html
+asserts(1) load 1402904.html
+asserts(1) load 1405747.html
 load 1405897.html
 load 1408170.html
-load 1414581.html
+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
--- a/editor/libeditor/tests/test_bug1425997.html
+++ b/editor/libeditor/tests/test_bug1425997.html
@@ -19,16 +19,19 @@ https://bugzilla.mozilla.org/show_bug.cg
 <!-- -->
 <span id="inline">foo</span>
 </div>
 
 <pre id="test">
 
 <script class="testbody" type="application/javascript">
 SimpleTest.waitForExplicitFinish();
+// 3 assertions are recorded due to nested execCommand() but not a problem.
+// They are necessary to detect invalid method call without mutation event listers.
+SimpleTest.expectAssertions(3, 3);
 SimpleTest.waitForFocus(function() {
   let selection = window.getSelection();
   let editor = document.getElementById("editor");
   let originalContent = editor.innerHTML;
   function onCharacterDataModified() {
     // Until removing all NBSPs which were inserted by the editor,
     // emulates Backspace key with "delete" command.
     // When this test is created, the behavior is: