- Revision
- 266248
- Author
- wenson_hs...@apple.com
- Date
- 2020-08-27 11:56:38 -0700 (Thu, 27 Aug 2020)
Log Message
Occasional crashes when restoring replaced text under Editor::changeBackToReplacedString
https://bugs.webkit.org/show_bug.cgi?id=215892
<rdar://problem/67731156>
Reviewed by Tim Horton.
Source/WebCore:
While reverting the replaced string in `Editor::changeBackToReplacedString`, it's possible for text replacement
(`Editor::replaceSelectionWithText`) to cause the text checking paragraph range to become orphaned (if for no
reason other than the fact that arbitrary script can run in between). If this happens, then our attempts to
expand the checking range to paragraph boundaries underneath `TextCheckingParagraph::paragraphRange` will result
in a null dereference, since the boundary points computed underneath the `expandToParagraphBoundary` helper will
be `nullopt`.
Mitigate this (and any other potentially similar crashes) by making `expandToParagraphBoundary` robust in the
case where expanding to the start and end fails, and just fall back to returning the original text checking
range instead.
Test: editing/mac/input/change-back-to-replaced-string.html
* editing/Editor.h:
* editing/TextCheckingHelper.cpp:
(WebCore::expandToParagraphBoundary):
See above for more details.
* testing/Internals.cpp:
(WebCore::Internals::changeBackToReplacedString):
* testing/Internals.h:
* testing/Internals.idl:
Add a simple internal testing hook to change the currently selected text back to the given replaced string.
LayoutTests:
Add a layout test to exercise the crash.
* editing/mac/input/change-back-to-replaced-string-expected.txt: Added.
* editing/mac/input/change-back-to-replaced-string.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (266247 => 266248)
--- trunk/LayoutTests/ChangeLog 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/LayoutTests/ChangeLog 2020-08-27 18:56:38 UTC (rev 266248)
@@ -1,3 +1,16 @@
+2020-08-27 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Occasional crashes when restoring replaced text under Editor::changeBackToReplacedString
+ https://bugs.webkit.org/show_bug.cgi?id=215892
+ <rdar://problem/67731156>
+
+ Reviewed by Tim Horton.
+
+ Add a layout test to exercise the crash.
+
+ * editing/mac/input/change-back-to-replaced-string-expected.txt: Added.
+ * editing/mac/input/change-back-to-replaced-string.html: Added.
+
2020-08-27 Antti Koivisto <an...@apple.com>
:host() pseudo-selector reported as :host in CSSStyleRule
Added: trunk/LayoutTests/editing/mac/input/change-back-to-replaced-string-expected.txt (0 => 266248)
--- trunk/LayoutTests/editing/mac/input/change-back-to-replaced-string-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/mac/input/change-back-to-replaced-string-expected.txt 2020-08-27 18:56:38 UTC (rev 266248)
@@ -0,0 +1,11 @@
+The test passes if we avoid crashing when removing the editable root while restoring replaced text. This test requires WebKitTestRunner or DumpRenderTree.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Removed editor.
+PASS Changed back to original text without crashing.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/editing/mac/input/change-back-to-replaced-string.html (0 => 266248)
--- trunk/LayoutTests/editing/mac/input/change-back-to-replaced-string.html (rev 0)
+++ trunk/LayoutTests/editing/mac/input/change-back-to-replaced-string.html 2020-08-27 18:56:38 UTC (rev 266248)
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <script src=""
+ <script src=""
+</head>
+<body>
+ <div id="editor" contenteditable></div>
+ <div id="console"></div>
+ <script>
+ internals.settings.setUnifiedTextCheckerEnabled(true);
+ description("The test passes if we avoid crashing when removing the editable root while restoring replaced text. This test requires WebKitTestRunner or DumpRenderTree.");
+
+ let editor = document.getElementById("editor");
+ editor.focus();
+
+ for (let character of "hello ")
+ typeCharacterCommand(character);
+
+ let textNode = editor.childNodes[0];
+ getSelection().setBaseAndExtent(textNode, 0, textNode, 5);
+
+ editor.addEventListener("input", event => {
+ editor.remove();
+ testPassed("Removed editor.");
+ });
+
+ internals.changeBackToReplacedString("helo");
+ testPassed("Changed back to original text without crashing.");
+ </script>
+</body>
+</html>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (266247 => 266248)
--- trunk/Source/WebCore/ChangeLog 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/Source/WebCore/ChangeLog 2020-08-27 18:56:38 UTC (rev 266248)
@@ -1,3 +1,37 @@
+2020-08-27 Wenson Hsieh <wenson_hs...@apple.com>
+
+ Occasional crashes when restoring replaced text under Editor::changeBackToReplacedString
+ https://bugs.webkit.org/show_bug.cgi?id=215892
+ <rdar://problem/67731156>
+
+ Reviewed by Tim Horton.
+
+ While reverting the replaced string in `Editor::changeBackToReplacedString`, it's possible for text replacement
+ (`Editor::replaceSelectionWithText`) to cause the text checking paragraph range to become orphaned (if for no
+ reason other than the fact that arbitrary script can run in between). If this happens, then our attempts to
+ expand the checking range to paragraph boundaries underneath `TextCheckingParagraph::paragraphRange` will result
+ in a null dereference, since the boundary points computed underneath the `expandToParagraphBoundary` helper will
+ be `nullopt`.
+
+ Mitigate this (and any other potentially similar crashes) by making `expandToParagraphBoundary` robust in the
+ case where expanding to the start and end fails, and just fall back to returning the original text checking
+ range instead.
+
+ Test: editing/mac/input/change-back-to-replaced-string.html
+
+ * editing/Editor.h:
+ * editing/TextCheckingHelper.cpp:
+ (WebCore::expandToParagraphBoundary):
+
+ See above for more details.
+
+ * testing/Internals.cpp:
+ (WebCore::Internals::changeBackToReplacedString):
+ * testing/Internals.h:
+ * testing/Internals.idl:
+
+ Add a simple internal testing hook to change the currently selected text back to the given replaced string.
+
2020-08-27 Antti Koivisto <an...@apple.com>
:host() pseudo-selector reported as :host in CSSStyleRule
Modified: trunk/Source/WebCore/editing/Editor.h (266247 => 266248)
--- trunk/Source/WebCore/editing/Editor.h 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/Source/WebCore/editing/Editor.h 2020-08-27 18:56:38 UTC (rev 266248)
@@ -336,7 +336,7 @@
#if PLATFORM(IOS_FAMILY)
NO_RETURN_DUE_TO_ASSERT
#endif
- void changeBackToReplacedString(const String& replacedString);
+ WEBCORE_EXPORT void changeBackToReplacedString(const String& replacedString);
#if !PLATFORM(IOS_FAMILY)
WEBCORE_EXPORT void advanceToNextMisspelling(bool startBeforeSelection = false);
Modified: trunk/Source/WebCore/editing/TextCheckingHelper.cpp (266247 => 266248)
--- trunk/Source/WebCore/editing/TextCheckingHelper.cpp 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/Source/WebCore/editing/TextCheckingHelper.cpp 2020-08-27 18:56:38 UTC (rev 266248)
@@ -106,10 +106,11 @@
static SimpleRange expandToParagraphBoundary(const SimpleRange& range)
{
- return {
- *makeBoundaryPoint(startOfParagraph(createLegacyEditingPosition(range.start))),
- *makeBoundaryPoint(endOfParagraph(createLegacyEditingPosition(range.end)))
- };
+ auto start = makeBoundaryPoint(startOfParagraph(createLegacyEditingPosition(range.start)));
+ auto end = makeBoundaryPoint(endOfParagraph(createLegacyEditingPosition(range.end)));
+ if (!start || !end)
+ return range;
+ return { *start, *end };
}
TextCheckingParagraph::TextCheckingParagraph(const SimpleRange& range)
Modified: trunk/Source/WebCore/testing/Internals.cpp (266247 => 266248)
--- trunk/Source/WebCore/testing/Internals.cpp 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/Source/WebCore/testing/Internals.cpp 2020-08-27 18:56:38 UTC (rev 266248)
@@ -2556,6 +2556,12 @@
frame->editor().changeSelectionListType();
}
+void Internals::changeBackToReplacedString(const String& replacedString)
+{
+ if (auto frame = makeRefPtr(this->frame()))
+ frame->editor().changeBackToReplacedString(replacedString);
+}
+
bool Internals::isOverwriteModeEnabled()
{
Document* document = contextDocument();
Modified: trunk/Source/WebCore/testing/Internals.h (266247 => 266248)
--- trunk/Source/WebCore/testing/Internals.h 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/Source/WebCore/testing/Internals.h 2020-08-27 18:56:38 UTC (rev 266248)
@@ -370,6 +370,7 @@
void handleAcceptedCandidate(const String& candidate, unsigned location, unsigned length);
void changeSelectionListType();
+ void changeBackToReplacedString(const String& replacedString);
bool isOverwriteModeEnabled();
void toggleOverwriteModeEnabled();
Modified: trunk/Source/WebCore/testing/Internals.idl (266247 => 266248)
--- trunk/Source/WebCore/testing/Internals.idl 2020-08-27 18:06:33 UTC (rev 266247)
+++ trunk/Source/WebCore/testing/Internals.idl 2020-08-27 18:56:38 UTC (rev 266248)
@@ -437,6 +437,7 @@
void handleAcceptedCandidate(DOMString candidate, unsigned long location, unsigned long length);
void changeSelectionListType();
+ void changeBackToReplacedString(DOMString replacedString);
boolean isOverwriteModeEnabled();
void toggleOverwriteModeEnabled();
