Skip to content

Commit

Permalink
Cherry-pick 35318b4. rdar://124409462
Browse files Browse the repository at this point in the history
    Crash under ~RenderMenuList due to CheckedPtr usage
    https://bugs.webkit.org/show_bug.cgi?id=269322
    rdar://119790256

    Reviewed by Alan Baradlay.

    From the crash trace, we can see that HTMLSelectElement::defaultEventHandler()
    holds a CheckedPtr to its RenderMenuList renderer and calls showPopup() on
    the renderer. This ends up running JS, which removes the select element from
    the DOM and in turns destroys the renderer. The usage is currently safe since
    nothing is using the renderer after the JS has run. However, it was tripping
    the CheckedPtr assertion.

    To address the issue, switch to using WeakPtr for now and add comments to
    clarify lifetime. We should consider refactoring this in a follow up though.

    * Source/WebCore/html/HTMLSelectElement.cpp:
    (WebCore::HTMLSelectElement::platformHandleKeydownEvent):
    (WebCore::HTMLSelectElement::menuListDefaultEventHandler):
    (WebCore::HTMLSelectElement::showPicker):
    * Source/WebCore/rendering/RenderMenuList.cpp:
    (RenderMenuList::showPopup):

    Canonical link: https://commits.webkit.org/274586@main
  • Loading branch information
cdumez authored and Mohsin Qureshi committed Mar 15, 2024
1 parent d3846a0 commit 904fdc1
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 12 deletions.
21 changes: 10 additions & 11 deletions Source/WebCore/html/HTMLSelectElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1145,7 +1145,7 @@ bool HTMLSelectElement::platformHandleKeydownEvent(KeyboardEvent* event)
// Calling focus() may cause us to lose our renderer. Return true so
// that our caller doesn't process the event further, but don't set
// the event as handled.
CheckedPtr renderer = dynamicDowncast<RenderMenuList>(this->renderer());
WeakPtr renderer = dynamicDowncast<RenderMenuList>(this->renderer());
if (!renderer)
return true;

Expand All @@ -1154,7 +1154,7 @@ bool HTMLSelectElement::platformHandleKeydownEvent(KeyboardEvent* event)
// gets called from RenderMenuList::valueChanged, which gets called
// after the user makes a selection from the menu.
saveLastSelection();
renderer->showPopup();
renderer->showPopup(); // showPopup() may run JS and cause the renderer to get destroyed.
event->setDefaultHandled();
}
return true;
Expand Down Expand Up @@ -1244,7 +1244,7 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event& event)
document().updateStyleIfNeeded();

// Calling focus() may remove the renderer or change the renderer type.
CheckedPtr renderer = dynamicDowncast<RenderMenuList>(this->renderer());
WeakPtr renderer = dynamicDowncast<RenderMenuList>(this->renderer());
if (!renderer)
return;

Expand All @@ -1253,7 +1253,7 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event& event)
// gets called from RenderMenuList::valueChanged, which gets called
// after the user makes a selection from the menu.
saveLastSelection();
renderer->showPopup();
renderer->showPopup(); // showPopup() may run JS and cause the renderer to get destroyed.
handled = true;
}
} else if (RenderTheme::singleton().popsMenuByArrowKeys()) {
Expand All @@ -1262,7 +1262,7 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event& event)
document().updateStyleIfNeeded();

// Calling focus() may remove the renderer or change the renderer type.
CheckedPtr renderer = dynamicDowncast<RenderMenuList>(this->renderer());
WeakPtr renderer = dynamicDowncast<RenderMenuList>(this->renderer());
if (!renderer)
return;

Expand All @@ -1271,7 +1271,7 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event& event)
// gets called from RenderMenuList::valueChanged, which gets called
// after the user makes a selection from the menu.
saveLastSelection();
renderer->showPopup();
renderer->showPopup(); // showPopup() may run JS and cause the renderer to get destroyed.
handled = true;
} else if (keyCode == '\r') {
if (RefPtr form = this->form())
Expand All @@ -1290,15 +1290,15 @@ void HTMLSelectElement::menuListDefaultEventHandler(Event& event)
#if !PLATFORM(IOS_FAMILY)
document().updateStyleIfNeeded();

if (CheckedPtr menuList = dynamicDowncast<RenderMenuList>(renderer())) {
if (WeakPtr menuList = dynamicDowncast<RenderMenuList>(renderer())) {
ASSERT(!menuList->popupIsVisible());
// Save the selection so it can be compared to the new
// selection when we call onChange during selectOption,
// which gets called from RenderMenuList::valueChanged,
// which gets called after the user makes a selection from
// the menu.
saveLastSelection();
menuList->showPopup();
menuList->showPopup(); // showPopup() may run JS and cause the renderer to get destroyed.
}
#endif
event.setDefaultHandled();
Expand Down Expand Up @@ -1687,9 +1687,8 @@ ExceptionOr<void> HTMLSelectElement::showPicker()
return Exception { ExceptionCode::NotAllowedError, "Select showPicker() requires a user gesture."_s };

#if !PLATFORM(IOS_FAMILY)
auto* renderer = this->renderer();
if (auto* renderMenuList = dynamicDowncast<RenderMenuList>(renderer))
renderMenuList->showPopup();
if (WeakPtr renderMenuList = dynamicDowncast<RenderMenuList>(renderer()))
renderMenuList->showPopup(); // showPopup() may run JS and cause the renderer to get destroyed.
#endif

return { };
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/rendering/RenderMenuList.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ void RenderMenuList::showPopup()
FloatPoint absTopLeft = localToAbsolute(FloatPoint(), UseTransforms);
IntRect absBounds = absoluteBoundingBoxRectIgnoringTransforms();
absBounds.setLocation(roundedIntPoint(absTopLeft));
m_popup->show(absBounds, &view().frameView(), selectElement().optionToListIndex(selectElement().selectedIndex()));
m_popup->show(absBounds, &view().frameView(), selectElement().optionToListIndex(selectElement().selectedIndex())); // May destroy `this`.
}
#endif

Expand Down

0 comments on commit 904fdc1

Please sign in to comment.