Skip to content

Commit 277383f

Browse files
committed
Fix crash in style invalidation when element has no parent during removal
https://bugs.webkit.org/show_bug.cgi?id=301879 rdar://163025404 Reviewed by Antti Koivisto. This is a fuzzer found bug that results in a crash. When hidePopoverInternal() is called during element removal (due to outerText being set), style invalidation runs even when parentNode is null. This is incorrect. We shouldn't be entering style invalidation in this case at all. While the rest of the cleanup in hidePopoverInternal() is necessary, style invalidation is not here. As a result, this fix adds a null check to only perform style invalidation when elements are NOT being removed (aka its parent still exists). Since we add this check, the existing null check further down in invalidateStyleWithMatchElement() is redundant and no longer necessary. There were 2 previous attempted fixes for this: 286644@main and 293967@main. 286644@main had an incorrect Style::InvalidationScope::Descendants. 293967@main didn't add null checks to the other relevant cases in invalidateStyleWithMatchElement(). This PR correctly implements the full fix. Test: fast/dom/Element/nth-child-of-popover-open-crash.html * LayoutTests/fast/dom/Element/nth-child-of-popover-open-crash-expected.txt: Added. * LayoutTests/fast/dom/Element/nth-child-of-popover-open-crash.html: Added. * Source/WebCore/html/HTMLElement.cpp: (WebCore::HTMLElement::hidePopoverInternal): * Source/WebCore/style/StyleInvalidator.cpp: (WebCore::Style::Invalidator::invalidateStyleWithMatchElement): Canonical link: https://commits.webkit.org/303507@main
1 parent 5e283cb commit 277383f

File tree

4 files changed

+24
-5
lines changed

4 files changed

+24
-5
lines changed
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
This test passes if WebKit does not crash.
2+
3+
X
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<p>This test passes if WebKit does not crash.</p>
2+
<style></style>
3+
<script>
4+
if (window.testRunner)
5+
testRunner.dumpAsText();
6+
7+
document.styleSheets[0].insertRule(`@scope (:nth-last-child(1 of :popover-open)) { i { } }`);
8+
9+
let element = document.createElement('div');
10+
document.body.append(element);
11+
12+
element.popover = 'auto';
13+
element.showPopover();
14+
element.scrollIntoView();
15+
element.outerText = 'X';
16+
</script>

Source/WebCore/html/HTMLElement.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1243,7 +1243,9 @@ ExceptionOr<void> HTMLElement::hidePopoverInternal(FocusPreviousElement focusPre
12431243
if (isInTopLayer())
12441244
removeFromTopLayer();
12451245

1246-
Style::PseudoClassChangeInvalidation styleInvalidation(*this, CSSSelector::PseudoClass::PopoverOpen, false);
1246+
std::optional<Style::PseudoClassChangeInvalidation> styleInvalidation;
1247+
if (parentNode())
1248+
styleInvalidation.emplace(*this, CSSSelector::PseudoClass::PopoverOpen, false);
12471249
popoverData()->setVisibilityState(PopoverVisibilityState::Hidden);
12481250

12491251
if (fireEvents == FireEvents::Yes)

Source/WebCore/style/StyleInvalidator.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -303,10 +303,8 @@ void Invalidator::invalidateStyleWithMatchElement(Element& element, MatchElement
303303
break;
304304
case MatchElement::AnySibling:
305305
// :nth-last-child(even of .changed)
306-
if (CheckedPtr parentNode = element.parentNode()) {
307-
for (auto& parentChild : childrenOfType<Element>(*element.parentNode()))
308-
invalidateIfNeeded(parentChild, nullptr);
309-
}
306+
for (auto& parentChild : childrenOfType<Element>(*element.parentNode()))
307+
invalidateIfNeeded(parentChild, nullptr);
310308
break;
311309
case MatchElement::ParentSibling:
312310
// .changed ~ .a > .subject

0 commit comments

Comments
 (0)