Bug 1467794 - Split TextEditor::DeleteSelectionAsAction() to itself a…

…nd 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

@@ -1350,8 +1350,8 @@ HTMLEditRules::WillInsertText(EditSubAction aEditSubAction,
   // 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;
     }
@@ -1707,8 +1707,8 @@ HTMLEditRules::WillInsertBreak(bool* aCancel,
   // 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;
     }

editor/libeditor/HTMLEditor.cpp

@@ -1163,7 +1163,7 @@ HTMLEditor::InsertBrElementAtSelectionWithTransaction()
   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;
     }
@@ -1587,7 +1587,7 @@ HTMLEditor::InsertElementAtSelection(Element* aElement,
         // 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;
         }

editor/libeditor/HTMLEditorDataTransfer.cpp

@@ -117,7 +117,7 @@ HTMLEditor::LoadHTML(const nsAString& aInputString)
   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;
       }
@@ -241,7 +241,7 @@ HTMLEditor::DoInsertHTMLWithContext(const nsAString& aInputString,
       // 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;
       }
@@ -270,7 +270,7 @@ HTMLEditor::DoInsertHTMLWithContext(const nsAString& aInputString,
     // 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;
       }

editor/libeditor/HTMLTableEditor.cpp

@@ -681,7 +681,7 @@ HTMLEditor::DeleteTable2(Element* aTable,
   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;
   }
@@ -980,6 +980,8 @@ HTMLEditor::DeleteTableColumn(int32_t aNumber)
   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);
@@ -988,7 +990,6 @@ HTMLEditor::DeleteTableColumn(int32_t aNumber)
   // 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,

editor/libeditor/TextEditRules.cpp

@@ -471,8 +471,8 @@ TextEditRules::WillInsertBreak(bool* aCancel,
 
     // 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;
       }
@@ -725,8 +725,8 @@ TextEditRules::WillInsertText(EditSubAction aEditSubAction,
 
   // 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;
     }

editor/libeditor/TextEditor.cpp

@@ -656,6 +656,28 @@ 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;
@@ -664,16 +686,11 @@ TextEditor::DeleteSelectionAsAction(EDirection aDirection,
   // 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
@@ -702,6 +719,10 @@ TextEditor::DeleteSelectionAsAction(EDirection aDirection,
     }
   }
 
+  AutoTopLevelEditSubActionNotifier maybeTopLevelEditSubAction(
+                                      *this,
+                                      EditSubAction::eDeleteSelectedContent,
+                                      aDirection);
   EditSubActionInfo subActionInfo(EditSubAction::eDeleteSelectedContent);
   subActionInfo.collapsedAction = aDirection;
   subActionInfo.stripWrappers = aStripWrappers;
@@ -864,7 +885,7 @@ TextEditor::DeleteSelectionAndPrepareToCreateNode()
   }
 
   if (!selection->GetAnchorFocusRange()->Collapsed()) {
-    nsresult rv = DeleteSelectionAsAction(eNone, eStrip);
+    nsresult rv = DeleteSelectionAsSubAction(eNone, eStrip);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
@@ -1142,7 +1163,7 @@ TextEditor::SetText(const nsAString& aString)
     }
     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);
@@ -1631,7 +1652,10 @@ 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;
 }

editor/libeditor/TextEditor.h

@@ -117,7 +117,8 @@ class TextEditor : public EditorBase
   /**
    * 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
@@ -198,6 +199,18 @@ class TextEditor : public EditorBase
   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.

editor/libeditor/TextEditorDataTransfer.cpp

@@ -82,7 +82,7 @@ TextEditor::InsertTextAt(const nsAString& aStringToInsert,
       // 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;
       }

editor/libeditor/crashtests/1405747.html

@@ -1,9 +1,17 @@
 <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) { }

editor/libeditor/crashtests/crashtests.list

@@ -91,11 +91,11 @@ 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

editor/libeditor/tests/test_bug1425997.html

@@ -24,6 +24,9 @@
 
 <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");