From 69d0564a093f796720fa216007811619b2c79223 Mon Sep 17 00:00:00 2001 From: David Capello Date: Fri, 14 Jan 2022 14:39:47 -0300 Subject: [PATCH] Fix crash closing a MenuBox with viewport (fix #3132, fix #3133) Regression introduced in ed595eb6b1fa0967732de7c9f35a658df569b3e6 calling MenuBox::setMenu() of the first child of a MenuBoxWindow when handling its kCloseMessage. The firstChild() was supposed to be a MenuBox, but we didn't take care of the case when the MenuBox is replaced with a View when scrollbars are needed (added by add_scrollbars_if_needed()). --- src/ui/menu.cpp | 86 +++++++++++++++++++++------------------------ src/ui/menu.h | 8 ++--- src/ui/viewport.cpp | 9 +++-- src/ui/widget.cpp | 22 ++++++++---- 4 files changed, 66 insertions(+), 59 deletions(-) diff --git a/src/ui/menu.cpp b/src/ui/menu.cpp index 7d67afbcf..b04103e79 100644 --- a/src/ui/menu.cpp +++ b/src/ui/menu.cpp @@ -352,12 +352,10 @@ void Menu::showPopup(const gfx::Point& pos, } // New window and new menu-box - MenuBox* menubox = new MenuBox(); - std::unique_ptr window( - new MenuBoxWindow(nullptr, menubox, - false)); // Deleted by unique_ptr - window->Open.connect([this]{ this->onOpenPopup(); }); + MenuBoxWindow window; + window.Open.connect([this]{ this->onOpenPopup(); }); + MenuBox* menubox = window.menubox(); MenuBaseData* base = menubox->createBase(); base->was_clicked = true; @@ -365,31 +363,29 @@ void Menu::showPopup(const gfx::Point& pos, menubox->setMenu(this); menubox->startFilteringMouseDown(); - window->remapWindow(); - + window.remapWindow(); fit_bounds(parentDisplay, - window.get(), - gfx::Rect(pos, window->size()), + &window, + gfx::Rect(pos, window.size()), [&window, pos](const gfx::Rect& workarea, gfx::Rect& bounds, std::function getWidgetBounds) { choose_side(bounds, workarea, gfx::Rect(bounds.x-1, bounds.y, 1, 1)); - add_scrollbars_if_needed(window.get(), workarea, bounds); + add_scrollbars_if_needed(&window, workarea, bounds); }); // Set the focus to the new menubox Manager* manager = Manager::getDefault(); manager->setFocus(menubox); - menubox->setFocusMagnet(true); // Open the window - window->openWindowInForeground(); + window.openWindowInForeground(); // Free the keyboard focus if it's in the menu popup, in other case // it means that the user set the focus to other specific widget // before we closed the popup. Widget* focus = manager->getFocus(); - if (focus && focus->window() == window.get()) + if (focus && focus->window() == &window) focus->releaseFocus(); menubox->stopFilteringMouseDown(); @@ -892,15 +888,17 @@ bool MenuItem::onProcessMessage(Message* msg) ASSERT(base->is_processing); ASSERT(hasSubmenu()); - MenuBox* menubox = new MenuBox(); + // New window that will be automatically deleted + auto window = new MenuBoxWindow(this); + window->Close.connect([window]{ + window->deferDelete(); + }); + + MenuBox* menubox = window->menubox(); m_submenu_menubox = menubox; menubox->setMenu(m_submenu); - // New window and new menu-box - auto window = new MenuBoxWindow( - this, menubox, - true); // Call defer delete when it's closed - + window->remapWindow(); fit_bounds( display(), window, window->bounds(), [this, window](const gfx::Rect& workarea, @@ -924,9 +922,6 @@ bool MenuItem::onProcessMessage(Message* msg) add_scrollbars_if_needed(window, workarea, bounds); }); - // Set the focus to the new menubox - menubox->setFocusMagnet(true); - // Setup the highlight of the new menubox if (select_first) { // Select the first child @@ -966,13 +961,10 @@ bool MenuItem::onProcessMessage(Message* msg) ASSERT(base->is_processing); - MenuBox* menubox = m_submenu_menubox; - m_submenu_menubox = nullptr; - - ASSERT(menubox != nullptr); - - Window* window = menubox->window(); + ASSERT(m_submenu_menubox); + Window* window = m_submenu_menubox->window(); ASSERT(window && window->type() == kWindowWidget); + m_submenu_menubox = nullptr; // Destroy the window window->closeWindow(nullptr); @@ -1493,26 +1485,34 @@ static MenuItem* find_previtem(Menu* menu, MenuItem* menuitem) ////////////////////////////////////////////////////////////////////// // MenuBoxWindow -MenuBoxWindow::MenuBoxWindow(MenuItem* menuitem, - MenuBox* menubox, - const bool deferDelete) +MenuBoxWindow::MenuBoxWindow(MenuItem* menuitem) : Window(WithoutTitleBar, "") , m_menuitem(menuitem) - , m_deferDelete(deferDelete) { setMoveable(false); // Can't move the window setSizeable(false); // Can't resize the window - addChild(menubox); - remapWindow(); + + m_menubox.setFocusMagnet(true); + + addChild(&m_menubox); } MenuBoxWindow::~MenuBoxWindow() { - if (auto menubox = this->menubox()) { - // The menu of the menubox should already be nullptr because it - // was reset in kCloseMessage. - ASSERT(menubox->getMenu() == nullptr); - menubox->setMenu(nullptr); + // The menu of the menubox should already be nullptr because it + // was reset in kCloseMessage. + ASSERT(m_menubox.getMenu() == nullptr); + m_menubox.setMenu(nullptr); + + // This can fail in case that add_scrollbars_if_needed() replaced + // the MenuBox widget with a View, and now the MenuBox is inside the + // viewport. + if (hasChild(&m_menubox)) { + removeChild(&m_menubox); + } + else { + ASSERT(firstChild() != nullptr && + firstChild()->type() == kViewWidget); } } @@ -1534,13 +1534,7 @@ bool MenuBoxWindow::onProcessMessage(Message* msg) } // Fetch the "menu" to avoid destroy it with 'delete'. - auto menubox = this->menubox(); - if (menubox) - menubox->setMenu(nullptr); - - // Delete this window automatically - if (m_deferDelete) - deferDelete(); + m_menubox.setMenu(nullptr); break; } diff --git a/src/ui/menu.h b/src/ui/menu.h index f7a0fc9d7..73596366a 100644 --- a/src/ui/menu.h +++ b/src/ui/menu.h @@ -172,16 +172,14 @@ namespace ui { class MenuBoxWindow : public Window { public: - MenuBoxWindow(MenuItem* menuitem, - MenuBox* menubox, - const bool deferDelete); + MenuBoxWindow(MenuItem* menuitem = nullptr); ~MenuBoxWindow(); - MenuBox* menubox() { return static_cast(firstChild()); } + MenuBox* menubox() { return &m_menubox; } protected: bool onProcessMessage(Message* msg) override; private: + MenuBox m_menubox; MenuItem* m_menuitem; - const bool m_deferDelete; }; extern RegisterMessage kOpenMenuItemMessage; diff --git a/src/ui/viewport.cpp b/src/ui/viewport.cpp index aebc6b7dc..57dd2358a 100644 --- a/src/ui/viewport.cpp +++ b/src/ui/viewport.cpp @@ -1,5 +1,5 @@ // Aseprite UI Library -// Copyright (C) 2018-2019 Igara Studio S.A. +// Copyright (C) 2018-2022 Igara Studio S.A. // Copyright (C) 2001-2015 David Capello // // This file is released under the terms of the MIT license. @@ -36,7 +36,12 @@ void Viewport::onResize(ResizeEvent& ev) Rect rect = ev.bounds(); setBoundsQuietly(rect); - Point scroll = static_cast(parent())->viewScroll(); + auto view = static_cast(parent()); + ASSERT(view); + if (!view) + return; + + Point scroll = view->viewScroll(); Rect cpos(0, 0, 0, 0); cpos.x = rect.x + border().left() - scroll.x; diff --git a/src/ui/widget.cpp b/src/ui/widget.cpp index 818967582..fba56702d 100644 --- a/src/ui/widget.cpp +++ b/src/ui/widget.cpp @@ -1,5 +1,5 @@ // Aseprite UI Library -// Copyright (C) 2018-2021 Igara Studio S.A. +// Copyright (C) 2018-2022 Igara Studio S.A. // Copyright (C) 2001-2018 David Capello // // This file is released under the terms of the MIT license. @@ -553,13 +553,21 @@ void Widget::addChild(Widget* child) void Widget::removeChild(WidgetsList::iterator& it) { - Widget* child = *it; + Widget* child = nullptr; ASSERT(it != m_children.end()); - if (it != m_children.end()) + if (it != m_children.end()) { + child = *it; m_children.erase(it); - // Free from manager + ASSERT(child); + if (!child) + return; + } + else + return; + + // Free child from manager if (auto man = manager()) man->freeWidget(child); @@ -571,8 +579,10 @@ void Widget::removeChild(Widget* child) ASSERT_VALID_WIDGET(this); ASSERT_VALID_WIDGET(child); - WidgetsList::iterator it = std::find(m_children.begin(), m_children.end(), child); - removeChild(it); + auto it = std::find(m_children.begin(), m_children.end(), child); + ASSERT(it != m_children.end()); + if (it != m_children.end()) + removeChild(it); } void Widget::removeAllChildren()