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

[bugzilla-daemon at webkit.org]

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

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

-- 
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/7e90f901/attachment.htm>