From 5abf73e3c9a2b013f14c4bec5e8cb5256d916675 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Fri, 16 Dec 2022 16:37:32 -0500 Subject: [PATCH] Never call ReCreateMenuBar inside a menu event handler As of wxWidgets 3.2, the wxWidgets event handler runs code after the the client event handler that depends on the menu still existing. Because there are potentially many paths to call ReCreateMenuBar from within a menu event handler, let's just wrap this action in a CallAfter to make sure it happens after the wx handler call completes. Fixes https://gitlab.com/kicad/code/kicad/-/issues/13149 --- bitmap2component/bitmap2cmp_gui.cpp | 2 +- bitmap2component/bitmap2cmp_gui.h | 3 ++- common/eda_base_frame.cpp | 11 +++++++++++ cvpcb/cvpcb_mainframe.h | 3 ++- cvpcb/menubar.cpp | 2 +- eeschema/menubar.cpp | 2 +- eeschema/sch_edit_frame.h | 3 ++- eeschema/symbol_editor/menubar_symbol_editor.cpp | 2 +- eeschema/symbol_editor/symbol_edit_frame.h | 4 ++-- eeschema/symbol_viewer_frame.h | 3 ++- eeschema/toolbars_symbol_viewer.cpp | 2 +- gerbview/gerbview_frame.h | 2 +- gerbview/menubar.cpp | 2 +- include/eda_base_frame.h | 4 +++- include/eda_draw_frame.h | 3 ++- include/pcb_base_frame.h | 3 ++- kicad/kicad_manager_frame.h | 3 ++- kicad/menubar.cpp | 2 +- pagelayout_editor/menubar.cpp | 2 +- pagelayout_editor/pl_editor_frame.h | 4 ++-- pcbnew/footprint_edit_frame.h | 10 +++++----- pcbnew/footprint_viewer_frame.h | 3 ++- pcbnew/menubar_footprint_editor.cpp | 2 +- pcbnew/menubar_pcb_editor.cpp | 2 +- pcbnew/pcb_base_frame.cpp | 2 +- pcbnew/pcb_edit_frame.h | 3 ++- pcbnew/toolbars_footprint_viewer.cpp | 2 +- 27 files changed, 54 insertions(+), 32 deletions(-) diff --git a/bitmap2component/bitmap2cmp_gui.cpp b/bitmap2component/bitmap2cmp_gui.cpp index c31276768c..e6a4227743 100644 --- a/bitmap2component/bitmap2cmp_gui.cpp +++ b/bitmap2component/bitmap2cmp_gui.cpp @@ -206,7 +206,7 @@ wxWindow* BM2CMP_FRAME::GetToolCanvas() const } -void BM2CMP_FRAME::ReCreateMenuBar() +void BM2CMP_FRAME::doReCreateMenuBar() { // wxWidgets handles the Mac Application menu behind the scenes, but that means // we always have to start from scratch with a new wxMenuBar. diff --git a/bitmap2component/bitmap2cmp_gui.h b/bitmap2component/bitmap2cmp_gui.h index 7a50b190c5..513e928787 100644 --- a/bitmap2component/bitmap2cmp_gui.h +++ b/bitmap2component/bitmap2cmp_gui.h @@ -158,7 +158,8 @@ private: wxWindow* GetToolCanvas() const override; - void ReCreateMenuBar() override; +protected: + void doReCreateMenuBar() override; private: wxImage m_Pict_Image; diff --git a/common/eda_base_frame.cpp b/common/eda_base_frame.cpp index fb5cae994c..ba1cfbc718 100644 --- a/common/eda_base_frame.cpp +++ b/common/eda_base_frame.cpp @@ -445,6 +445,17 @@ void EDA_BASE_FRAME::setupUIConditions() void EDA_BASE_FRAME::ReCreateMenuBar() { + /** + * As of wxWidgets 3.2, recreating the menubar from within an event handler of that menubar + * will result in memory corruption on macOS. In order to minimize the chance of programmer + * error causing regressions here, we always wrap calls to ReCreateMenuBar in a CallAfter to + * ensure that they do not occur within the same event handling call stack. + */ + + CallAfter( [=]() + { + doReCreateMenuBar(); + } ); } diff --git a/cvpcb/cvpcb_mainframe.h b/cvpcb/cvpcb_mainframe.h index 0fa684843d..05931368a7 100644 --- a/cvpcb/cvpcb_mainframe.h +++ b/cvpcb/cvpcb_mainframe.h @@ -144,7 +144,6 @@ public: * Functions to rebuild the toolbars and menubars */ void ReCreateHToolbar(); - void ReCreateMenuBar() override; void ShowChangedLanguage() override; @@ -304,6 +303,8 @@ public: protected: CVPCB_MAINFRAME( KIWAY* aKiway, wxWindow* aParent ); + void doReCreateMenuBar() override; + void setupUIConditions() override; bool canCloseWindow( wxCloseEvent& aCloseEvent ) override; diff --git a/cvpcb/menubar.cpp b/cvpcb/menubar.cpp index ea6f9a141a..0b60b64ca9 100644 --- a/cvpcb/menubar.cpp +++ b/cvpcb/menubar.cpp @@ -34,7 +34,7 @@ #include -void CVPCB_MAINFRAME::ReCreateMenuBar() +void CVPCB_MAINFRAME::doReCreateMenuBar() { COMMON_CONTROL* tool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/eeschema/menubar.cpp b/eeschema/menubar.cpp index 668cccce9e..f01d8a90a2 100644 --- a/eeschema/menubar.cpp +++ b/eeschema/menubar.cpp @@ -40,7 +40,7 @@ #include -void SCH_EDIT_FRAME::ReCreateMenuBar() +void SCH_EDIT_FRAME::doReCreateMenuBar() { EE_SELECTION_TOOL* selTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/eeschema/sch_edit_frame.h b/eeschema/sch_edit_frame.h index c27eca07ae..d79fd8b97b 100644 --- a/eeschema/sch_edit_frame.h +++ b/eeschema/sch_edit_frame.h @@ -132,7 +132,6 @@ public: void ReCreateHToolbar() override; void ReCreateVToolbar() override; void ReCreateOptToolbar() override; - void ReCreateMenuBar() override; void setupUIConditions() override; @@ -837,6 +836,8 @@ protected: */ bool doAutoSave() override; + void doReCreateMenuBar() override; + /** * Send the KiCad netlist over to CVPCB. */ diff --git a/eeschema/symbol_editor/menubar_symbol_editor.cpp b/eeschema/symbol_editor/menubar_symbol_editor.cpp index 2c70a39e1b..2353cf18d0 100644 --- a/eeschema/symbol_editor/menubar_symbol_editor.cpp +++ b/eeschema/symbol_editor/menubar_symbol_editor.cpp @@ -34,7 +34,7 @@ #include -void SYMBOL_EDIT_FRAME::ReCreateMenuBar() +void SYMBOL_EDIT_FRAME::doReCreateMenuBar() { EE_SELECTION_TOOL* selTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/eeschema/symbol_editor/symbol_edit_frame.h b/eeschema/symbol_editor/symbol_edit_frame.h index 2a6e05a7a4..db80cfc925 100644 --- a/eeschema/symbol_editor/symbol_edit_frame.h +++ b/eeschema/symbol_editor/symbol_edit_frame.h @@ -118,8 +118,6 @@ public: SELECTION& GetCurrentSelection() override; - void ReCreateMenuBar() override; - // See comments for m_SyncPinEdit. bool SynchronizePins(); @@ -392,6 +390,8 @@ public: protected: void setupUIConditions() override; + void doReCreateMenuBar() override; + private: // Set up the tool framework void setupTools(); diff --git a/eeschema/symbol_viewer_frame.h b/eeschema/symbol_viewer_frame.h index 45821145e6..154b837a26 100644 --- a/eeschema/symbol_viewer_frame.h +++ b/eeschema/symbol_viewer_frame.h @@ -93,7 +93,6 @@ public: void ReCreateHToolbar() override; void ReCreateVToolbar() override; void ReCreateOptToolbar() override {} - void ReCreateMenuBar() override; void ClickOnLibList( wxCommandEvent& event ); void ClickOnSymbolList( wxCommandEvent& event ); @@ -145,6 +144,8 @@ public: protected: void setupUIConditions() override; + void doReCreateMenuBar() override; + private: // Set up the tool framework. void setupTools(); diff --git a/eeschema/toolbars_symbol_viewer.cpp b/eeschema/toolbars_symbol_viewer.cpp index cef3c79906..ab04910303 100644 --- a/eeschema/toolbars_symbol_viewer.cpp +++ b/eeschema/toolbars_symbol_viewer.cpp @@ -97,7 +97,7 @@ void SYMBOL_VIEWER_FRAME::ReCreateVToolbar() } -void SYMBOL_VIEWER_FRAME::ReCreateMenuBar() +void SYMBOL_VIEWER_FRAME::doReCreateMenuBar() { SYMBOL_EDITOR_CONTROL* libControl = m_toolManager->GetTool(); // wxWidgets handles the OSX Application menu behind the scenes, but that means diff --git a/gerbview/gerbview_frame.h b/gerbview/gerbview_frame.h index 016fb23fc2..92d4cb2081 100644 --- a/gerbview/gerbview_frame.h +++ b/gerbview/gerbview_frame.h @@ -87,7 +87,6 @@ public: */ void ReCreateOptToolbar() override; - void ReCreateMenuBar() override; void UpdateStatusBar() override; void UpdateToolbarControlSizes() override; @@ -474,6 +473,7 @@ public: protected: void setupUIConditions() override; + void doReCreateMenuBar() override; private: void updateComponentListSelectBox(); diff --git a/gerbview/menubar.cpp b/gerbview/menubar.cpp index 26e2cc6ae6..1142945053 100644 --- a/gerbview/menubar.cpp +++ b/gerbview/menubar.cpp @@ -38,7 +38,7 @@ #include -void GERBVIEW_FRAME::ReCreateMenuBar() +void GERBVIEW_FRAME::doReCreateMenuBar() { GERBVIEW_SELECTION_TOOL* selTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/include/eda_base_frame.h b/include/eda_base_frame.h index 9e3894f8ab..f57f21e986 100644 --- a/include/eda_base_frame.h +++ b/include/eda_base_frame.h @@ -435,7 +435,7 @@ public: * * Needed when the language or icons are changed */ - virtual void ReCreateMenuBar(); + void ReCreateMenuBar(); /** * Adds the standard KiCad help menu to the menubar. @@ -602,6 +602,8 @@ protected: return wxT( "_autosave-" ); } + virtual void doReCreateMenuBar() {} + /** * Handle the auto save timer event. */ diff --git a/include/eda_draw_frame.h b/include/eda_draw_frame.h index e2c2ec3046..1225ad0508 100644 --- a/include/eda_draw_frame.h +++ b/include/eda_draw_frame.h @@ -195,7 +195,6 @@ public: void EraseMsgBox(); - void ReCreateMenuBar() override { } virtual void ReCreateHToolbar() = 0; virtual void ReCreateVToolbar() = 0; virtual void ReCreateOptToolbar() = 0; @@ -460,6 +459,8 @@ protected: void setupUnits( APP_SETTINGS_BASE* aCfg ); + void doReCreateMenuBar() override { } + std::vector findDialogs(); /** diff --git a/include/pcb_base_frame.h b/include/pcb_base_frame.h index aa6560a0e3..29ef8848dc 100644 --- a/include/pcb_base_frame.h +++ b/include/pcb_base_frame.h @@ -210,7 +210,6 @@ public: // General virtual void ReCreateOptToolbar() override { } virtual void ShowChangedLanguage() override; - virtual void ReCreateMenuBar() override; virtual void UpdateStatusBar() override; PCB_SCREEN* GetScreen() const override { return (PCB_SCREEN*) EDA_DRAW_FRAME::GetScreen(); } @@ -384,6 +383,8 @@ protected: void handleIconizeEvent( wxIconizeEvent& aEvent ) override; + virtual void doReCreateMenuBar() override; + /** * Attempts to load \a aFootprintId from the footprint library table. * diff --git a/kicad/kicad_manager_frame.h b/kicad/kicad_manager_frame.h index c0ce0faf6f..a74aea651c 100644 --- a/kicad/kicad_manager_frame.h +++ b/kicad/kicad_manager_frame.h @@ -63,7 +63,6 @@ public: void OnClearFileHistory( wxCommandEvent& aEvent ); void OnExit( wxCommandEvent& event ); - void ReCreateMenuBar() override; void RecreateBaseHToolbar(); wxString GetCurrentFileName() const override @@ -159,6 +158,8 @@ public: protected: virtual void setupUIConditions() override; + void doReCreateMenuBar() override; + private: void setupTools(); void setupActions(); diff --git a/kicad/menubar.cpp b/kicad/menubar.cpp index e07c8f9ba8..c287e5aea2 100644 --- a/kicad/menubar.cpp +++ b/kicad/menubar.cpp @@ -42,7 +42,7 @@ #include -void KICAD_MANAGER_FRAME::ReCreateMenuBar() +void KICAD_MANAGER_FRAME::doReCreateMenuBar() { KICAD_MANAGER_CONTROL* controlTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/pagelayout_editor/menubar.cpp b/pagelayout_editor/menubar.cpp index 62f7aad2c5..ae70101a4f 100644 --- a/pagelayout_editor/menubar.cpp +++ b/pagelayout_editor/menubar.cpp @@ -38,7 +38,7 @@ #include "tools/pl_selection_tool.h" -void PL_EDITOR_FRAME::ReCreateMenuBar() +void PL_EDITOR_FRAME::doReCreateMenuBar() { PL_SELECTION_TOOL* selTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/pagelayout_editor/pl_editor_frame.h b/pagelayout_editor/pl_editor_frame.h index 2cf8541227..3c969d02bb 100644 --- a/pagelayout_editor/pl_editor_frame.h +++ b/pagelayout_editor/pl_editor_frame.h @@ -142,8 +142,6 @@ public: */ void ReCreateOptToolbar() override; - void ReCreateMenuBar() override; - const PL_EDITOR_LAYOUT& GetPageLayout() const { return m_pageLayout; } PL_EDITOR_LAYOUT& GetPageLayout() { return m_pageLayout; } @@ -276,6 +274,8 @@ protected: void setupUIConditions() override; + void doReCreateMenuBar() override; + DECLARE_EVENT_TABLE(); /// The last filename chosen to be proposed to the user diff --git a/pcbnew/footprint_edit_frame.h b/pcbnew/footprint_edit_frame.h index 2f34c2d44b..0819d00bd9 100644 --- a/pcbnew/footprint_edit_frame.h +++ b/pcbnew/footprint_edit_frame.h @@ -117,11 +117,6 @@ public: void ReCreateOptToolbar() override; void UpdateToolbarControlSizes() override; - /** - * @brief (Re)Create the menubar for the Footprint Editor frame - */ - void ReCreateMenuBar() override; - /** * Re create the layer Box by clearing the old list, and building a new one from the new * layers names and layer colors.. @@ -339,6 +334,11 @@ protected: void restoreLastFootprint(); void retainLastFootprint(); + /** + * @brief (Re)Create the menubar for the Footprint Editor frame + */ + void doReCreateMenuBar() override; + /** * Run the Footprint Properties dialog and handle changes made in it. */ diff --git a/pcbnew/footprint_viewer_frame.h b/pcbnew/footprint_viewer_frame.h index 9b6f76790d..f66a3db244 100644 --- a/pcbnew/footprint_viewer_frame.h +++ b/pcbnew/footprint_viewer_frame.h @@ -91,6 +91,8 @@ protected: void setupUIConditions() override; + void doReCreateMenuBar() override; + private: const wxString getCurNickname(); void setCurNickname( const wxString& aNickname ); @@ -117,7 +119,6 @@ private: void ReCreateHToolbar() override; void ReCreateVToolbar() override; void ReCreateOptToolbar() override; - void ReCreateMenuBar() override; void OnLibFilter( wxCommandEvent& aEvent ); void OnFPFilter( wxCommandEvent& aEvent ); diff --git a/pcbnew/menubar_footprint_editor.cpp b/pcbnew/menubar_footprint_editor.cpp index 220e24a26f..aee503675f 100644 --- a/pcbnew/menubar_footprint_editor.cpp +++ b/pcbnew/menubar_footprint_editor.cpp @@ -36,7 +36,7 @@ #include -void FOOTPRINT_EDIT_FRAME::ReCreateMenuBar() +void FOOTPRINT_EDIT_FRAME::doReCreateMenuBar() { PCB_SELECTION_TOOL* selTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/pcbnew/menubar_pcb_editor.cpp b/pcbnew/menubar_pcb_editor.cpp index 7e6050b91d..9f6fbeb860 100644 --- a/pcbnew/menubar_pcb_editor.cpp +++ b/pcbnew/menubar_pcb_editor.cpp @@ -40,7 +40,7 @@ #include -void PCB_EDIT_FRAME::ReCreateMenuBar() +void PCB_EDIT_FRAME::doReCreateMenuBar() { PCB_SELECTION_TOOL* selTool = m_toolManager->GetTool(); // wxWidgets handles the Mac Application menu behind the scenes, but that means diff --git a/pcbnew/pcb_base_frame.cpp b/pcbnew/pcb_base_frame.cpp index 77aa53f7a1..39155280fd 100644 --- a/pcbnew/pcb_base_frame.cpp +++ b/pcbnew/pcb_base_frame.cpp @@ -665,7 +665,7 @@ BOX2I PCB_BASE_FRAME::GetBoardBoundingBox( bool aBoardEdgesOnly ) const // Virtual function -void PCB_BASE_FRAME::ReCreateMenuBar() +void PCB_BASE_FRAME::doReCreateMenuBar() { } diff --git a/pcbnew/pcb_edit_frame.h b/pcbnew/pcb_edit_frame.h index 7cda7fd492..6893813ffc 100644 --- a/pcbnew/pcb_edit_frame.h +++ b/pcbnew/pcb_edit_frame.h @@ -245,7 +245,6 @@ public: void ReCreateAuxiliaryToolbar() override; void ReCreateVToolbar() override; void ReCreateOptToolbar() override; - void ReCreateMenuBar() override; void UpdateToolbarControlSizes() override; /** @@ -726,6 +725,8 @@ protected: LAYER_TOOLBAR_ICON_VALUES m_prevIconVal; + void doReCreateMenuBar() override; + // The Tool Framework initialization void setupTools(); void setupUIConditions() override; diff --git a/pcbnew/toolbars_footprint_viewer.cpp b/pcbnew/toolbars_footprint_viewer.cpp index 5faab2fbdc..9d8ffd5fa4 100644 --- a/pcbnew/toolbars_footprint_viewer.cpp +++ b/pcbnew/toolbars_footprint_viewer.cpp @@ -159,7 +159,7 @@ void FOOTPRINT_VIEWER_FRAME::ReCreateVToolbar() } -void FOOTPRINT_VIEWER_FRAME::ReCreateMenuBar() +void FOOTPRINT_VIEWER_FRAME::doReCreateMenuBar() { PCB_SELECTION_TOOL* selTool = m_toolManager->GetTool();