Bug 223639 – Crash in WebCore::positionInParentBeforeNode(..)

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

[Venky Dass]

Crash when there is a selectall and insert image is executed.

    #0 0x2ec93327a in WebCore::positionInParentBeforeNode(WebCore::Node*)+0x4a (/Users/user/dev/wk/ak/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x493127a)
    #1 0x2eccc8314 in WebCore::ReplaceSelectionCommand::doApply()+0x2644 (/Users/user/dev/wk/ak/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4cc6314)
    #2 0x2ecaa3860 in WebCore::CompositeEditCommand::apply()+0x250 (/Users/user/dev/wk/ak/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4aa1860)
    #3 0x2ecc4923b in WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::RawPtrTraits<WebCore::DocumentFragment> >&&)+0x1ab (/Users/user/dev/wk/ak/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4c4723b)
    #4 0x2ecc496f7 in WebCore::executeInsertNode(WebCore::Frame&, WTF::Ref<WebCore::Node, WTF::RawPtrTraits<WebCore::Node> >&&)+0x197 (/Users/user/dev/wk/ak/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4c476f7)
    #5 0x2ecc3ec43 in WebCore::executeInsertImage(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)+0x1c3 (/Users/user/dev/wk/ak/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4c3cc43)
    #6 0x2ec72668e in WebCore::Document::execCommand(WTF::String const&, bool, WTF

rdar://75426958

[Venky Dass]

Created attachment 424032 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 424032 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1144
> +        Node* br = endingSelection().start().deprecatedNode();
> +        if (br != nullptr) {

Please use RefPtr. Also we don't compare against nullptr:
https://webkit.org/code-style-guidelines/#zero-comparison
So this should be:
if (auto br = makeRefPtr(endingSelection().start().deprecatedNode()))

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1146
> +            // Insert content between the two blockquotes, but remove the br (since it was just a placeholder).

Can we just delete this code? It's pretty obvious from the code below.
There is no need to repeat the code in a comment.

> LayoutTests/editing/inserting/edit-style-and-insert-image.html:8
> +testRunner.dumpAsText();

Please indent this by 4 spaces for better readability.

[Venky Dass]

Created attachment 424258 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 424258 [details]
Patch

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

> LayoutTests/editing/inserting/edit-style-and-insert-image.html:4
> +<p>This tests Pass.</p>

This text isn't printed in the output because of the code we're running.
Also, we want something likely more descriptive than that. Something like this:
This tests InsertImage after setting content property on the document element.<br>
WebKit should not crash.<br>
<br>
PASS

> LayoutTests/editing/inserting/edit-style-and-insert-image.html:11
> +document.execCommand('InsertImage');

Let's reset the style on documentElement here.

[Venky Dass]

Created attachment 424668 [details]
Patch

[EWS]

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

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