Title: [258371] trunk
- Revision
- 258371
- Author
- [email protected]
- Date
- 2020-03-12 16:29:31 -0700 (Thu, 12 Mar 2020)
Log Message
Crash in TextManipulationController::replace
https://bugs.webkit.org/show_bug.cgi?id=209021
Reviewed by Wenson Hsieh.
Source/WebCore:
This patch addresses two issues that can lead to a crash in TextManipulationController::replace.
The biggest issue here is that commonAncestor can be a descendent of insertionPoint's containerNode.
Addressed this issue by computing the first node to remove in the same traveral where commonAncestor
is computed by way of remembering the very first content node (firstContentNode). This also lets us
eliminate the secondary, redundant traversal to discover all the nodes to remove.
In addition, the set of nodes to remove could sometimes contain commonAncestor and its ancestors.
This patch addresses this issue by removing all inclusive ancestors of commonAncestor from nodesToRemove.
* editing/TextManipulationController.cpp:
(WebCore::TextManipulationController::replace):
Tools:
Added a regression test.
* TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
(TextManipulation.CompleteTextManipulationShouldReplaceContentFollowedAfterImageInCSSTable):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (258370 => 258371)
--- trunk/Source/WebCore/ChangeLog 2020-03-12 23:21:47 UTC (rev 258370)
+++ trunk/Source/WebCore/ChangeLog 2020-03-12 23:29:31 UTC (rev 258371)
@@ -1,3 +1,23 @@
+2020-03-12 Ryosuke Niwa <[email protected]>
+
+ Crash in TextManipulationController::replace
+ https://bugs.webkit.org/show_bug.cgi?id=209021
+
+ Reviewed by Wenson Hsieh.
+
+ This patch addresses two issues that can lead to a crash in TextManipulationController::replace.
+
+ The biggest issue here is that commonAncestor can be a descendent of insertionPoint's containerNode.
+ Addressed this issue by computing the first node to remove in the same traveral where commonAncestor
+ is computed by way of remembering the very first content node (firstContentNode). This also lets us
+ eliminate the secondary, redundant traversal to discover all the nodes to remove.
+
+ In addition, the set of nodes to remove could sometimes contain commonAncestor and its ancestors.
+ This patch addresses this issue by removing all inclusive ancestors of commonAncestor from nodesToRemove.
+
+ * editing/TextManipulationController.cpp:
+ (WebCore::TextManipulationController::replace):
+
2020-03-12 Per Arne Vollan <[email protected]>
[macOS] _AXSApplicationAccessibilityEnabled should not be called
Modified: trunk/Source/WebCore/editing/TextManipulationController.cpp (258370 => 258371)
--- trunk/Source/WebCore/editing/TextManipulationController.cpp 2020-03-12 23:21:47 UTC (rev 258370)
+++ trunk/Source/WebCore/editing/TextManipulationController.cpp 2020-03-12 23:29:31 UTC (rev 258371)
@@ -481,10 +481,15 @@
}
RefPtr<Node> commonAncestor;
+ RefPtr<Node> firstContentNode;
ParagraphContentIterator iterator { item.start, item.end };
HashSet<Ref<Node>> excludedNodes;
+ HashSet<Ref<Node>> nodesToRemove;
for (; !iterator.atEnd(); iterator.advance()) {
auto content = iterator.currentContent();
+
+ if (content.node)
+ nodesToRemove.add(*content.node);
if (!content.isReplacedContent && !content.isTextContent)
continue;
@@ -498,33 +503,24 @@
tokenExchangeMap.set(currentToken.identifier, TokenExchangeData { content.node.copyRef(), currentToken.content, currentToken.isExcluded });
++currentTokenIndex;
+
+ if (!firstContentNode)
+ firstContentNode = content.node;
// FIXME: Take care of when currentNode is nullptr.
- if (content.node) {
+ if (RefPtr<Node> parent = content.node ? content.node->parentNode() : nullptr) {
if (!commonAncestor)
- commonAncestor = content.node;
- else if (!content.node->isDescendantOf(commonAncestor.get())) {
- commonAncestor = Range::commonAncestorContainer(commonAncestor.get(), content.node.get());
+ commonAncestor = parent;
+ else if (!parent->isDescendantOf(commonAncestor.get())) {
+ commonAncestor = Range::commonAncestorContainer(commonAncestor.get(), parent.get());
ASSERT(commonAncestor);
}
}
}
- RefPtr<Node> nodeAfterStart = item.start.computeNodeAfterPosition();
- if (!nodeAfterStart)
- nodeAfterStart = item.start.containerNode();
+ for (auto node = commonAncestor; node; node = node->parentNode())
+ nodesToRemove.remove(*node);
- RefPtr<Node> nodeAfterEnd = item.end.computeNodeAfterPosition();
- if (!nodeAfterEnd)
- nodeAfterEnd = NodeTraversal::nextSkippingChildren(*item.end.containerNode());
-
- HashSet<Ref<Node>> nodesToRemove;
- for (RefPtr<Node> currentNode = nodeAfterStart; currentNode && currentNode != nodeAfterEnd; currentNode = NodeTraversal::next(*currentNode)) {
- if (commonAncestor == currentNode)
- commonAncestor = currentNode->parentNode();
- nodesToRemove.add(*currentNode);
- }
-
Vector<Ref<Node>> currentElementStack;
HashSet<Ref<Node>> reusedOriginalNodes;
Vector<NodeInsertion> insertions;
@@ -581,9 +577,7 @@
}
}
- Position insertionPoint = item.start;
- if (insertionPoint.anchorNode() != insertionPoint.containerNode())
- insertionPoint = insertionPoint.parentAnchoredEquivalent();
+ Position insertionPoint = positionBeforeNode(firstContentNode.get()).parentAnchoredEquivalent();
while (insertionPoint.containerNode() != commonAncestor)
insertionPoint = positionInParentBeforeNode(insertionPoint.containerNode());
ASSERT(!insertionPoint.isNull());
Modified: trunk/Tools/ChangeLog (258370 => 258371)
--- trunk/Tools/ChangeLog 2020-03-12 23:21:47 UTC (rev 258370)
+++ trunk/Tools/ChangeLog 2020-03-12 23:29:31 UTC (rev 258371)
@@ -1,3 +1,15 @@
+2020-03-12 Ryosuke Niwa <[email protected]>
+
+ Crash in TextManipulationController::replace
+ https://bugs.webkit.org/show_bug.cgi?id=209021
+
+ Reviewed by Wenson Hsieh.
+
+ Added a regression test.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:
+ (TextManipulation.CompleteTextManipulationShouldReplaceContentFollowedAfterImageInCSSTable):
+
2020-03-12 Per Arne Vollan <[email protected]>
[macOS] _AXSApplicationAccessibilityEnabled should not be called
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm (258370 => 258371)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-03-12 23:21:47 UTC (rev 258370)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-03-12 23:29:31 UTC (rev 258371)
@@ -906,6 +906,42 @@
"<img src="" alt=\"Apple\"></div></body>", [webView stringByEvaluatingJavaScript:@"document.documentElement.innerHTML"]);
}
+TEST(TextManipulation, CompleteTextManipulationShouldReplaceContentFollowedAfterImageInCSSTable)
+{
+ auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
+ [webView _setTextManipulationDelegate:delegate.get()];
+
+ [webView synchronouslyLoadHTMLString:@"<!DOCTYPE html><html><body>"
+ "<div style=\"display: table\"><div style=\"float: left;\"><img src="" style=\"display: flex;\"></div>"
+ "<div><span style=\"display: block\">hello world</span></div></body></html>"];
+
+ done = false;
+ [webView _startTextManipulationsWithConfiguration:nil completion:^{
+ done = true;
+ }];
+ TestWebKitAPI::Util::run(&done);
+
+ auto *items = [delegate items];
+ EXPECT_EQ(items.count, 2UL);
+ EXPECT_EQ(items[1].tokens.count, 1UL);
+ EXPECT_STREQ("[]", items[0].tokens[0].content.UTF8String);
+ EXPECT_EQ(items[0].tokens.count, 1UL);
+ EXPECT_STREQ("hello world", items[1].tokens[0].content.UTF8String);
+
+ done = false;
+ [webView _completeTextManipulationForItems:@[
+ (_WKTextManipulationItem *)createItem(items[0].identifier, { { items[0].tokens[0].identifier, nil } }),
+ (_WKTextManipulationItem *)createItem(items[1].identifier, { { items[1].tokens[0].identifier, @"hello, world" } }),
+ ] completion:^(NSArray<NSError *> *errors) {
+ EXPECT_EQ(errors, nil);
+ done = true;
+ }];
+ TestWebKitAPI::Util::run(&done);
+ EXPECT_WK_STREQ("<div style=\"display: table\"><div style=\"float: left;\"><img src="" style=\"display: flex;\"></div>"
+ "<div><span style=\"display: block\">hello, world</span></div></div>", [webView stringByEvaluatingJavaScript:@"document.body.innerHTML"]);
+}
+
TEST(TextManipulation, CompleteTextManipulationShouldBatchItemCallback)
{
auto delegate = adoptNS([[TextManipulationDelegate alloc] init]);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
