- Revision
- 273330
- Author
- [email protected]
- Date
- 2021-02-23 12:19:23 -0800 (Tue, 23 Feb 2021)
Log Message
Nullptr crash in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
https://bugs.webkit.org/show_bug.cgi?id=221651
Patch by Frederic Wang <[email protected]> on 2021-02-23
Reviewed by Ryosuke Niwa.
Source/WebCore:
There are assumptions in the editing code that new containers have an associated renderer,
which is not the case if their style is "display: contents". This patch replaces the
strict debug ASSERT in CompositeEditCommand::appendBlockPlaceholder with a failure
specified by a nullptr return value, similar to other BlockPlaceholder APIs. That way
InsertParagraphSeparatorCommand::doApply can exit early when the block placeholder cannot
be inserted. This patch also fixes one nullptr dereference in release build happening when
executing Editor::replaceSelectionWithFragment.
Test: fast/editing/mutation-with-display-contents-container-crash.html
* editing/CompositeEditCommand.cpp:
(WebCore::CompositeEditCommand::appendBlockPlaceholder): Return null is the container
has no associated renderer.
* editing/CompositeEditCommand.h: Return a RefPtr, so it is null in case of failure.
* editing/InsertParagraphSeparatorCommand.cpp:
(WebCore::InsertParagraphSeparatorCommand::doApply): Abort command if the block placeholder
cannot be inserted in the container.
LayoutTests:
* fast/editing/mutation-with-display-contents-container-crash-expected.txt: Added.
* fast/editing/mutation-with-display-contents-container-crash.html: Added.
* platform/ios/fast/editing/mutation-with-display-contents-container-crash-expected.txt: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (273329 => 273330)
--- trunk/LayoutTests/ChangeLog 2021-02-23 20:09:56 UTC (rev 273329)
+++ trunk/LayoutTests/ChangeLog 2021-02-23 20:19:23 UTC (rev 273330)
@@ -1,3 +1,14 @@
+2021-02-23 Frederic Wang <[email protected]>
+
+ Nullptr crash in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
+ https://bugs.webkit.org/show_bug.cgi?id=221651
+
+ Reviewed by Ryosuke Niwa.
+
+ * fast/editing/mutation-with-display-contents-container-crash-expected.txt: Added.
+ * fast/editing/mutation-with-display-contents-container-crash.html: Added.
+ * platform/ios/fast/editing/mutation-with-display-contents-container-crash-expected.txt: Added.
+
2021-02-23 Chris Fleizach <[email protected]>
AX: VoiceOver incorrectly announces groups in ARIA tree instances as empty
Added: trunk/LayoutTests/fast/editing/mutation-with-display-contents-container-crash-expected.txt (0 => 273330)
--- trunk/LayoutTests/fast/editing/mutation-with-display-contents-container-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/editing/mutation-with-display-contents-container-crash-expected.txt 2021-02-23 20:19:23 UTC (rev 273330)
@@ -0,0 +1 @@
+This test passes if it does not crash.
Added: trunk/LayoutTests/fast/editing/mutation-with-display-contents-container-crash.html (0 => 273330)
--- trunk/LayoutTests/fast/editing/mutation-with-display-contents-container-crash.html (rev 0)
+++ trunk/LayoutTests/fast/editing/mutation-with-display-contents-container-crash.html 2021-02-23 20:19:23 UTC (rev 273330)
@@ -0,0 +1,25 @@
+<style>
+ div {
+ display: contents;
+ }
+ b, span {
+ border-style: ridge;
+ float: right;
+ }
+ b {
+ font-weight: inherit;
+ }
+</style>
+<div>This test passes if it does not crash.</div>
+<script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+ _onload_ = () => {
+ document.documentElement.appendChild(document.createElement('b'));
+ document.documentElement.appendChild(document.createElement('span'));
+ document.execCommand('SelectAll');
+ document.execCommand('Copy');
+ document.designMode = 'on';
+ document.execCommand('Paste');
+ };
+</script>
Added: trunk/LayoutTests/platform/ios/fast/editing/mutation-with-display-contents-container-crash-expected.txt (0 => 273330)
--- trunk/LayoutTests/platform/ios/fast/editing/mutation-with-display-contents-container-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/ios/fast/editing/mutation-with-display-contents-container-crash-expected.txt 2021-02-23 20:19:23 UTC (rev 273330)
@@ -0,0 +1,2 @@
+This test passes if it does not crash.
+
Modified: trunk/Source/WebCore/ChangeLog (273329 => 273330)
--- trunk/Source/WebCore/ChangeLog 2021-02-23 20:09:56 UTC (rev 273329)
+++ trunk/Source/WebCore/ChangeLog 2021-02-23 20:19:23 UTC (rev 273330)
@@ -1,3 +1,28 @@
+2021-02-23 Frederic Wang <[email protected]>
+
+ Nullptr crash in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
+ https://bugs.webkit.org/show_bug.cgi?id=221651
+
+ Reviewed by Ryosuke Niwa.
+
+ There are assumptions in the editing code that new containers have an associated renderer,
+ which is not the case if their style is "display: contents". This patch replaces the
+ strict debug ASSERT in CompositeEditCommand::appendBlockPlaceholder with a failure
+ specified by a nullptr return value, similar to other BlockPlaceholder APIs. That way
+ InsertParagraphSeparatorCommand::doApply can exit early when the block placeholder cannot
+ be inserted. This patch also fixes one nullptr dereference in release build happening when
+ executing Editor::replaceSelectionWithFragment.
+
+ Test: fast/editing/mutation-with-display-contents-container-crash.html
+
+ * editing/CompositeEditCommand.cpp:
+ (WebCore::CompositeEditCommand::appendBlockPlaceholder): Return null is the container
+ has no associated renderer.
+ * editing/CompositeEditCommand.h: Return a RefPtr, so it is null in case of failure.
+ * editing/InsertParagraphSeparatorCommand.cpp:
+ (WebCore::InsertParagraphSeparatorCommand::doApply): Abort command if the block placeholder
+ cannot be inserted in the container.
+
2021-02-23 Alicia Boya García <[email protected]>
Make system console logging synchronous
Modified: trunk/Source/WebCore/editing/CompositeEditCommand.cpp (273329 => 273330)
--- trunk/Source/WebCore/editing/CompositeEditCommand.cpp 2021-02-23 20:09:56 UTC (rev 273329)
+++ trunk/Source/WebCore/editing/CompositeEditCommand.cpp 2021-02-23 20:19:23 UTC (rev 273330)
@@ -1065,12 +1065,13 @@
deleteInsignificantText(position, VisiblePosition(position).next().deepEquivalent().downstream());
}
-Ref<Element> CompositeEditCommand::appendBlockPlaceholder(Ref<Element>&& container)
+RefPtr<Element> CompositeEditCommand::appendBlockPlaceholder(Ref<Element>&& container)
{
document().updateLayoutIgnorePendingStylesheets();
- // Should assert isBlockFlow || isInlineFlow when deletion improves. See 4244964.
- ASSERT(container->renderer());
+ // Should check isBlockFlow || isInlineFlow when deletion improves. See 4244964.
+ if (!container->renderer())
+ return nullptr;
auto placeholder = createBlockPlaceholderElement(document());
appendNode(placeholder.copyRef(), WTFMove(container));
Modified: trunk/Source/WebCore/editing/CompositeEditCommand.h (273329 => 273330)
--- trunk/Source/WebCore/editing/CompositeEditCommand.h 2021-02-23 20:09:56 UTC (rev 273329)
+++ trunk/Source/WebCore/editing/CompositeEditCommand.h 2021-02-23 20:19:23 UTC (rev 273330)
@@ -187,7 +187,7 @@
void deleteInsignificantText(const Position& start, const Position& end);
void deleteInsignificantTextDownstream(const Position&);
- Ref<Element> appendBlockPlaceholder(Ref<Element>&&);
+ RefPtr<Element> appendBlockPlaceholder(Ref<Element>&&);
RefPtr<Node> insertBlockPlaceholder(const Position&);
RefPtr<Node> addBlockPlaceholderIfNeeded(Element*);
void removePlaceholderAt(const Position&);
Modified: trunk/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp (273329 => 273330)
--- trunk/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp 2021-02-23 20:09:56 UTC (rev 273329)
+++ trunk/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp 2021-02-23 20:19:23 UTC (rev 273330)
@@ -224,7 +224,8 @@
// represent the paragraph that we're leaving.
auto extraBlock = createDefaultParagraphElement(document());
appendNode(extraBlock.copyRef(), *startBlock);
- appendBlockPlaceholder(WTFMove(extraBlock));
+ if (!appendBlockPlaceholder(WTFMove(extraBlock)))
+ return;
}
appendNode(*blockToInsert, *startBlock);
} else {
@@ -250,7 +251,8 @@
auto parent = cloneHierarchyUnderNewBlock(ancestors, *blockToInsert);
auto* parentPtr = parent.ptr();
- appendBlockPlaceholder(WTFMove(parent));
+ if (!appendBlockPlaceholder(WTFMove(parent)))
+ return;
setEndingSelection(VisibleSelection(firstPositionInNode(parentPtr), Affinity::Downstream, endingSelection().isDirectional()));
return;
@@ -286,9 +288,11 @@
Vector<RefPtr<Element>> ancestors;
getAncestorsInsideBlock(positionAvoidingSpecialElementBoundary(positionOutsideTabSpan(insertionPosition)).deprecatedNode(), startBlock.get(), ancestors);
+
+ auto parent = cloneHierarchyUnderNewBlock(ancestors, *blockToInsert);
+ if (!appendBlockPlaceholder(WTFMove(parent)))
+ return;
- appendBlockPlaceholder(cloneHierarchyUnderNewBlock(ancestors, *blockToInsert));
-
// In this case, we need to set the new ending selection.
setEndingSelection(VisibleSelection(insertionPosition, Affinity::Downstream, endingSelection().isDirectional()));
return;
