Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-24 21:31:04 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1727185 - Add MOZ_DIAGNOSTIC_ASSERT to the constructors of DeleteNodeTransaction and SplitNodeTransaction and make their users return error before hitting it r=m_kato

Browse Source 
On Thunderbird, we still have a bug to delete `<html>` and `<body>` elements
(bug 1727201).  However, it's hard to know where deletes the unexpected elements
from warning messages in the log.  Additionally, it's really serious bug
because editor and layout code rely on the basic structure of HTML document.
Therefore, we should block the worst scenario before deleting such nodes.

Differential Revision: https://phabricator.services.mozilla.com/D123418
This commit is contained in:
Masayuki Nakano 2021-08-25 00:39:41 +00:00
parent bb02f9029f
commit 229b5deba5
7 changed files with 75 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
editor/libeditor/DeleteNodeTransaction.cpp
View File

@ -32,7 +32,10 @@ DeleteNodeTransaction::DeleteNodeTransaction(EditorBase& aEditorBase,
nsIContent& aContentToDelete)
: mEditorBase(&aEditorBase),
mContentToDelete(&aContentToDelete),
mParentNode(aContentToDelete.GetParentNode()) {}
mParentNode(aContentToDelete.GetParentNode()) {
MOZ_DIAGNOSTIC_ASSERT_IF(aEditorBase.IsHTMLEditor(),
HTMLEditUtils::IsRemovableNode(aContentToDelete));
}
std::ostream& operator<<(std::ostream& aStream,
const DeleteNodeTransaction& aTransaction) {

2
editor/libeditor/HTMLEditSubActionHandler.cpp
View File

@ -892,7 +892,7 @@ nsresult HTMLEditor::EnsureNoPaddingBRElementForEmptyEditor() {
return NS_ERROR_EDITOR_DESTROYED;
}
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
"EditorBase::DeleteNodeWithTransaction() failed");
"HTMLEditor::DeleteNodeWithTransaction() failed");
return rv;
}

29
editor/libeditor/HTMLEditUtils.h
View File

@ -80,22 +80,37 @@ class HTMLEditUtils final {
/*
* IsRemovalNode() returns true when parent of aContent is editable even
* if aContent isn't editable.
* This is a valid method to check it if you find the content from point
* of view of siblings or parents of aContent.
* Note that padding `<br>` element for empty editor and manual native
* anonymous content should be deletable even after `HTMLEditor` is destroyed
* because they are owned/managed by `HTMLEditor`.
*/
static bool IsRemovableNode(const nsIContent& aContent) {
return aContent.GetParentNode() && aContent.GetParentNode()->IsEditable() &&
&aContent != aContent.OwnerDoc()->GetBody() &&
&aContent != aContent.OwnerDoc()->GetDocumentElement();
return EditorUtils::IsPaddingBRElementForEmptyEditor(aContent) ||
aContent.IsRootOfNativeAnonymousSubtree() ||
(aContent.GetParentNode() &&
aContent.GetParentNode()->IsEditable() &&
&aContent != aContent.OwnerDoc()->GetBody() &&
&aContent != aContent.OwnerDoc()->GetDocumentElement());
}
/**
* IsRemovableFromParentNode() returns true when aContent is editable, has a
* parent node and the parent node is also editable.
* This is a valid method to check it if you find the content from point
* of view of descendants of aContent.
* Note that padding `<br>` element for empty editor and manual native
* anonymous content should be deletable even after `HTMLEditor` is destroyed
* because they are owned/managed by `HTMLEditor`.
*/
static bool IsRemovableFromParentNode(const nsIContent& aContent) {
return aContent.IsEditable() && aContent.GetParentNode() &&
aContent.GetParentNode()->IsEditable() &&
&aContent != aContent.OwnerDoc()->GetBody() &&
&aContent != aContent.OwnerDoc()->GetDocumentElement();
return EditorUtils::IsPaddingBRElementForEmptyEditor(aContent) ||
aContent.IsRootOfNativeAnonymousSubtree() ||
(aContent.IsEditable() && aContent.GetParentNode() &&
aContent.GetParentNode()->IsEditable() &&
&aContent != aContent.OwnerDoc()->GetBody() &&
&aContent != aContent.OwnerDoc()->GetDocumentElement());
}
/**

52
editor/libeditor/HTMLEditor.cpp
View File

@ -3298,11 +3298,8 @@ nsresult HTMLEditor::RemoveEmptyInclusiveAncestorInlineElements(
nsresult HTMLEditor::DeleteNodeWithTransaction(nsIContent& aContent) {
// Do nothing if the node is read-only.
// XXX This is not a override method of EditorBase's method. This might
// cause not called accidentally. We need to investigate this issue.
if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(aContent) &&
!EditorUtils::IsPaddingBRElementForEmptyEditor(aContent))) {
return NS_ERROR_FAILURE;
if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(aContent))) {
return NS_ERROR_EDITOR_UNEXPECTED_DOM_TREE;
}
nsresult rv = EditorBase::DeleteNodeWithTransaction(aContent);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
@ -3668,9 +3665,9 @@ already_AddRefed<Element> HTMLEditor::InsertContainerWithTransactionInternal(
// Put aNode in the new container, first.
// XXX Perhaps, we should not remove the container if it's not editable.
nsresult rv = EditorBase::DeleteNodeWithTransaction(aContent);
nsresult rv = DeleteNodeWithTransaction(aContent);
if (NS_FAILED(rv)) {
NS_WARNING("EditorBase::DeleteNodeWithTransaction() failed");
NS_WARNING("HTMLEditor::DeleteNodeWithTransaction() failed");
return nullptr;
}
@ -3734,12 +3731,9 @@ already_AddRefed<Element> HTMLEditor::ReplaceContainerWithTransactionInternal(
if (NS_WARN_IF(!child)) {
return nullptr;
}
// HTMLEditor::DeleteNodeWithTransaction() does not move non-editable
// node, but we need to move non-editable nodes too. Therefore, call
// EditorBase's method directly.
nsresult rv = EditorBase::DeleteNodeWithTransaction(*child);
nsresult rv = DeleteNodeWithTransaction(*child);
if (NS_FAILED(rv)) {
NS_WARNING("EditorBase::DeleteNodeWithTransaction() failed");
NS_WARNING("HTMLEditor::DeleteNodeWithTransaction() failed");
return nullptr;
}
@ -3762,10 +3756,9 @@ already_AddRefed<Element> HTMLEditor::ReplaceContainerWithTransactionInternal(
}
// Delete old container.
// XXX Perhaps, we should not remove the container if it's not editable.
rv = EditorBase::DeleteNodeWithTransaction(aOldContainer);
rv = DeleteNodeWithTransaction(aOldContainer);
if (NS_FAILED(rv)) {
NS_WARNING("EditorBase::DeleteNodeWithTransaction() failed");
NS_WARNING("HTMLEditor::DeleteNodeWithTransaction() failed");
return nullptr;
}
@ -3790,12 +3783,9 @@ nsresult HTMLEditor::RemoveContainerWithTransaction(Element& aElement) {
if (NS_WARN_IF(!child)) {
return NS_ERROR_FAILURE;
}
// HTMLEditor::DeleteNodeWithTransaction() does not move non-editable
// node, but we need to move non-editable nodes too. Therefore, call
// EditorBase's method directly.
nsresult rv = EditorBase::DeleteNodeWithTransaction(*child);
nsresult rv = DeleteNodeWithTransaction(*child);
if (NS_FAILED(rv)) {
NS_WARNING("EditorBase::DeleteNodeWithTransaction() failed");
NS_WARNING("HTMLEditor::DeleteNodeWithTransaction() failed");
return rv;
}
@ -3811,10 +3801,9 @@ nsresult HTMLEditor::RemoveContainerWithTransaction(Element& aElement) {
}
}
// XXX Perhaps, we should not remove the container if it's not editable.
nsresult rv = EditorBase::DeleteNodeWithTransaction(aElement);
nsresult rv = DeleteNodeWithTransaction(aElement);
NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
"EditorBase::DeleteNodeWithTransaction() failed");
"HTMLEditor::DeleteNodeWithTransaction() failed");
return rv;
}
@ -4250,6 +4239,12 @@ already_AddRefed<nsIContent> HTMLEditor::SplitNodeWithTransaction(
}
MOZ_ASSERT(aStartOfRightNode.IsSetAndValid());
if (NS_WARN_IF(!HTMLEditUtils::IsSplittableNode(
*aStartOfRightNode.ContainerAsContent()))) {
aError.Throw(NS_ERROR_EDITOR_UNEXPECTED_DOM_TREE);
return nullptr;
}
AutoEditSubActionNotifier startToHandleEditSubAction(
*this, EditSubAction::eSplitNode, nsIEditor::eNext, aError);
if (NS_WARN_IF(aError.ErrorCodeIs(NS_ERROR_EDITOR_DESTROYED))) {
@ -4894,16 +4889,9 @@ nsresult HTMLEditor::MoveNodeWithTransaction(
// Notify our internal selection state listener
AutoMoveNodeSelNotify selNotify(RangeUpdaterRef(), oldPoint, aPointToInsert);
// Hold a reference so aNode doesn't go away when we remove it (bug 772282)
// HTMLEditor::DeleteNodeWithTransaction() does not move non-editable
// node, but we need to move non-editable nodes too. Therefore, call
// EditorBase's method directly.
// XXX Perhaps, this method and DeleteNodeWithTransaction() should take
// new argument for making callers specify whether non-editable nodes
// should be moved or not.
nsresult rv = EditorBase::DeleteNodeWithTransaction(aContent);
nsresult rv = DeleteNodeWithTransaction(aContent);
if (NS_FAILED(rv)) {
NS_WARNING("EditorBase::DeleteNodeWithTransaction() failed");
NS_WARNING("HTMLEditor::DeleteNodeWithTransaction() failed");
return rv;
}

7
editor/libeditor/HTMLEditorDataTransfer.cpp
View File

@ -905,12 +905,17 @@ HTMLEditor::HTMLWithContextInserter::InsertContents(
}
// If the child of current node is not list item nor list element,
// we should remove it from the DOM tree.
else {
else if (HTMLEditUtils::IsRemovableNode(*firstChild)) {
AutoEditorDOMPointChildInvalidator lockOffset(pointToInsert);
IgnoredErrorResult ignoredError;
content->RemoveChild(*firstChild, ignoredError);
NS_WARNING_ASSERTION(!ignoredError.Failed(),
"nsINode::RemoveChild() failed, but ignored");
} else {
NS_WARNING(
"Failed to delete the first child of a list element because the "
"list element non-editable");
break;
}
}
}

21
editor/libeditor/HTMLEditorDeleteHandler.cpp
View File

@ -4843,7 +4843,7 @@ void HTMLEditor::MoveChildrenBetween(nsIContent& aFirstChild,
ErrorResult& aError) {
nsCOMPtr<nsINode> oldContainer = aFirstChild.GetParentNode();
if (NS_WARN_IF(oldContainer != aLastChild.GetParentNode()) ||
NS_WARN_IF(!aPointToInsert.IsSet()) ||
NS_WARN_IF(!aPointToInsert.IsInContentNode()) ||
NS_WARN_IF(!aPointToInsert.CanContainerHaveChildren())) {
aError.Throw(NS_ERROR_INVALID_ARG);
return;
@ -4864,7 +4864,7 @@ void HTMLEditor::MoveChildrenBetween(nsIContent& aFirstChild,
return;
}
nsCOMPtr<nsINode> newContainer = aPointToInsert.GetContainer();
nsCOMPtr<nsIContent> newContainer = aPointToInsert.ContainerAsContent();
nsCOMPtr<nsIContent> nextNode = aPointToInsert.GetChild();
for (size_t i = children.Length(); i > 0; --i) {
nsCOMPtr<nsIContent>& child = children[i - 1];
@ -4873,7 +4873,15 @@ void HTMLEditor::MoveChildrenBetween(nsIContent& aFirstChild,
// touch it.
continue;
}
if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(*child))) {
aError.Throw(NS_ERROR_EDITOR_UNEXPECTED_DOM_TREE);
return;
}
oldContainer->RemoveChild(*child, aError);
if (NS_WARN_IF(Destroyed())) {
aError.Throw(NS_ERROR_EDITOR_DESTROYED);
return;
}
if (aError.Failed()) {
NS_WARNING("nsINode::RemoveChild() failed");
return;
@ -4892,7 +4900,16 @@ void HTMLEditor::MoveChildrenBetween(nsIContent& aFirstChild,
return;
}
}
if (NS_WARN_IF(
!EditorUtils::IsEditableContent(*newContainer, EditorType::HTML))) {
aError.Throw(NS_ERROR_EDITOR_UNEXPECTED_DOM_TREE);
return;
}
newContainer->InsertBefore(*child, nextNode, aError);
if (NS_WARN_IF(Destroyed())) {
aError.Throw(NS_ERROR_EDITOR_DESTROYED);
return;
}
if (aError.Failed()) {
NS_WARNING("nsINode::InsertBefore() failed");
return;

3
editor/libeditor/SplitNodeTransaction.cpp
View File

@ -5,6 +5,7 @@
#include "SplitNodeTransaction.h"
#include "HTMLEditUtils.h"
#include "mozilla/EditorDOMPoint.h" // for RangeBoundary, EditorRawDOMPoint
#include "mozilla/HTMLEditor.h" // for HTMLEditor
#include "mozilla/Logging.h"
@ -40,6 +41,8 @@ SplitNodeTransaction::SplitNodeTransaction(
const EditorDOMPointBase<PT, CT>& aStartOfRightContent)
: mHTMLEditor(&aHTMLEditor), mStartOfRightContent(aStartOfRightContent) {
MOZ_DIAGNOSTIC_ASSERT(aStartOfRightContent.IsInContentNode());
MOZ_DIAGNOSTIC_ASSERT(HTMLEditUtils::IsSplittableNode(
*aStartOfRightContent.ContainerAsContent()));
}
std::ostream& operator<<(std::ostream& aStream,