| Summary: | Nullptr crash in CollectionTraversal::traverseForward for selectedOptions | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | DOM | Assignee: | Rob Buis <rbuis> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, cdumez, cgarcia, changseok, esprehn+autocc, ews-feeder, ews-watchlist, fred.wang, gpoo, gyuyoung.kim, mifenton, product-security, rbuis, svillar, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ryosuke Niwa
2021-04-21 14:07:37 PDT
Created attachment 426743 [details]
Test
Created attachment 426815 [details]
Patch
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. 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)... 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. (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 Created attachment 426894 [details]
Patch
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. ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Created attachment 426975 [details]
Patch
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]. |