- Revision
- 258378
- Author
- [email protected]
- Date
- 2020-03-12 17:51:46 -0700 (Thu, 12 Mar 2020)
Log Message
Cherry-pick r258371. rdar://problem/60395490
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):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@258371 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-610.1.7-branch/Source/WebCore/ChangeLog (258377 => 258378)
--- branches/safari-610.1.7-branch/Source/WebCore/ChangeLog 2020-03-13 00:51:42 UTC (rev 258377)
+++ branches/safari-610.1.7-branch/Source/WebCore/ChangeLog 2020-03-13 00:51:46 UTC (rev 258378)
@@ -1,5 +1,59 @@
2020-03-12 Russell Epstein <[email protected]>
+ Cherry-pick r258371. rdar://problem/60395490
+
+ 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):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@258371 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 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 Russell Epstein <[email protected]>
+
Revert r258353. rdar://problem/60395490
2020-03-12 Russell Epstein <[email protected]>
Modified: branches/safari-610.1.7-branch/Source/WebCore/editing/TextManipulationController.cpp (258377 => 258378)
--- branches/safari-610.1.7-branch/Source/WebCore/editing/TextManipulationController.cpp 2020-03-13 00:51:42 UTC (rev 258377)
+++ branches/safari-610.1.7-branch/Source/WebCore/editing/TextManipulationController.cpp 2020-03-13 00:51:46 UTC (rev 258378)
@@ -480,10 +480,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;
@@ -497,33 +502,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;
@@ -580,9 +576,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: branches/safari-610.1.7-branch/Tools/ChangeLog (258377 => 258378)
--- branches/safari-610.1.7-branch/Tools/ChangeLog 2020-03-13 00:51:42 UTC (rev 258377)
+++ branches/safari-610.1.7-branch/Tools/ChangeLog 2020-03-13 00:51:46 UTC (rev 258378)
@@ -1,5 +1,51 @@
2020-03-12 Russell Epstein <[email protected]>
+ Cherry-pick r258371. rdar://problem/60395490
+
+ 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):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@258371 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 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 Russell Epstein <[email protected]>
+
Revert r258353. rdar://problem/60395490
2020-03-12 Russell Epstein <[email protected]>
Modified: branches/safari-610.1.7-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm (258377 => 258378)
--- branches/safari-610.1.7-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-03-13 00:51:42 UTC (rev 258377)
+++ branches/safari-610.1.7-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm 2020-03-13 00:51:46 UTC (rev 258378)
@@ -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]);
