From c66ed1275f1039fdf23a8f2c172d7c0b4691d1c2 Mon Sep 17 00:00:00 2001 From: "Matt A. Tobin" Date: Wed, 15 Apr 2020 01:55:25 -0400 Subject: Bug 1336011 - Fix Crash in InvalidArrayIndex_CRASH in mozilla::EditorBase::DeleteSelectionImpl * EditorBase shouldn't refer mActionListeners directly in loops because it might be removed during a loop * Create an alias of the type of mEditorObservers * Create an alias of the type of mDocStateListeners Tag #1375 --- editor/libeditor/EditorBase.cpp | 183 +++++++++++++++++++++++++--------------- editor/libeditor/EditorBase.h | 12 ++- 2 files changed, 124 insertions(+), 71 deletions(-) diff --git a/editor/libeditor/EditorBase.cpp b/editor/libeditor/EditorBase.cpp index f7988cd1a..f0f3095d6 100644 --- a/editor/libeditor/EditorBase.cpp +++ b/editor/libeditor/EditorBase.cpp @@ -1385,9 +1385,12 @@ EditorBase::CreateNode(nsIAtom* aTag, AutoRules beginRulesSniffing(this, EditAction::createNode, nsIEditor::eNext); - for (auto& listener : mActionListeners) { - listener->WillCreateNode(nsDependentAtomString(aTag), - GetAsDOMNode(aParent), aPosition); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillCreateNode(nsDependentAtomString(aTag), + GetAsDOMNode(aParent), aPosition); + } } nsCOMPtr ret; @@ -1402,9 +1405,12 @@ EditorBase::CreateNode(nsIAtom* aTag, mRangeUpdater.SelAdjCreateNode(aParent, aPosition); - for (auto& listener : mActionListeners) { - listener->DidCreateNode(nsDependentAtomString(aTag), GetAsDOMNode(ret), - GetAsDOMNode(aParent), aPosition, rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidCreateNode(nsDependentAtomString(aTag), GetAsDOMNode(ret), + GetAsDOMNode(aParent), aPosition, rv); + } } return ret.forget(); @@ -1429,9 +1435,12 @@ EditorBase::InsertNode(nsIContent& aNode, { AutoRules beginRulesSniffing(this, EditAction::insertNode, nsIEditor::eNext); - for (auto& listener : mActionListeners) { - listener->WillInsertNode(aNode.AsDOMNode(), aParent.AsDOMNode(), - aPosition); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillInsertNode(aNode.AsDOMNode(), aParent.AsDOMNode(), + aPosition); + } } RefPtr transaction = @@ -1440,9 +1449,12 @@ EditorBase::InsertNode(nsIContent& aNode, mRangeUpdater.SelAdjInsertNode(aParent.AsDOMNode(), aPosition); - for (auto& listener : mActionListeners) { - listener->DidInsertNode(aNode.AsDOMNode(), aParent.AsDOMNode(), aPosition, - rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidInsertNode(aNode.AsDOMNode(), aParent.AsDOMNode(), aPosition, + rv); + } } return rv; @@ -1468,8 +1480,11 @@ EditorBase::SplitNode(nsIContent& aNode, { AutoRules beginRulesSniffing(this, EditAction::splitNode, nsIEditor::eNext); - for (auto& listener : mActionListeners) { - listener->WillSplitNode(aNode.AsDOMNode(), aOffset); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillSplitNode(aNode.AsDOMNode(), aOffset); + } } RefPtr transaction = @@ -1482,9 +1497,12 @@ EditorBase::SplitNode(nsIContent& aNode, mRangeUpdater.SelAdjSplitNode(aNode, aOffset, newNode); nsresult rv = aResult.StealNSResult(); - for (auto& listener : mActionListeners) { - listener->DidSplitNode(aNode.AsDOMNode(), aOffset, GetAsDOMNode(newNode), - rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidSplitNode(aNode.AsDOMNode(), aOffset, GetAsDOMNode(newNode), + rv); + } } // Note: result might be a success code, so we can't use Throw() to // set it on aResult. @@ -1520,9 +1538,12 @@ EditorBase::JoinNodes(nsINode& aLeftNode, // Find the number of children of the lefthand node uint32_t oldLeftNodeLen = aLeftNode.Length(); - for (auto& listener : mActionListeners) { - listener->WillJoinNodes(aLeftNode.AsDOMNode(), aRightNode.AsDOMNode(), - parent->AsDOMNode()); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillJoinNodes(aLeftNode.AsDOMNode(), aRightNode.AsDOMNode(), + parent->AsDOMNode()); + } } nsresult rv = NS_OK; @@ -1535,9 +1556,12 @@ EditorBase::JoinNodes(nsINode& aLeftNode, mRangeUpdater.SelAdjJoinNodes(aLeftNode, aRightNode, *parent, offset, (int32_t)oldLeftNodeLen); - for (auto& listener : mActionListeners) { - listener->DidJoinNodes(aLeftNode.AsDOMNode(), aRightNode.AsDOMNode(), - parent->AsDOMNode(), rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidJoinNodes(aLeftNode.AsDOMNode(), aRightNode.AsDOMNode(), + parent->AsDOMNode(), rv); + } } return rv; @@ -1558,8 +1582,11 @@ EditorBase::DeleteNode(nsINode* aNode) nsIEditor::ePrevious); // save node location for selection updating code. - for (auto& listener : mActionListeners) { - listener->WillDeleteNode(aNode->AsDOMNode()); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillDeleteNode(aNode->AsDOMNode()); + } } RefPtr transaction; @@ -1568,8 +1595,11 @@ EditorBase::DeleteNode(nsINode* aNode) rv = DoTransaction(transaction); } - for (auto& listener : mActionListeners) { - listener->DidDeleteNode(aNode->AsDOMNode(), rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidDeleteNode(aNode->AsDOMNode(), rv); + } } NS_ENSURE_SUCCESS(rv, rv); @@ -1844,7 +1874,7 @@ void EditorBase::NotifyEditorObservers(NotificationForEditorObservers aNotification) { // Copy the observers since EditAction()s can modify mEditorObservers. - nsTArray> observers(mEditorObservers); + AutoEditorObserverArray observers(mEditorObservers); switch (aNotification) { case eNotifyEditorObserversOfEnd: mIsInEditAction = false; @@ -2505,10 +2535,13 @@ EditorBase::InsertTextIntoTextNodeImpl(const nsAString& aStringToInsert, } // Let listeners know what's up - for (auto& listener : mActionListeners) { - listener->WillInsertText( - static_cast(insertedTextNode->AsDOMNode()), - insertedOffset, aStringToInsert); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillInsertText( + static_cast(insertedTextNode->AsDOMNode()), + insertedOffset, aStringToInsert); + } } // XXX We may not need these view batches anymore. This is handled at a @@ -2518,10 +2551,13 @@ EditorBase::InsertTextIntoTextNodeImpl(const nsAString& aStringToInsert, EndUpdateViewBatch(); // let listeners know what happened - for (auto& listener : mActionListeners) { - listener->DidInsertText( - static_cast(insertedTextNode->AsDOMNode()), - insertedOffset, aStringToInsert, rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidInsertText( + static_cast(insertedTextNode->AsDOMNode()), + insertedOffset, aStringToInsert, rv); + } } // Added some cruft here for bug 43366. Layout was crashing because we left @@ -2583,8 +2619,7 @@ EditorBase::NotifyDocumentListeners( return NS_OK; } - nsTArray> - listeners(mDocStateListeners); + AutoDocumentStateListenerArray listeners(mDocStateListeners); nsresult rv = NS_OK; switch (aNotificationType) { @@ -2656,19 +2691,25 @@ EditorBase::DeleteText(nsGenericDOMDataNode& aCharData, nsIEditor::ePrevious); // Let listeners know what's up - for (auto& listener : mActionListeners) { - listener->WillDeleteText( - static_cast(GetAsDOMNode(&aCharData)), aOffset, - aLength); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->WillDeleteText( + static_cast(GetAsDOMNode(&aCharData)), aOffset, + aLength); + } } nsresult rv = DoTransaction(transaction); // Let listeners know what happened - for (auto& listener : mActionListeners) { - listener->DidDeleteText( - static_cast(GetAsDOMNode(&aCharData)), aOffset, - aLength, rv); + { + AutoActionListenerArray listeners(mActionListeners); + for (auto& listener : listeners) { + listener->DidDeleteText( + static_cast(GetAsDOMNode(&aCharData)), aOffset, + aLength, rv); + } } return rv; @@ -4034,17 +4075,20 @@ EditorBase::DeleteSelectionImpl(EDirection aAction, if (NS_SUCCEEDED(rv)) { AutoRules beginRulesSniffing(this, EditAction::deleteSelection, aAction); // Notify nsIEditActionListener::WillDelete[Selection|Text|Node] - if (!deleteNode) { - for (auto& listener : mActionListeners) { - listener->WillDeleteSelection(selection); - } - } else if (deleteCharData) { - for (auto& listener : mActionListeners) { - listener->WillDeleteText(deleteCharData, deleteCharOffset, 1); - } - } else { - for (auto& listener : mActionListeners) { - listener->WillDeleteNode(deleteNode->AsDOMNode()); + { + AutoActionListenerArray listeners(mActionListeners); + if (!deleteNode) { + for (auto& listener : listeners) { + listener->WillDeleteSelection(selection); + } + } else if (deleteCharData) { + for (auto& listener : listeners) { + listener->WillDeleteText(deleteCharData, deleteCharOffset, 1); + } + } else { + for (auto& listener : listeners) { + listener->WillDeleteNode(deleteNode->AsDOMNode()); + } } } @@ -4052,17 +4096,20 @@ EditorBase::DeleteSelectionImpl(EDirection aAction, rv = DoTransaction(transaction); // Notify nsIEditActionListener::DidDelete[Selection|Text|Node] - if (!deleteNode) { - for (auto& listener : mActionListeners) { - listener->DidDeleteSelection(selection); - } - } else if (deleteCharData) { - for (auto& listener : mActionListeners) { - listener->DidDeleteText(deleteCharData, deleteCharOffset, 1, rv); - } - } else { - for (auto& listener : mActionListeners) { - listener->DidDeleteNode(deleteNode->AsDOMNode(), rv); + { + AutoActionListenerArray listeners(mActionListeners); + if (!deleteNode) { + for (auto& listener : mActionListeners) { + listener->DidDeleteSelection(selection); + } + } else if (deleteCharData) { + for (auto& listener : mActionListeners) { + listener->DidDeleteText(deleteCharData, deleteCharOffset, 1, rv); + } + } else { + for (auto& listener : mActionListeners) { + listener->DidDeleteNode(deleteNode->AsDOMNode(), rv); + } } } } diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h index dbd00771e..8680b57ae 100644 --- a/editor/libeditor/EditorBase.h +++ b/editor/libeditor/EditorBase.h @@ -987,11 +987,17 @@ protected: RefPtr mComposition; // Listens to all low level actions on the doc. - nsTArray> mActionListeners; + typedef AutoTArray, 5> + AutoActionListenerArray; + AutoActionListenerArray mActionListeners; // Just notify once per high level change. - nsTArray> mEditorObservers; + typedef AutoTArray, 3> + AutoEditorObserverArray; + AutoEditorObserverArray mEditorObservers; // Listen to overall doc state (dirty or not, just created, etc.). - nsTArray> mDocStateListeners; + typedef AutoTArray, 1> + AutoDocumentStateListenerArray; + AutoDocumentStateListenerArray mDocStateListeners; // Cached selection for AutoSelectionRestorer. SelectionState mSavedSel; -- cgit v1.2.3