[Webkit-unassigned] [Bug 224259] Nullptr dereference in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=224259

--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 425343
  --> https://bugs.webkit.org/attachment.cgi?id=425343
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:402
> +inline void ReplaceSelectionCommand::InsertedNodes::willRemovePossibleAncestorNode(Node *node)

Nit: * should be on the right of Node, not on the left of node.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:409
> +        m_lastNodeInserted = nullptr;
> +    } else {

We should just add an early exit here.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:415
> +        // Check if the first and last nodes have swapped relative positions.

I don't think we need to have this comment.
It's pretty obvious what we're doing from the code below.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:420
> +        else if (m_firstNodeInserted != m_lastNodeInserted && m_lastNodeInserted->contains(m_firstNodeInserted.get())) {

This simply m_firstNodeInserted->isDescendantOf(*m_lastNodeInserted)

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:421
> +            // Now first == last, and last == first. Swap the pointers.

This is a confusing comment. If we used std::swap below, it would be trivially obvious.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:422
> +            ASSERT(!m_firstNodeInserted->contains(m_lastNodeInserted.get()));

This assertion is unnecessary if we used isDescendantOf because it would be trivially true.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:425
> +            auto temp = m_firstNodeInserted;
> +            m_firstNodeInserted = m_lastNodeInserted;
> +            m_lastNodeInserted = temp;

Why not just std::swap(~)?

> LayoutTests/editing/inserting/insert-display-contents-crash.html:23
> +    document.execCommand('InsertHTML', false, '<h1>PASS');

Can we add some description that this test passes if we don't crash?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210407/b874c771/attachment.htm>