Bug 221651 – Nullptr crash in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 419814 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000002f339272f WebCore::Node::parentNode() const + 0 (Node.h:835) [inlined]
1   com.apple.WebCore             	0x00000002f339272f WebCore::ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline(WebCore::ReplaceSelectionCommand::InsertedNodes&) + 799 (ReplaceSelectionCommand.cpp:631)
2   com.apple.WebCore             	0x00000002f3397095 WebCore::ReplaceSelectionCommand::doApply() + 10261 (ReplaceSelectionCommand.cpp:1317)
3   com.apple.WebCore             	0x00000002f331d267 WebCore::CompositeEditCommand::apply() + 327 (CompositeEditCommand.cpp:375)
4   com.apple.WebCore             	0x00000002f334ec24 WebCore::Editor::replaceSelectionWithFragment(WebCore::DocumentFragment&, WebCore::Editor::SelectReplacement, WebCore::Editor::SmartReplace, WebCore::Editor::MatchStyle, WebCore::EditAction, WebCore::MailBlockquoteHandling) + 868 (Editor.cpp:685)
5   com.apple.WebCore             	0x00000002f334e7d8 WebCore::Editor::handleTextEvent(WebCore::TextEvent&) + 72 (Editor.cpp:334)
6   com.apple.WebCore             	0x00000002f38006cf WebCore::EventHandler::defaultTextInputEventHandler(WebCore::TextEvent&) + 31 (EventHandler.cpp:4120)
7   com.apple.WebCore             	0x00000002f328abb4 WebCore::callDefaultEventHandlersInBubblingOrder(WebCore::Event&, WebCore::EventPath const&) + 39 (EventDispatcher.cpp:62) [inlined]
8   com.apple.WebCore             	0x00000002f328abb4 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 1684 (EventDispatcher.cpp:203)
9   com.apple.WebCore             	0x00000002f3350ee2 WebCore::Editor::pasteAsFragment(WTF::Ref<WebCore::DocumentFragment, WTF::RawPtrTraits<WebCore::DocumentFragment> >&&, bool, bool, WebCore::MailBlockquoteHandling) + 226 (Editor.cpp:616)
10  com.apple.WebCore             	0x00000002f26db582 WebCore::Editor::pasteWithPasteboard(WebCore::Pasteboard*, WTF::OptionSet<WebCore::Editor::PasteOption>) + 274 (EditorMac.mm:96)
11  com.apple.WebCore             	0x00000002f33579e2 WebCore::Editor::paste(WebCore::Pasteboard&, WebCore::Editor::FromMenuOrKeyBinding) + 354 (Editor.cpp:1464)
12  com.apple.WebCore             	0x00000002f3378355 WebCore::Editor::paste(WebCore::Editor::FromMenuOrKeyBinding) + 19 (Editor.cpp:1450) [inlined]
13  com.apple.WebCore             	0x00000002f3378355 WebCore::executePaste(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 69 (EditorCommand.cpp:903)
14  com.apple.WebCore             	0x00000002f324e2bc WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 76 (Document.cpp:5662)
15  com.apple.WebCore             	0x00000002f2551ab0 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 229 (JSDocument.cpp:5889) [inlined]
16  com.apple.WebCore             	0x00000002f2551ab0 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 402 (JSDOMOperation.h:53) [inlined]


<rdar://problem/74150947>

[Frédéric Wang (:fredw)]

This test hits an ASSERT failure in debug mode:

#0  WTFCrash() () at ../../Source/WTF/wtf/Assertions.cpp:295
#1  0x00007fccabafe8cd in CRASH_WITH_INFO(...) () at DerivedSources/ForwardingHeaders/wtf/Assertions.h:713
#2  0x00007fccb0c85389 in WebCore::CompositeEditCommand::appendBlockPlaceholder(WTF::Ref<WebCore::Element, WTF::RawPtrTraits<WebCore::Element> >&&) (this=0x7fcc934445a0, container=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:1073
#3  0x00007fccaefcaec2 in WebCore::InsertParagraphSeparatorCommand::doApply() (this=0x7fcc934445a0)
    at ../../Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:253
#4  0x00007fccb0c81820 in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) (this=0x7fcc934a3570, command=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:466
#5  0x00007fccb0c81cba in WebCore::CompositeEditCommand::insertParagraphSeparator(bool, bool)
    (this=0x7fcc934a3570, useDefaultParagraphElement=false, pasteBlockqutoeIntoUnquotedArea=false)
    at ../../Source/WebCore/editing/CompositeEditCommand.cpp:507
#6  0x00007fccaefe48e0 in WebCore::ReplaceSelectionCommand::doApply() (this=0x7fcc934a3570)
    at ../../Source/WebCore/editing/ReplaceSelectionCommand.cpp:1112

[Frédéric Wang (:fredw)]

Some preliminary reverse debugging below. This is a nullptr dereference due to a disconnected <b> tag. This tag seems to be initially created in a document fragment and does not have a renderer. It still lacks a renderer when it is inserted later in an orphan <div> container. This <div> does not create any renderer (because of the `display: content` rule of the testcase) and so in debug mode hits the assertion from comment 1.

If one changes the testcase to remove "display: content", then the <b> in the document fragment still does not have a renderer initially but it is later properly inserted in the tree via the <div> container and both of them have a renderer.

At that point I have not checked in more details, but I wonder what a proper fix would be. It seems we would like to use a generated <div style="display: block"> as a container. WDYT?

---
Debugging session

SIGSEGV, Segmentation fault in WebCore::Node::parentNode (this=0x0) at DerivedSources/ForwardingHeaders/WebCore/Node.h:835
(rr) reverse-finish
(rr) reverse-finish
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceSelectionCommand.cpp#631
(rr) p element
$1 = (WebCore::StyledElement *) 0x0
(rr) watch -l element
(rr) rc
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceSelectionCommand.cpp#631
(rr) disable 1
(rr) p node.get()
$2 = (WebCore::Node *) 0x0
(rr) b replaceElementWithSpanPreservingChildrenAndAttributes
(rr) rc
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/CompositeEditCommand.cpp#643
(rr) n
(rr) n
(rr) n
(rr) p commandPtr->spanElement()
$3 = (WebCore::HTMLElement *) 0x0
(rr) b ReplaceNodeWithSpanCommand::doApply
(rr) c
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceNodeWithSpanCommand.cpp#61
(rr) p ((Node*) m_elementToReplace)->isConnected()
$4 = false
(rr) p showTree((Node*) m_elementToReplace)
DIV	0x7fc689af8a60 (renderer (nil))
*	B	0x7fc689af85f0 (renderer (nil))  STYLE=border-style: ridge; float: right; font-weight: inherit; caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: -webkit-standard; font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-tap-highlight-color: rgba(0, 0, 0, 0.4); -webkit-text-stroke-width: 0px; text-decoration: none;
$5 = void
(rr) delete
(rr) reverse-finish
(rr) reverse-finish
(rr) reverse-finish
(rr) reverse-finish
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1317
(rr) watch -l insertedNodes
(rr) rc
(rr) disable
(rr) reverse-finish
(rr) reverse-finish
(rr) reverse-finish
(rr) reverse-finish
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1255
(rr) p insertedNodes.isEmpty()
$6 = true
(rr) refNode
(rr) watch -l refNode
(rr) rc
(rr) disable
(rr) reverse-finish
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1242
(rr) watch -l &fragment
(rr) rc
https://webkit-search.igalia.com/webkit/rev/a1f338a6e6776f3d63b7eb8451aa6b5dc6ddff0e/Source/WebCore/editing/ReplaceSelectionCommand.cpp#1063
(rr) p showTree((Node*) fragment.m_fragment)
*#document-fragment	0x7fc689af84e0 (renderer (nil)) 
	B	0x7fc689af85f0 (renderer (nil))  STYLE=border-style: ridge; float: right; font-weight: inherit; caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: -webkit-standard; font-style: normal; font-variant-caps: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-tap-highlight-color: rgba(0, 0, 0, 0.4); -webkit-text-stroke-width: 0px; text-decoration: none;
$7 = void

[Frédéric Wang (:fredw)]

Created attachment 420830 [details]
(proof-of-concept) Patch

Here is a patch that fixes the crash, as suggested in comment 2. Asking review to know if that would be an appropriate approach and if so, any suggestions for refinements (probably this kind of issue can happen everywhere with display: contents).

[Darin Adler]

Comment on attachment 420830 [details]
(proof-of-concept) Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420830&action=review

> Source/WebCore/editing/Editing.cpp:865
> +        div->setAttribute(styleAttr, "display:block");

A few thoughts:

We would not want to unconditionally add a style attribute to all paragraphs we create. Instead we would add something like this only in a case where the style was needed to override something inherited. The editing machinery has lots of code that does that, adding explicit styles to prevent being adversely affected by the surrounding context, like adding boldness when pasting in a non-bold area to keep the text bold.

(You forgot the return statement, by the way.)

But honestly, not sure this is the best solution to a null dereference. The question is what would be good behavior in those null dereference cases. If actually changing the markup so the paragraphs are truly paragraphs is desired, then I agree that this is one of the best ways to do it.

Also, a solution specific to "display" might not be sufficient. Are the other inherited properties that could make a paragraph not be a paragraph? We have code to do these kinds of operations; not sure exactly where it is off hand, and it’s often very inefficient because it computes style for the surrounding context, often repeatedly.

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #4)
> A few thoughts:

Thanks Darin,

> 
> We would not want to unconditionally add a style attribute to all paragraphs
> we create. Instead we would add something like this only in a case where the
> style was needed to override something inherited. The editing machinery has
> lots of code that does that, adding explicit styles to prevent being
> adversely affected by the surrounding context, like adding boldness when
> pasting in a non-bold area to keep the text bold.

I agree. I definitely want to refine this once we know what would be a "proper" fix.

> (You forgot the return statement, by the way.)

Ah silly me. Just checked and it still works when really returning the div. (and that my incorrect patch still crashes if the test has p {display: contents})

> But honestly, not sure this is the best solution to a null dereference. The
> question is what would be good behavior in those null dereference cases. If
> actually changing the markup so the paragraphs are truly paragraphs is
> desired, then I agree that this is one of the best ways to do it.
> 
> Also, a solution specific to "display" might not be sufficient. Are the
> other inherited properties that could make a paragraph not be a paragraph?
> We have code to do these kinds of operations; not sure exactly where it is
> off hand, and it’s often very inefficient because it computes style for the
> surrounding context, often repeatedly.

As I explained in comment 2 the issue is not about "paragraphs being truly paragraphs" but seems to be that an element with "display: contents" does not generate any renderer. For now I can't think of other CSS properties than can have this effect.

(BTW I also expect this issue with "display: contents" not having a renderer to happen in a lot of places. I just noticed Rob fixed bug 221388 for example.)

[Frédéric Wang (:fredw)]

Created attachment 420933 [details]
Patch

After Darin's feedback here is another version that does not modify the DOM tree and instead just aborts InsertParagraphSeparatorCommand::doApply if the container has display: contents. Probably that makes the behavior weird, but it seems OK to do so for this edge case, at least the method has various places where it just returns. I still suspect there could be more issues in the editing code with display: contents (at least there are definitely more callers of WebCore::CompositeEditCommand::appendBlockPlaceholder that could hit the debug assert from comment 1).

[Frédéric Wang (:fredw)]

Created attachment 421172 [details]
Patch

Update ios expectations.

[Ryosuke Niwa]

Comment on attachment 421172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421172&action=review

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:255
> +        if (parent->hasDisplayContents())
> +            return;

Don't we want to just check parent->renderer() instead?

[Frédéric Wang (:fredw)]

Comment on attachment 421172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421172&action=review

>> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:255
>> +            return;
> 
> Don't we want to just check parent->renderer() instead?

I think so. As I said in comment 6, there are probably more issues of these kind so to be safe I'm going to upload a stricter patch for all the appendBlockPlaceholder calls.

[Frédéric Wang (:fredw)]

Created attachment 421293 [details]
Patch

[EWS]

Committed r273330: <https://commits.webkit.org/r273330>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421293 [details].