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

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Julian Gonzalez <julian_a_gonzalez at apple.com> ---
(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.

-- 
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/6a8a2388/attachment-0001.htm>