Bug 211206

Summary:

Nullptr crash in CompositeEditCommand::moveParagraphs when changing style on elements that are user-select:none

Product:

WebKit

Reporter:

Jack <shihchieh_lee>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, ews-feeder, ggaren, megan_gardner, product-security, rniwa, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Jack

Reported 2020-04-29 14:20:24 PDT

<rdar://61830589> 0 com.apple.WebCore 0x00000001b14059aa WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 11514 1 com.apple.WebCore 0x00000001b13f27e7 WebCore::ApplyStyleCommand::applyBlockStyle(WebCore::EditingStyle&) + 4423 2 com.apple.WebCore 0x00000001b01b5c5d WebCore::CompositeEditCommand::apply() + 397 3 com.apple.WebCore 0x00000001b142aee1 WebCore::Editor::applyParagraphStyle(WebCore::StyleProperties*, WebCore::EditAction) + 433 4 com.apple.WebCore 0x00000001b145b783 WebCore::executeApplyParagraphStyle(WebCore::Frame&, WebCore::EditorCommandSource, WebCore::EditAction, WebCore::CSSPropertyID, WTF::String const&) + 131 5 com.apple.WebCore 0x00000001b1455726 WebCore::executeJustifyLeft(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 54 6 com.apple.WebCore 0x00000001b021c2b1 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 81 7 com.apple.WebCore 0x00000001b05f21ea WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 426

Attachments
Patch (2.10 KB, patch) 2020-04-30 16:05 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (4.45 KB, patch) 2020-04-30 23:04 PDT, Jack	no flags	Details Formatted Diff Diff
Patch (4.14 KB, patch) 2020-05-01 15:22 PDT, Jack	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jack

Comment 1 2020-04-29 14:52:43 PDT

Test case: <style> q { -webkit-user-select: none; -webkit-border-image: url() } label { direction: rtl; } </style> <body><label contenteditable="true" style="-webkit-appearance: button"><q id=q>a</q> <script> window.getSelection().selectAllChildren(q); document.execCommand("justifyLeft", false); </script> Root cause: 1. In processing “selectAllChildren(q)”, because Q is user-select:none, the selection is moved to #text “\n”. 2. In processing justifyLeft command for element Q, we create and insert DIV and BR. 3. Then we call moveParagraphs to move #text “\n” into firstPositionInNode(DIV). 4. However, in finding firstPositionInNode(DIV), because DIV is also user-select:none, and no other candidate is found, we pass an empty position to moveParagraphs. 5. The null anchorNode in the empty position is dereferenced later in the function. BODY 0x60c000088780 (renderer 0x612000068bc0) LABEL 0x60c000088840 (renderer 0x612000068d40) STYLE=-webkit-appearance: button Q 0x60c000088900 (renderer 0x6110000d7400) #text 0x60800004e420 "a" * DIV 0x60c0000a21c0 (renderer 0x61200006b740) BR 0x60c0000a2280 (renderer 0x6110000dab00) #text 0x60800004e4a0 "\n" SCRIPT 0x610000032740 (renderer 0x0) #text 0x60800004e520 "\n window.getSelection().selectAllChildren(q);\n document.execCommand("justifyLeft", false);\n"

Jack

Comment 2 2020-04-29 15:04:21 PDT

There are two options for the patch: 1. Check for empty position in moveParagraphs and returns early. However, it is a little bit late because at this point we have already inserted DIV and BR. Besides, we would return the newly created DIV element back to the caller. 2. Check the node where DIV is being inserted to see if it is userSelect:None and return early. However, I am not sure if this is equivalent to checking the candidate later after DIV and BR are inserted.

Geoffrey Garen

Comment 3 2020-04-29 15:10:03 PDT

> 1. In processing “selectAllChildren(q)”, because Q is user-select:none, the > selection is moved to #text “\n”. > 2. In processing justifyLeft command for element Q, we create and insert DIV > and BR. It's a little strange that the DIV and BR are inserted into Q, even though Q is not selected. My naive take is that we should have tried to left-justify only the selected "\n" (which is basically a no-op).

Jack

Comment 4 2020-04-29 16:07:38 PDT

Yeah, the code insert DIV to upstreamStart, instead of the selected position (paragraph start). Position upstreamStart = visibleParagraphStart.deepEquivalent().upstream(); ... insertNewDefaultParagraphElementAt(upstreamStart); (In reply to Geoffrey Garen from comment #3) > It's a little strange that the DIV and BR are inserted into Q, even though Q > is not selected. My naive take is that we should have tried to left-justify > only the selected "\n" (which is basically a no-op).

Jack

Comment 5 2020-04-30 16:05:10 PDT

Created attachment 398111 [details] Patch

Jack

Comment 6 2020-04-30 16:22:54 PDT

Please see the patch for two patch options. Geoff also brought up a good point: should upstream() skip the user-select:none elements? If a node is not selectable, does it makes sense to return its position? (In reply to Jack from comment #5) > Created attachment 398111 [details] > Patch

Jack

Comment 7 2020-04-30 23:04:27 PDT

Created attachment 398163 [details] Patch

Jack

Comment 8 2020-04-30 23:10:41 PDT

Function moveParagraphContentsToNewBlockIfNecessary checks if upstreamStart is editable or not. Added check for user-select:none at the same line. (In reply to Jack from comment #7) > Created attachment 398163 [details] > Patch

Geoffrey Garen

Comment 9 2020-05-01 11:57:53 PDT

Comment on attachment 398163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398163&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:1204 > - if (!isEditablePosition(upstreamStart)) > + if (!isEditablePosition(upstreamStart) || Position::nodeIsUserSelectNone(upstreamStart.containerNode())) > return nullptr; I'm curious what happen is you change isEditablePosition() to check nodeIsUserSelectNone(). That seems like a cleaner fix to me, if it passes tests, since we have other code that checks isEditablePosition. I don't love the idea of adding a check for nodeIsUserSelectNone() only in moveParagraphContentsToNewBlockIfNecessary(). That seems like an arbitrary difference from other editing code.

Geoffrey Garen

Comment 10 2020-05-01 12:01:57 PDT

Comment on attachment 398111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398111&action=review > Source/WebCore/editing/CompositeEditCommand.cpp:1399 > + if (destination.isNull() || startOfParagraphToMove == destination) > return; I don't really mind this null check. It seems like this function is already pretty lenient, since it allows a caller to specify startOfParagraphToMove == destination. Still, it seems like a bug for a caller to ask to move to an invalid destination. Surely the caller would have preferred to pick a valid destination, if it had known? I guess I think we should try adding the nodeIsUserSelectNone() check to isEditablePosition(), and see if that passes tests. If it does pass tests, let's go with that. Otherwise, let's add this null check. Mechanically, what causes a user-select: none element to be a null VisiblePosition? Which other piece of code is checking nodeIsUserSelectNone() and concluding that the position is not visible? That might help us understand whether isEditablePosition() should change to match.

Jack

Comment 11 2020-05-01 12:40:52 PDT

(In reply to Geoffrey Garen from comment #10) > Comment on attachment 398111 [details] > Patch > > Mechanically, what causes a user-select: none element to be a null > VisiblePosition? Which other piece of code is checking > nodeIsUserSelectNone() and concluding that the position is not visible? That > might help us understand whether isEditablePosition() should change to match. NodeIsUserSelectNone is only called in isCandidate(), except in EventHandler::updateSelectionForMouseDownDispatchingSelectStart, probably as a patch as well. In isCandidate(), node is checked for editable style by calling hasEditableStyle, which uses the same function computeEditability, as isEditablePosition. However, in function isCandidate(), different nodes can be used to check editable and select styles -- m_anchorNode is always used for editable style, and for select style, deprecatedNode() or its parentNode() can be checked. Therefore, the behavior may be different if we check m_anchorNode for both editable and select styles.

Geoffrey Garen

Comment 12 2020-05-01 13:47:01 PDT

(In reply to Jack from comment #11) > (In reply to Geoffrey Garen from comment #10) > > Comment on attachment 398111 [details] > > Patch > > > > > Mechanically, what causes a user-select: none element to be a null > > VisiblePosition? Which other piece of code is checking > > nodeIsUserSelectNone() and concluding that the position is not visible? That > > might help us understand whether isEditablePosition() should change to match. > > NodeIsUserSelectNone is only called in isCandidate() In this test case, which position ends up being null? Is it upstreamStart? Or firstPositionInNode(newBlock.ptr())? Or something else?

Jack

Comment 13 2020-05-01 13:49:54 PDT

(In reply to Geoffrey Garen from comment #12) > (In reply to Jack from comment #11) > In this test case, which position ends up being null? Is it upstreamStart? > Or firstPositionInNode(newBlock.ptr())? Or something else? firstPositionInNode(newBlock.ptr()) returns empty position. upstreamStart is the #text in Q. newBlock is DIV in Q.

Geoffrey Garen

Comment 14 2020-05-01 14:09:39 PDT

> upstreamStart is the #text in Q. If Position::isCandidate rejects nodes that are user-select: none, and the #text in Q is user-select: none (inherited from Q), how did upstreamStart get set to the #text in Q to begin with? How is it even possible to construct such a Position? What is visibleParagraphStart?

Jack

Comment 15 2020-05-01 14:26:42 PDT

(In reply to Geoffrey Garen from comment #14) > > upstreamStart is the #text in Q. > > If Position::isCandidate rejects nodes that are user-select: none, and the > #text in Q is user-select: none (inherited from Q), how did upstreamStart > get set to the #text in Q to begin with? How is it even possible to > construct such a Position? In function upstream, there is no user-select check, although it does check editability. > What is visibleParagraphStart? It is #text "\n"

Geoffrey Garen

Comment 16 2020-05-01 14:33:48 PDT

> > If Position::isCandidate rejects nodes that are user-select: none, and the > > #text in Q is user-select: none (inherited from Q), how did upstreamStart > > get set to the #text in Q to begin with? How is it even possible to > > construct such a Position? > In function upstream, there is no user-select check, although it does check > editability. I see. I believed, incorrectly, that upstream() called isCandidate(). But it doesn't. Tantalizingly, upstream() does call endsOfNodeAreVisuallyDistinctPositions(), which says this: // FIXME: Share code with isCandidate, if possible. static bool endsOfNodeAreVisuallyDistinctPositions(Node* node) 🤨

Jack

Comment 17 2020-05-01 14:42:04 PDT

(In reply to Geoffrey Garen from comment #16) > // FIXME: Share code with isCandidate, if possible. > static bool endsOfNodeAreVisuallyDistinctPositions(Node* node) > > 🤨 Does it means it is time to FIXME? :-) So should we try checking selectable in endsOfNodeAreVisuallyDistinctPositions?

Geoffrey Garen

Comment 18 2020-05-01 14:45:36 PDT

OK, here's my thinking: I think we should fix this bug by changing CompositeEditCommand::moveParagraphs to return early if destination is null. Because nothing mechanically prevents someone from passing null to moveParagraphs, we might as well check for it, so that a minor bug in an edit operation is just a no-op, rather than a crash. That said, Node::computeEditability() should probably return Editability::ReadOnly if the node is user-select: none. Logically, you can't edit if you can't select. And in our implementation, we seem to expect only to edit what we can select. We don't necessarily have to pursue this change, but if you have some time to test it, and it passes all tests, it might be a neat improvement.

Jack

Comment 19 2020-05-01 14:55:15 PDT

(In reply to Geoffrey Garen from comment #18) > OK, here's my thinking: > > I think we should fix this bug by changing > CompositeEditCommand::moveParagraphs to return early if destination is null. > Because nothing mechanically prevents someone from passing null to > moveParagraphs, we might as well check for it, so that a minor bug in an > edit operation is just a no-op, rather than a crash. Sounds great! Yes, you are right, it's already happening in another radar where DeleteSelectionCommand::mergeParagraphs calls moveParagraphs with empty destination. > That said, Node::computeEditability() should probably return > Editability::ReadOnly if the node is user-select: none. Logically, you can't > edit if you can't select. And in our implementation, we seem to expect only > to edit what we can select. We don't necessarily have to pursue this change, > but if you have some time to test it, and it passes all tests, it might be a > neat improvement. Got it. I will put this on back burner when bug count is low.

Jack

Comment 20 2020-05-01 15:22:32 PDT

Created attachment 398249 [details] Patch

Geoffrey Garen

Comment 21 2020-05-04 11:41:55 PDT

Comment on attachment 398249 [details] Patch r=me

EWS

Comment 22 2020-05-04 16:55:10 PDT

Committed r261126: <https://trac.webkit.org/changeset/261126> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398249 [details].

Note You need to log in before you can comment on or make changes to this bug.