- Revision
- 287992
- Author
- wenson_hs...@apple.com
- Date
- 2022-01-13 13:25:03 -0800 (Thu, 13 Jan 2022)
Log Message
Crash in Document::updateStyleIfNeeded() when removing a node containing the drag caret
https://bugs.webkit.org/show_bug.cgi?id=235184
rdar://74845918
Reviewed by Darin Adler.
Source/WebCore:
While performing drag and drop over editable content, DragCaretController handles updating and painting a drag
caret, which indicates where the dragged content will be inserted upon drop. If the node containing this drag
caret is disconnected, `DragCaretController::nodeWillBeRemoved()` resets the drag caret position and issues a
repaint on the renderer responsible for drawing the drag caret. This call to `nodeWillBeRemoved()` occurs in the
middle of node removal, so it's encapsulated by a `ScriptDisallowedScope::InMainThread` scope which causes a
release assertion in WebKit2 if anything tries to trigger layout or style updates.
Currently, if the node being removed would cause the caret position to be removed as well, DragCaretController
calls into `setCaretPosition()` with a null visible position, which then calls into `invalidateCaretRect` with
the current caret position's anchor node. In turn, `invalidateCaretRect` contains logic to issue a repaint on
the anchor node's renderer if the node is editable. However, to check whether the node is editable, we use the
helper function `WebCore::isEditableNode()`, which triggers a style update if needed, only in the case where:
1. A style recalc is needed, and...
2. The document contains an element with the `-webkit-user-modify` CSS property.
As such, dirtying element styles right before removing the drag caret's anchor node from the document while
dispatching a `drag` or `drop` event in an editor with `-webkit-user-modify: read-write;` is sufficient to
trigger the release assertion and cause a crash.
To address this, instead of calling `clear()` inside of `DragCaretController::nodeWillBeRemoved()`, we can
instead directly invalidate the caret rect using the current drag caret anchor (passing in `true` for
`caretRectChanged()` since we already know that the drag caret is being cleared out), and then reset the current
drag caret position and caret rect. This allows us to avoid the `isEditableNode()` check in this scenario when
deciding whether to trigger paint invalidation, which is unnecessary because we already know that the node
containing the caret is being removed, so the renderer is going to be repainted anyways. We also pull this logic
out into a separate helper method, `clearCaretPositionWithoutUpdatingStyle()`, to make it clear that we must
avoid triggering style recalc here.
Test: DragAndDropTests.DoNotCrashWhenRemovingNodeOnDrop
* editing/FrameSelection.cpp:
(WebCore::DragCaretController::nodeWillBeRemoved):
(WebCore::DragCaretController::clearCaretPositionWithoutUpdatingStyle):
* editing/FrameSelection.h:
Tools:
Add a test to exercise the crash using drag and drop in WebKit2, on both iOS and macOS.
* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKit/remove-node-on-drop.html: Added.
* TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:
(TEST):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (287991 => 287992)
--- trunk/Source/WebCore/ChangeLog 2022-01-13 21:12:09 UTC (rev 287991)
+++ trunk/Source/WebCore/ChangeLog 2022-01-13 21:25:03 UTC (rev 287992)
@@ -1,3 +1,47 @@
+2022-01-13 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Crash in Document::updateStyleIfNeeded() when removing a node containing the drag caret
+ https://bugs.webkit.org/show_bug.cgi?id=235184
+ rdar://74845918
+
+ Reviewed by Darin Adler.
+
+ While performing drag and drop over editable content, DragCaretController handles updating and painting a drag
+ caret, which indicates where the dragged content will be inserted upon drop. If the node containing this drag
+ caret is disconnected, `DragCaretController::nodeWillBeRemoved()` resets the drag caret position and issues a
+ repaint on the renderer responsible for drawing the drag caret. This call to `nodeWillBeRemoved()` occurs in the
+ middle of node removal, so it's encapsulated by a `ScriptDisallowedScope::InMainThread` scope which causes a
+ release assertion in WebKit2 if anything tries to trigger layout or style updates.
+
+ Currently, if the node being removed would cause the caret position to be removed as well, DragCaretController
+ calls into `setCaretPosition()` with a null visible position, which then calls into `invalidateCaretRect` with
+ the current caret position's anchor node. In turn, `invalidateCaretRect` contains logic to issue a repaint on
+ the anchor node's renderer if the node is editable. However, to check whether the node is editable, we use the
+ helper function `WebCore::isEditableNode()`, which triggers a style update if needed, only in the case where:
+
+ 1. A style recalc is needed, and...
+ 2. The document contains an element with the `-webkit-user-modify` CSS property.
+
+ As such, dirtying element styles right before removing the drag caret's anchor node from the document while
+ dispatching a `drag` or `drop` event in an editor with `-webkit-user-modify: read-write;` is sufficient to
+ trigger the release assertion and cause a crash.
+
+ To address this, instead of calling `clear()` inside of `DragCaretController::nodeWillBeRemoved()`, we can
+ instead directly invalidate the caret rect using the current drag caret anchor (passing in `true` for
+ `caretRectChanged()` since we already know that the drag caret is being cleared out), and then reset the current
+ drag caret position and caret rect. This allows us to avoid the `isEditableNode()` check in this scenario when
+ deciding whether to trigger paint invalidation, which is unnecessary because we already know that the node
+ containing the caret is being removed, so the renderer is going to be repainted anyways. We also pull this logic
+ out into a separate helper method, `clearCaretPositionWithoutUpdatingStyle()`, to make it clear that we must
+ avoid triggering style recalc here.
+
+ Test: DragAndDropTests.DoNotCrashWhenRemovingNodeOnDrop
+
+ * editing/FrameSelection.cpp:
+ (WebCore::DragCaretController::nodeWillBeRemoved):
+ (WebCore::DragCaretController::clearCaretPositionWithoutUpdatingStyle):
+ * editing/FrameSelection.h:
+
2022-01-13 Antoine Quint <grao...@webkit.org>
Remove use of PseudoElement in ComputedStyleExtractor
Modified: trunk/Source/WebCore/editing/FrameSelection.cpp (287991 => 287992)
--- trunk/Source/WebCore/editing/FrameSelection.cpp 2022-01-13 21:12:09 UTC (rev 287991)
+++ trunk/Source/WebCore/editing/FrameSelection.cpp 2022-01-13 21:25:03 UTC (rev 287992)
@@ -536,9 +536,19 @@
if (RenderView* view = node.document().renderView())
view->selection().clear();
- clear();
+ // It's important to avoid updating style or layout here, since we're in the middle of removing the node from the document.
+ clearCaretPositionWithoutUpdatingStyle();
}
+void DragCaretController::clearCaretPositionWithoutUpdatingStyle()
+{
+ if (RefPtr node = m_position.deepEquivalent().anchorNode())
+ invalidateCaretRect(node.get(), true);
+
+ m_position = { };
+ clearCaretRect();
+}
+
void FrameSelection::nodeWillBeRemoved(Node& node)
{
// There can't be a selection inside a fragment, so if a fragment's node is being removed,
Modified: trunk/Source/WebCore/editing/FrameSelection.h (287991 => 287992)
--- trunk/Source/WebCore/editing/FrameSelection.h 2022-01-13 21:12:09 UTC (rev 287991)
+++ trunk/Source/WebCore/editing/FrameSelection.h 2022-01-13 21:25:03 UTC (rev 287992)
@@ -105,6 +105,8 @@
void nodeWillBeRemoved(Node&);
private:
+ void clearCaretPositionWithoutUpdatingStyle();
+
VisiblePosition m_position;
};
Modified: trunk/Tools/ChangeLog (287991 => 287992)
--- trunk/Tools/ChangeLog 2022-01-13 21:12:09 UTC (rev 287991)
+++ trunk/Tools/ChangeLog 2022-01-13 21:25:03 UTC (rev 287992)
@@ -1,3 +1,18 @@
+2022-01-13 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Crash in Document::updateStyleIfNeeded() when removing a node containing the drag caret
+ https://bugs.webkit.org/show_bug.cgi?id=235184
+ rdar://74845918
+
+ Reviewed by Darin Adler.
+
+ Add a test to exercise the crash using drag and drop in WebKit2, on both iOS and macOS.
+
+ * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+ * TestWebKitAPI/Tests/WebKit/remove-node-on-drop.html: Added.
+ * TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:
+ (TEST):
+
2022-01-13 Ross Kirsling <ross.kirsl...@sony.com>
PlayStation MiniBrowser should accept a command-line URL argument
Modified: trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (287991 => 287992)
--- trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2022-01-13 21:12:09 UTC (rev 287991)
+++ trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2022-01-13 21:25:03 UTC (rev 287992)
@@ -1036,6 +1036,7 @@
F43E3BC120DADBC500A4E7ED /* fixed-nav-bar.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F43E3BC020DADB8000A4E7ED /* fixed-nav-bar.html */; };
F442851D2140DF2900CCDA22 /* NSFontPanelTesting.mm in Sources */ = {isa = PBXBuildFile; fileRef = F442851C2140DF2900CCDA22 /* NSFontPanelTesting.mm */; };
F4451C761EB8FD890020C5DA /* two-paragraph-contenteditable.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4451C751EB8FD7C0020C5DA /* two-paragraph-contenteditable.html */; };
+ F4476F4B279082F900931786 /* remove-node-on-drop.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4476F4A279082EB00931786 /* remove-node-on-drop.html */; };
F44A531121B8990300DBB99C /* InstanceMethodSwizzler.mm in Sources */ = {isa = PBXBuildFile; fileRef = F44A531021B8976900DBB99C /* InstanceMethodSwizzler.mm */; };
F44A9AF72649BBDD00E7CB16 /* ImmediateActionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = F44A9AF62649BBDD00E7CB16 /* ImmediateActionTests.mm */; };
F44C7A0020F9EEBF0014478C /* ParserYieldTokenPlugIn.mm in Sources */ = {isa = PBXBuildFile; fileRef = F44C79FB20F9E50C0014478C /* ParserYieldTokenPlugIn.mm */; };
@@ -1525,6 +1526,7 @@
F41AB9A81EF4696B0083FA08 /* prevent-operation.html in Copy Resources */,
F41AB9A91EF4696B0083FA08 /* prevent-start.html in Copy Resources */,
F6FDDDD614241C6F004F1729 /* push-state.html in Copy Resources */,
+ F4476F4B279082F900931786 /* remove-node-on-drop.html in Copy Resources */,
A12DDC001E8373E700CF6CAE /* rendered-image-excluding-overflow.html in Copy Resources */,
498D030A26376B34009CBFAD /* resourceLoadStatisticsMissingUniqueIndex.db in Copy Resources */,
F46849C01EEF5EF300B937FE /* rich-and-plain-text.html in Copy Resources */,
@@ -2993,6 +2995,7 @@
F442851B2140DF2900CCDA22 /* NSFontPanelTesting.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = NSFontPanelTesting.h; sourceTree = "<group>"; };
F442851C2140DF2900CCDA22 /* NSFontPanelTesting.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = NSFontPanelTesting.mm; sourceTree = "<group>"; };
F4451C751EB8FD7C0020C5DA /* two-paragraph-contenteditable.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "two-paragraph-contenteditable.html"; sourceTree = "<group>"; };
+ F4476F4A279082EB00931786 /* remove-node-on-drop.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "remove-node-on-drop.html"; sourceTree = "<group>"; };
F44A530D21B8976900DBB99C /* InstanceMethodSwizzler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = InstanceMethodSwizzler.h; path = ../TestRunnerShared/cocoa/InstanceMethodSwizzler.h; sourceTree = "<group>"; };
F44A530E21B8976900DBB99C /* ClassMethodSwizzler.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = ClassMethodSwizzler.mm; path = ../TestRunnerShared/cocoa/ClassMethodSwizzler.mm; sourceTree = "<group>"; };
F44A530F21B8976900DBB99C /* ClassMethodSwizzler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = ClassMethodSwizzler.h; path = ../TestRunnerShared/cocoa/ClassMethodSwizzler.h; sourceTree = "<group>"; };
@@ -4701,6 +4704,7 @@
83148B08202AC76800BADE99 /* override-builtins-test.html */,
0EBBCC651FFF9DCE00FA42AB /* pop-up-check.html */,
F6FDDDD514241C48004F1729 /* push-state.html */,
+ F4476F4A279082EB00931786 /* remove-node-on-drop.html */,
0F5651F81FCE50E800310FBC /* scroll-to-anchor.html */,
7A66BDB71EAF150100CCC924 /* set-long-title.html */,
CEBABD481B71687C0051210A /* should-open-external-schemes.html */,
Added: trunk/Tools/TestWebKitAPI/Tests/WebKit/remove-node-on-drop.html (0 => 287992)
--- trunk/Tools/TestWebKitAPI/Tests/WebKit/remove-node-on-drop.html (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit/remove-node-on-drop.html 2022-01-13 21:25:03 UTC (rev 287992)
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf8">
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<style>
+body {
+ font-family: system-ui;
+ -webkit-user-modify: read-write;
+ margin: 0;
+}
+
+div {
+ width: 300px;
+ height: 100px;
+}
+
+div.drag {
+ -webkit-user-select: all;
+ border: 1px solid green;
+ font-size: 50px;
+ line-height: 100px;
+ text-align: center;
+}
+
+div.drop {
+ border: 1px solid tomato;
+}
+</style>
+<script>
+addEventListener("load", () => {
+ document.querySelector(".drop").addEventListener("drop", event => {
+ document.body.style.webkitUserModify = "read-write-plaintext-only";
+ document.querySelector("br").remove();
+ });
+ getSelection().selectAllChildren(document.querySelector(".drag"));
+});
+</script>
+</head>
+<body>
+ <div class="drag" draggable="true">Drag me</div>
+ <div class="drop"><br></div>
+ <p>To run the test manually, drag and drop the selected text into the red box.</p>
+</body>
+</html>
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm (287991 => 287992)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm 2022-01-13 21:12:09 UTC (rev 287991)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm 2022-01-13 21:25:03 UTC (rev 287992)
@@ -311,6 +311,15 @@
EXPECT_FALSE(result.containsFile);
}
+TEST(DragAndDropTests, DoNotCrashWhenRemovingNodeOnDrop)
+{
+ auto simulator = adoptNS([[DragAndDropSimulator alloc] initWithWebViewFrame:CGRectMake(0, 0, 320, 500)]);
+ auto webView = [simulator webView];
+ [webView synchronouslyLoadTestPageNamed:@"remove-node-on-drop"];
+ [simulator runFrom:CGPointMake(150, 50) to:CGPointMake(150, 150)];
+ EXPECT_TRUE([[webView contentsAsString] containsString:@"Drag me"]);
+}
+
#if ENABLE(INPUT_TYPE_COLOR)
TEST(DragAndDropTests, ColorInputToColorInput)
