editing/firstPositionInNode-crash.html in crashing in Debug: ASSERTION FAILED: !m_anchorNode || !editingIgnoresContent(*m_anchorNode) ./dom/Position.cpp(133) : WebCore::Position::Position(WebCore::Node *, int, WebCore::Position::AnchorType) 1 0x4a2697c09 WTFCrash 2 0x48a2dd88b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x48ca6ceb9 WebCore::Position::Position(WebCore::Node*, int, WebCore::Position::AnchorType) 4 0x48ca6cfc9 WebCore::Position::Position(WebCore::Node*, int, WebCore::Position::AnchorType) 5 0x48c2d89ac WebCore::positionInParentBeforeNode(WebCore::Node const*) 6 0x48cb899e6 WebCore::updatePositionForNodeRemoval(WebCore::Position&, WebCore::Node&) 7 0x48cbc1db6 WebCore::FrameSelection::respondToNodeModification(WebCore::Node&, bool, bool, bool, bool) 8 0x48cbc1cd6 WebCore::FrameSelection::nodeWillBeRemoved(WebCore::Node&) 9 0x48c8f641f WebCore::Document::nodeWillBeRemoved(WebCore::Node&) 10 0x48c88b5c3 WebCore::ContainerNode::removeNodeWithScriptAssertion(WebCore::Node&, WebCore::ContainerNode::ChildChangeSource) 11 0x48c88af21 WebCore::ContainerNode::removeChild(WebCore::Node&) 12 0x48c889678 WebCore::collectChildrenAndRemoveFromOldParent(WebCore::Node&, WTF::Vector<WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >, 11ul, WTF::CrashOnOverflow, 16ul>&) 13 0x48c888eea WebCore::ContainerNode::insertBefore(WebCore::Node&, WebCore::Node*) 14 0x48ca4a018 WebCore::Node::before(WTF::Vector<WTF::Variant<WTF::RefPtr<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >, WTF::String>, 0ul, WTF::CrashOnOverflow, 16ul>&&) 15 0x48acda648 WebCore::jsElementPrototypeFunctionBeforeBody(JSC::ExecState*, WebCore::JSElement*, JSC::ThrowScope&) 16 0x48acc3130 long long WebCore::IDLOperation<WebCore::JSElement>::call<&(WebCore::jsElementPrototypeFunctionBeforeBody(JSC::ExecState*, WebCore::JSElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) 17 0x48acc2e1c WebCore::jsElementPrototypeFunctionBefore(JSC::ExecState*)
<rdar://problem/55947146>
Created attachment 382106 [details] Patch
Comment on attachment 382106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382106&action=review > Source/WebCore/editing/Editing.h:248 > +inline Position positionInParentBeforeNode(const Node* node) I don't think we want to generate the loop everywhere. This function is used in a lot of places. We should just make this out-of-line function in Editing.cpp instead. > Source/WebCore/editing/Editing.h:254 > + return Position(ancestor, node->computeNodeIndex(), Position::PositionIsOffsetInAnchor); Clearly, we can't use whatever node index of the starting node. We need to use the node index of the last node. r- because this is clearly wrong. > Source/WebCore/editing/Editing.h:257 > +inline Position positionInParentAfterNode(const Node* node) Ditto. > Source/WebCore/editing/Editing.h:263 > + return Position(ancestor, node->computeNodeIndex() + 1, Position::PositionIsOffsetInAnchor); Ditto.
Created attachment 382113 [details] Patch
Comment on attachment 382113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382113&action=review > Source/WebCore/dom/Position.cpp:1574 > + auto* ancestor = node->parentNode(); Please use RefPtr. > Source/WebCore/dom/Position.cpp:1585 > + auto* ancestor = node->parentNode(); Ditto.
Why is this in the security component?? There is no security implication here.
(In reply to Ryosuke Niwa from comment #6) > Why is this in the security component?? There is no security implication > here. I thought the radar was tagged as security since this is a security test, but it looks like it is not.
Comment on attachment 382113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382113&action=review >> Source/WebCore/dom/Position.cpp:1574 >> + auto* ancestor = node->parentNode(); > > Please use RefPtr. The Position() constructor takes in a raw pointer. So if ref here, it will create unnecessary ref counting churn. Why do you feel like I should ref here? I am merely going up the frame tree, not running script.
(In reply to Chris Dumez from comment #8) > Comment on attachment 382113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382113&action=review > > >> Source/WebCore/dom/Position.cpp:1574 > >> + auto* ancestor = node->parentNode(); > > > > Please use RefPtr. > > The Position() constructor takes in a raw pointer. So if ref here, it will > create unnecessary ref counting churn. Why do you feel like I should ref > here? I am merely going up the frame tree, not running script. *DOM tree, not frame tree
Comment on attachment 382113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382113&action=review >>>> Source/WebCore/dom/Position.cpp:1574 >>>> + auto* ancestor = node->parentNode(); >>> >>> Please use RefPtr. >> >> The Position() constructor takes in a raw pointer. So if ref here, it will create unnecessary ref counting churn. Why do you feel like I should ref here? I am merely going up the frame tree, not running script. > > *DOM tree, not frame tree Also, if we really want to use RefPtr<>, then I'd likely have to ref node too, which means I'd have to add a new local variable to ref it.
Created attachment 382114 [details] Patch
(In reply to Chris Dumez from comment #10) > Comment on attachment 382113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382113&action=review > > >>>> Source/WebCore/dom/Position.cpp:1574 > >>>> + auto* ancestor = node->parentNode(); > >>> > >>> Please use RefPtr. > >> > >> The Position() constructor takes in a raw pointer. So if ref here, it will create unnecessary ref counting churn. Why do you feel like I should ref here? I am merely going up the frame tree, not running script. We're trying to get rid of all uses of raw pointers in WebCore. > > *DOM tree, not frame tree > > Also, if we really want to use RefPtr<>, then I'd likely have to ref node > too, which means I'd have to add a new local variable to ref it. Okay. We can leave it for now. I'll get rid of it in a subsequent patch.
Comment on attachment 382114 [details] Patch Clearing flags on attachment: 382114 Committed r251683: <https://trac.webkit.org/changeset/251683>
All reviewed patches have been landed. Closing bug.