Bug 224259 – Nullptr dereference in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline

• Component: HTML Editing
• Status: RESOLVED FIXED

[Julian Gonzalez]

e.g.

    #0 0x114f9e976 in WebCore::ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline(WebCore::ReplaceSelectionCommand::InsertedNodes&)
    #1 0x114faf0c0 in WebCore::ReplaceSelectionCommand::doApply()
    #2 0x114d83d80 in WebCore::CompositeEditCommand::apply()
    #3 0x114f2b7ab in WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::RawPtrTraits<WebCore::DocumentFragment> >&&)
    #4 0x114f207d5 in WebCore::executeInsertHTML(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
    #5 0x1149f779e in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)

<rdar://problem/76003461>

[Julian Gonzalez]

Created attachment 425343 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 425343 [details]
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?

[Julian Gonzalez]

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 425343 [details]
> 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.
> 

This is matching willRemoveNodePreservingChildren() - that seems wrong too. What's the policy on fixing style nits in code that would not otherwise be part of a patch?


> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:409
> > +        m_lastNodeInserted = nullptr;
> > +    } else {
> 
> We should just add an early exit here.

Agreed.

> 
> > 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.
> 

It doesn't feel obvious to me, but I'll remove it.

> > 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)

Thanks, that's definitely an improvement.

> 
> > 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.

I'll use std::swap.

> 
> > 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.

I'll use it here too.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:425
> > +            auto temp = m_firstNodeInserted;
> > +            m_firstNodeInserted = m_lastNodeInserted;
> > +            m_lastNodeInserted = temp;
> 
> Why not just std::swap(~)?

You made that point already :)

> 
> > 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?

This test does not crash if you add a body (or doctype, even). I can add an HTML comment.

[Julian Gonzalez]

(In reply to Julian Gonzalez from comment #3)
> (In reply to Ryosuke Niwa from comment #2)
> > Comment on attachment 425343 [details]
> > 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.
> > 
> 
> This is matching willRemoveNodePreservingChildren() - that seems wrong too.
> What's the policy on fixing style nits in code that would not otherwise be
> part of a patch?

Ah, I think that was a typo I introduced, never mind.
> 
> 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:409
> > > +        m_lastNodeInserted = nullptr;
> > > +    } else {
> > 
> > We should just add an early exit here.
> 
> Agreed.
> 
> > 
> > > 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.
> > 
> 
> It doesn't feel obvious to me, but I'll remove it.
> 
> > > 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)
> 
> Thanks, that's definitely an improvement.
> 
> > 
> > > 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.
> 
> I'll use std::swap.
> 
> > 
> > > 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.
> 
> I'll use it here too.
> 
> > 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:425
> > > +            auto temp = m_firstNodeInserted;
> > > +            m_firstNodeInserted = m_lastNodeInserted;
> > > +            m_lastNodeInserted = temp;
> > 
> > Why not just std::swap(~)?
> 
> You made that point already :)
> 
> > 
> > > 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?
> 
> This test does not crash if you add a body (or doctype, even). I can add an
> HTML comment.

[Julian Gonzalez]

Created attachment 425435 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 425435 [details]
Patch

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

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

Oh I mean that here, can do:
document.execCommand('InsertHTML', false, '<h1>This test passes if WebKit does not crash. PASS');

[Julian Gonzalez]

Created attachment 425464 [details]
Patch

[Darin Adler]

Comment on attachment 425464 [details]
Patch

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

Our new code has many branches. Our new test does not cover them. I think we need more tests to cover all the cases that require different logic.

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

Seems like we should consider "!isDescendantOf" here to shorten these assertions:

    ASSERT(!m_firstNodeInserted || !m_firstNodeInserted->isDescendantOf(node));
    ASSERT(!m_lastNodeInserted || !m_lastNodeInserted->isDescendantOf(node));

Would be even better if we had a non-member version of isDescendantOf that understands that null isn’t a descendant of anything.

[Julian Gonzalez]

Created attachment 425474 [details]
Patch

[EWS]

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

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