Bug 200023 – Fix crashes in ScrollingStateNode::insertChild()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Fix crashes in ScrollingStateNode::insertChild()

[Simon Fraser (smfr)]

Created attachment 374662 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/53265378>

[Darin Adler]

Comment on attachment 374662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374662&action=review

> Source/WebCore/ChangeLog:13
> +        Crash data suggest that ScrollingStateNode::insertChild() can be passed an index that
> +        is larger than the size of the vector, causing crashes.
> +
> +        Fix defensively by falling back to append() if the passed index is equal to or larger
> +        than the size of the children vector.

Is there a reason we don’t do this inside the insert function instead? Are there other call sites where we want the stricter behavior?

What about asserting the index is valid, even if we prevent the crash in such cases? That could help us some day understand how this happens if we reproduce in a build with assertions on.

[Simon Fraser (smfr)]

https://trac.webkit.org/r247734

[Darin Adler]

Comment on attachment 374662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374662&action=review

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:119
> +    if (index >= m_children->size())

The patch you landed uses ">" rather than ">=". Why is that?

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #5)
> Comment on attachment 374662 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374662&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:119
> > +    if (index >= m_children->size())
> 
> The patch you landed uses ">" rather than ">=". Why is that?

Because Vector::insert() allows inserting at index = size() - it has:
    ASSERT_WITH_SECURITY_IMPLICATION(position <= size());