Bug 227390 – Nullptr crash in HTMLStackItem::create via DocumentFragment::parseHTML

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

Debug assertion makes this bug clear:

ASSERTION FAILED: contextElement
./dom/DocumentFragment.cpp(87) : void WebCore::DocumentFragment::parseHTML(const WTF::String &, WebCore::Element *, WebCore::ParserContentPolicy)
1   0x7ba119179 WTFCrash
2   0x79a8ac29b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x79d9dc7e0 WebCore::DocumentFragment::parseHTML(WTF::String const&, WebCore::Element*, WebCore::ParserContentPolicy)
4   0x79b7ee968 WebCore::createFragment(WebCore::Frame&, NSAttributedString*)
5   0x79b7edfba WebCore::createFragmentAndAddResources(WebCore::Frame&, NSAttributedString*)
6   0x79b7f3ccb WebCore::WebContentReader::readRTF(WebCore::SharedBuffer&)
7   0x79c9b0f0f WebCore::Pasteboard::read(WebCore::PasteboardWebContentReader&, WebCore::WebContentReadingPolicy, std::__1::optional<unsigned long>)
8   0x79b663a98 WebCore::Editor::webContentFromPasteboard(WebCore::Pasteboard&, WebCore::SimpleRange const&, bool, bool&)
9   0x79b7f74d6 WebCore::Editor::pasteWithPasteboard(WebCore::Pasteboard*, WTF::OptionSet<WebCore::Editor::PasteOption>)
10  0x79dcbbd4e WebCore::Editor::paste(WebCore::Pasteboard&, WebCore::Editor::FromMenuOrKeyBinding)
11  0x79dcbbbf3 WebCore::Editor::paste(WebCore::Editor::FromMenuOrKeyBinding)
12  0x79dce7463 WebCore::executePaste(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
13  0x79dcbdc6b WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
14  0x79d9ca7eb WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
15  0x79b2aff3b WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)
16  0x79b2af89c 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*)

In release, we crash with the following backtrace:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000011343596a WebCore::Node::ref() const + 0 (Node.h:781) [inlined]
1   com.apple.WebCore             	0x000000011343596a WTF::Ref<WebCore::ContainerNode, WTF::RawPtrTraits<WebCore::ContainerNode> >::Ref(WebCore::ContainerNode&) + 4 (Ref.h:67) [inlined]
2   com.apple.WebCore             	0x000000011343596a WTF::Ref<WebCore::ContainerNode, WTF::RawPtrTraits<WebCore::ContainerNode> >::Ref(WebCore::ContainerNode&) + 4 (Ref.h:66) [inlined]
3   com.apple.WebCore             	0x000000011343596a WebCore::HTMLStackItem::HTMLStackItem(WebCore::Element&) + 10 (HTMLStackItem.h:107) [inlined]
4   com.apple.WebCore             	0x000000011343596a WebCore::HTMLStackItem::HTMLStackItem(WebCore::Element&) + 10 (HTMLStackItem.h:110) [inlined]
5   com.apple.WebCore             	0x000000011343596a WebCore::HTMLStackItem::create(WebCore::Element&) + 20 (HTMLStackItem.h:115) [inlined]
6   com.apple.WebCore             	0x000000011343596a WebCore::HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext(WebCore::DocumentFragment&, WebCore::Element&) + 32 (HTMLTreeBuilder.cpp:311) [inlined]
7   com.apple.WebCore             	0x000000011343596a WebCore::HTMLTreeBuilder::FragmentParsingContext::FragmentParsingContext(WebCore::DocumentFragment&, WebCore::Element&) + 32 (HTMLTreeBuilder.cpp:309) [inlined]
8   com.apple.WebCore             	0x000000011343596a WebCore::HTMLTreeBuilder::HTMLTreeBuilder(WebCore::HTMLDocumentParser&, WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy, WebCore::HTMLParserOptions const&) + 74 (HTMLTreeBuilder.cpp:281)
9   com.apple.WebCore             	0x000000011342184a std::__1::__unique_if<WebCore::HTMLTreeBuilder>::__unique_single std::__1::make_unique<WebCore::HTMLTreeBuilder, WebCore::HTMLDocumentParser&, WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy, WebCore::HTMLParserOptions&>(WebCore::HTMLDocumentParser&, WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy&&, WebCore::HTMLParserOptions&) + 37 (memory:2755) [inlined]
10  com.apple.WebCore             	0x000000011342184a decltype(auto) WTF::makeUnique<WebCore::HTMLTreeBuilder, WebCore::HTMLDocumentParser&, WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy, WebCore::HTMLParserOptions&>(WebCore::HTMLDocumentParser&, WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy&&, WebCore::HTMLParserOptions&) + 37 (StdLibExtras.h:507) [inlined]
11  com.apple.WebCore             	0x000000011342184a WebCore::HTMLDocumentParser::HTMLDocumentParser(WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy) + 314 (HTMLDocumentParser.cpp:82)
12  com.apple.WebCore             	0x000000011341a50b WebCore::HTMLDocumentParser::HTMLDocumentParser(WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy) + 17 (HTMLDocumentParser.cpp:85) [inlined]
13  com.apple.WebCore             	0x000000011341a50b WebCore::HTMLDocumentParser::create(WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy) + 30 (HTMLDocumentParser.cpp:94) [inlined]
14  com.apple.WebCore             	0x000000011341a50b WebCore::HTMLDocumentParser::parseDocumentFragment(WTF::String const&, WebCore::DocumentFragment&, WebCore::Element&, WebCore::ParserContentPolicy) + 59 (HTMLDocumentParser.cpp:621)
15  com.apple.WebCore             	0x00000001125077be WebCore::createFragment(WebCore::Frame&, NSAttributedString*) + 708 (WebContentReaderCocoa.mm:148) [inlined]
16  com.apple.WebCore             	0x00000001125077be WebCore::createFragmentAndAddResources(WebCore::Frame&, NSAttributedString*) + 926 (WebContentReaderCocoa.mm:395)
17  com.apple.WebCore             	0x000000011250c6f5 WebCore::WebContentReader::readRTF(WebCore::SharedBuffer&) + 133 (WebContentReaderCocoa.mm:646)
18  com.apple.WebCore             	0x0000000112aa2892 WebCore::Pasteboard::read(WebCore::PasteboardWebContentReader&, WebCore::WebContentReadingPolicy, std::__1::optional<unsigned long>) + 4866 (PasteboardMac.mm:493)
19  com.apple.WebCore             	0x00000001124969c6 WebCore::Editor::webContentFromPasteboard(WebCore::Pasteboard&, WebCore::SimpleRange const&, bool, bool&) + 150 (EditorCocoa.mm:235)
20  com.apple.WebCore             	0x000000011250f033 WebCore::Editor::pasteWithPasteboard(WebCore::Pasteboard*, WTF::OptionSet<WebCore::Editor::PasteOption>) + 147 (EditorMac.mm:91)
21  com.apple.WebCore             	0x00000001131e6cd9 WebCore::Editor::paste(WebCore::Pasteboard&, WebCore::Editor::FromMenuOrKeyBinding) + 377 (Editor.cpp:1479)
22  com.apple.WebCore             	0x00000001131e6b1f WebCore::Editor::paste(WebCore::Editor::FromMenuOrKeyBinding) + 95 (Editor.cpp:1465)
23  com.apple.WebCore             	0x0000000113208f03 WebCore::executePaste(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 51 (EditorCommand.cpp:904)
24  com.apple.WebCore             	0x00000001130d70dd WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 93 (Document.cpp:5762)
25  com.apple.WebCore             	0x0000000112386d04 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 240 (JSDocument.cpp:5858) [inlined]
26  com.apple.WebCore             	0x0000000112386d04 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*) + 419 (JSDOMOperation.h:63) [inlined]
27  com.apple.WebCore             	0x0000000112386d04 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 452 (JSDocument.cpp:5863)

<rdar://79664201>

[Ryosuke Niwa]

Created attachment 432248 [details]
Test

[Ryosuke Niwa]

Both reproduce with WebKitTestRunner at r279254.

[Rob Buis]

Created attachment 432327 [details]
Patch

[Rob Buis]

Created attachment 432328 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 432328 [details]
Patch

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

> Source/WebCore/page/DOMSelection.cpp:357
> +        if (!rangeCount())
> +            return Exception { InvalidStateError };

Given we had never thrown before in this case, I think it's too risky to do this.
Let's just ignore and return { } instead at least for now.

[Rob Buis]

Created attachment 432349 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 432349 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Selection.extend() should throw if rangeCount is 0 [1].
> +        However as a first step just make it do nothing.
> +
> +        Behavior matches Firefox and Chrome.

Could you explain how this relates to the actual crash we're fixing?

[Rob Buis]

Document looks like this before calling parseHTML:
*#document  0x61f00005f680 (renderer 0x61700004db00)
    HTML    0x60c0000ba4c0 (renderer 0x612000080d40)
        #text   0x60b0000fd7d0 "<d800>"
        DIV 0x60c0000bd940 (renderer 0x612000083140)
            BR  0x60c0000bddc0 (renderer 0x6110000eeb00)
            HEAD    0x60c0000ba580 (renderer 0x0)
                SCRIPT  0x610000039340 (renderer 0x0)
                    #text   0x60b0000fcee0 "\n  onload = () => {\n    document.documentElement.prepend('\\ud800');\nconsole.log(document.head);\n    getSelection().collapse(document.head, 1);\n    getSelection().extend(document.head);\n    document.designMode = 'on';\n    document.execCommand('InsertParagraph');\n    document.execCommand('SelectAll');\n    document.execCommand('Copy');\n    document.execCommand('Paste');\n  };\n"
                #text   0x60b0000fd040 "\n"
            BODY    0x60c0000bb000 (renderer 0x612000083a40)

[Rob Buis]

Comment on attachment 432349 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Behavior matches Firefox and Chrome.
> 
> Could you explain how this relates to the actual crash we're fixing?

Looking at it again, it is probably a workaround rather than a fix.

[Rob Buis]

Created attachment 432390 [details]
Patch

[Rob Buis]

Created attachment 432399 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 432399 [details]
Patch

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

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:149
> +    // We use a fake body element here to trick the HTML parser into using the InBody insertion mode.
> +    auto fakeBody = HTMLBodyElement::create(document);

This isn't really a fake element, right? It's just a dummy element to force InBody insertion mode.
Instead of adding a comment like this, give it a longer variable name like this:
auto dummyBodyToForceInBodyInsertionMode = ~

[Ryosuke Niwa]

Comment on attachment 432399 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use fake body for parseHTML call

Since this isn't a security bug, please make this patch the bug title.

> Source/WebCore/ChangeLog:8
> +        Use fake body for parseHTML call.

Please explain what was causing the crash & how you fixed it.

> LayoutTests/ChangeLog:3
> +        Use fake body for parseHTML call

Ditto.

[Rob Buis]

Created attachment 432462 [details]
Patch

[EWS]

Committed r279371 (239237@main): <https://commits.webkit.org/239237@main>

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