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>
Created attachment 425343 [details] Patch
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?
(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.
(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.
Created attachment 425435 [details] Patch
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');
Created attachment 425464 [details] Patch
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.
Created attachment 425474 [details] Patch
Committed r275652: <https://commits.webkit.org/r275652> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425474 [details].