Bug 203520 – editing/firstPositionInNode-crash.html in crashing in Debug

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Chris Dumez]

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

[Chris Dumez]

<rdar://problem/55947146>

[Chris Dumez]

Created attachment 382106 [details]
Patch

[Ryosuke Niwa]

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.

[Chris Dumez]

Created attachment 382113 [details]
Patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Why is this in the security component?? There is no security implication here.

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

(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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 382114 [details]
Patch

[Ryosuke Niwa]

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

[Chris Dumez]

Comment on attachment 382114 [details]
Patch

Clearing flags on attachment: 382114

Committed r251683: <https://trac.webkit.org/changeset/251683>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.