Bug 224894 – Nullptr crash in CollectionTraversal::traverseForward for selectedOptions

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore                   0x000000010f83a340 WebCore::ContainerNode::firstChild() const + 0 (ContainerNode.h:43) [inlined]
1   com.apple.WebCore                   0x000000010f83a340 WebCore::Node* WebCore::NodeTraversal::traverseNextTemplate<WebCore::ContainerNode const>(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (NodeTraversal.h:90) [inlined]
2   com.apple.WebCore                   0x000000010f83a340 WebCore::NodeTraversal::next(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (NodeTraversal.h:99) [inlined]
3   com.apple.WebCore                   0x000000010f83a340 WebCore::Element* WebCore::Traversal<WebCore::Element>::nextTemplate<WebCore::ContainerNode const>(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (ElementTraversal.h:106) [inlined]
4   com.apple.WebCore                   0x000000010f83a340 WebCore::Traversal<WebCore::Element>::next(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (ElementTraversal.h:252) [inlined]
5   com.apple.WebCore                   0x000000010f83a340 WebCore::ElementIterator<WebCore::Element>::traverseNext() + 8 (ElementIterator.h:87) [inlined]
6   com.apple.WebCore                   0x000000010f83a340 WebCore::ElementDescendantIterator<WebCore::Element>::operator++() + 8 (TypedElementDescendantIterator.h:127) [inlined]
7   com.apple.WebCore                   0x000000010f83a340 void WebCore::CollectionTraversal<(WebCore::CollectionTraversalType)0>::traverseForward<WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0> >(WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0> const&, WebCore::ElementDescendantIterator<WebCore::Element>&, unsigned int, unsigned int&) + 44 (CollectionTraversal.h:81) [inlined]
8   com.apple.WebCore                   0x000000010f83a340 WebCore::CachedHTMLCollection<WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0>, (WebCore::CollectionTraversalType)0>::collectionTraverseForward(WebCore::ElementDescendantIterator<WebCore::Element>&, unsigned int, unsigned int&) const + 48 (CachedHTMLCollection.h:58)
9   com.apple.WebCore                   0x000000010f83a5ea WebCore::CollectionIndexCache<WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0>, WebCore::ElementDescendantIterator<WebCore::Element> >::nodeAt(WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0> const&, unsigned int) + 538 (CollectionIndexCache.h:202)
10  com.apple.WebCore                   0x000000010ead662e WebCore::jsHTMLCollectionPrototypeFunction_itemBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSHTMLCollection*) + 63 (JSHTMLCollection.cpp:294) [inlined]

<rdar://76946123>

[Ryosuke Niwa]

Created attachment 426743 [details]
Test

[Rob Buis]

Created attachment 426815 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 426815 [details]
Patch

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

> Source/WebCore/dom/CollectionIndexCache.h:202
> +        collection.collectionTraverseForward(current, index, currentIndex);

What!? tcollectionTraverseForward is invalidating this collection?
That's certainly wrong! We need to fix that.

[Rob Buis]

Comment on attachment 426815 [details]
Patch

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

>> Source/WebCore/dom/CollectionIndexCache.h:202
>> +        collection.collectionTraverseForward(current, index, currentIndex);
> 
> What!? tcollectionTraverseForward is invalidating this collection?
> That's certainly wrong! We need to fix that.

Indirectly it does. elementMatches will call HTMLOptionElement::selected.  This can call updateListItemSelectedStates on HTMLSelectElement, this can call HTMLOptionElement::setSelectedState which will call HTMLSelectElement::invalidateSelectedItems, which will call invalidateCache on the SelectedOptions cache. Obviously the test case triggers exactly this chain of events. But it seems reasonable to me for the option to ask the select to update itself so selected really reflects the state of selection (I guess this is just in time design)...

[Rob Buis]

A thing about the test, it builds a DOM tree under <select> that would not be accepted as-is when parsed. However Firefox and chromium also build the desired DOM tree when created dynamically.

[Ryosuke Niwa]

(In reply to Rob Buis from comment #4)
> Comment on attachment 426815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426815&action=review
> 
> >> Source/WebCore/dom/CollectionIndexCache.h:202
> >> +        collection.collectionTraverseForward(current, index, currentIndex);
> > 
> > What!? tcollectionTraverseForward is invalidating this collection?
> > That's certainly wrong! We need to fix that.
> 
> Indirectly it does. elementMatches will call HTMLOptionElement::selected. 
> This can call updateListItemSelectedStates on HTMLSelectElement, this can
> call HTMLOptionElement::setSelectedState which will call
> HTMLSelectElement::invalidateSelectedItems, which will call invalidateCache
> on the SelectedOptions cache.

This is not okay. The HTML collection traversal cannot have a side effect like this. We need to change this so that we don't invalidate the HTMLCollection while we're traversing options to find the selected options.

It is a bit suspicious that HTMLOptionElement::setSelectedState calls HTMLSelectElement::invalidateSelectedItems.

Maybe the right fix is to get rid of that call and instead invalidate the HTML Collection in HTMLSelectElement::updateListBoxSelection, HTMLSelectElement::selectOption, HTMLSelectElement::deselectItemsWithoutValidation, HTMLSelectElement::restoreFormControlState, HTMLSelectElement::reset(), and HTMLSelectElement::updateSelectedState

[Rob Buis]

Created attachment 426894 [details]
Patch

[Rob Buis]

Comment on attachment 426815 [details]
Patch

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

>>>> Source/WebCore/dom/CollectionIndexCache.h:202
>>>> +        collection.collectionTraverseForward(current, index, currentIndex);
>>> 
>>> What!? tcollectionTraverseForward is invalidating this collection?
>>> That's certainly wrong! We need to fix that.
>> 
>> Indirectly it does. elementMatches will call HTMLOptionElement::selected.  This can call updateListItemSelectedStates on HTMLSelectElement, this can call HTMLOptionElement::setSelectedState which will call HTMLSelectElement::invalidateSelectedItems, which will call invalidateCache on the SelectedOptions cache. Obviously the test case triggers exactly this chain of events. But it seems reasonable to me for the option to ask the select to update itself so selected really reflects the state of selection (I guess this is just in time design)...
> 
> This is not okay. The HTML collection traversal cannot have a side effect like this. We need to change this so that we don't invalidate the HTMLCollection while we're traversing options to find the selected options.
> 
> It is a bit suspicious that HTMLOptionElement::setSelectedState calls HTMLSelectElement::invalidateSelectedItems.
> 
> Maybe the right fix is to get rid of that call and instead invalidate the HTML Collection in HTMLSelectElement::updateListBoxSelection, HTMLSelectElement::selectOption, HTMLSelectElement::deselectItemsWithoutValidation, HTMLSelectElement::restoreFormControlState, HTMLSelectElement::reset(), and HTMLSelectElement::updateSelectedState

Thanks, that makes a lot of sense. I implemented that and added a somewhat revealing ChangeLog and test, since I think this is not a security issue.

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Rob Buis]

Created attachment 426975 [details]
Patch

[EWS]

Committed r276547 (236987@main): <https://commits.webkit.org/236987@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426975 [details].