Merge branch 'wxQt-memory-leaks' of https://github.com/catalinr/wxWidgets

Fix many memory leaks in wxQt port.

See https://github.com/wxWidgets/wxWidgets/pull/1243

include/wx/qt/accel.h

@@ -43,7 +43,7 @@ public:
     wxAcceleratorTable(int n, const wxAcceleratorEntry entries[]);
 
     // Implementation
-    QList < QShortcut* > *ConvertShortcutTable( QWidget *parent ) const;
+    wxVector<QShortcut*> ConvertShortcutTable( QWidget *parent ) const;
 
     bool Ok() const { return IsOk(); }
     bool IsOk() const;

include/wx/qt/app.h

@@ -9,6 +9,8 @@
 #ifndef _WX_QT_APP_H_
 #define _WX_QT_APP_H_
 
+#include <wx/scopedarray.h>
+
 class QApplication;
 class WXDLLIMPEXP_CORE wxApp : public wxAppBase
 {
@@ -19,9 +21,9 @@ public:
     virtual bool Initialize(int& argc, wxChar **argv);
 
 private:
-    QApplication *m_qtApplication;
+    wxScopedPtr<QApplication> m_qtApplication;
     int m_qtArgc;
-    char **m_qtArgv;
+    wxScopedArray<char*> m_qtArgv;
 
     wxDECLARE_DYNAMIC_CLASS_NO_COPY( wxApp );
 };

include/wx/qt/statusbar.h

@@ -45,7 +45,7 @@ private:
     void UpdateFields();
 
     QStatusBar *m_qtStatusBar;
-    QList< QLabel* > *m_qtPanes;
+    wxVector<QLabel*> m_qtPanes;
 
     wxDECLARE_DYNAMIC_CLASS(wxStatusBar);
 };

include/wx/qt/window.h

@@ -216,24 +216,24 @@ protected:
 
 private:
     void Init();
-    QScrollArea *m_qtContainer;
+    QScrollArea *m_qtContainer;  // either NULL or the same as m_qtWindow pointer
 
-    QScrollBar *m_horzScrollBar;
+    QScrollBar *m_horzScrollBar; // owned by m_qtWindow when allocated
-    QScrollBar *m_vertScrollBar;
+    QScrollBar *m_vertScrollBar; // owned by m_qtWindow when allocated
 
     QScrollBar *QtGetScrollBar( int orientation ) const;
     QScrollBar *QtSetScrollBar( int orientation, QScrollBar *scrollBar=NULL );
 
     bool QtSetBackgroundStyle();
 
-    QPicture *m_qtPicture;
+    QPicture *m_qtPicture;                                   // not owned
-    QPainter *m_qtPainter;
+    wxScopedPtr<QPainter> m_qtPainter;                       // always allocated
 
     bool m_mouseInside;
 
 #if wxUSE_ACCEL
-    QList< QShortcut* > *m_qtShortcuts;
+    wxVector<QShortcut*> m_qtShortcuts; // owned by whatever GetHandle() returns
-    wxQtShortcutHandler *m_qtShortcutHandler;
+    wxScopedPtr<wxQtShortcutHandler> m_qtShortcutHandler;    // always allocated
     bool m_processingShortcut;
 #endif // wxUSE_ACCEL
 

samples/minimal/minimal.cpp

@@ -170,6 +170,7 @@ MyFrame::MyFrame(const wxString& title)
     wxButton* aboutBtn = new wxButton(this, wxID_ANY, "About...");
     aboutBtn->Bind(wxEVT_BUTTON, &MyFrame::OnAbout, this);
     sizer->Add(aboutBtn, wxSizerFlags().Center());
+    SetSizer(sizer);
 #endif // wxUSE_MENUS/!wxUSE_MENUS
 
 #if wxUSE_STATUSBAR

src/qt/accel.cpp

@@ -82,16 +82,17 @@ wxAcceleratorTable::wxAcceleratorTable(int n, const wxAcceleratorEntry entries[]
     }
 }
 
-QList< QShortcut* > *wxAcceleratorTable::ConvertShortcutTable( QWidget *parent ) const
+wxVector<QShortcut*> wxAcceleratorTable::ConvertShortcutTable( QWidget *parent ) const
 {
-    QList< QShortcut* > *qtList = new QList< QShortcut* >;
+    wxVector<QShortcut*> shortcuts;
 
-    for ( wxAccelList::compatibility_iterator node = M_ACCELDATA->m_accels.GetFirst(); node; node = node->GetNext() )
+    for ( wxAccelList::compatibility_iterator node = M_ACCELDATA->m_accels.GetFirst();
+          node; node = node->GetNext() )
     {
-        qtList->push_back(ConvertAccelerator( node->GetData(), parent ));
+        shortcuts.push_back(ConvertAccelerator(node->GetData(), parent));
     }
 
-    return qtList;
+    return shortcuts;
 }
 
 wxObjectRefData *wxAcceleratorTable::CreateRefData() const

src/qt/app.cpp

@@ -19,19 +19,16 @@ wxIMPLEMENT_DYNAMIC_CLASS(wxApp, wxEvtHandler);
 
 wxApp::wxApp()
 {
-    m_qtApplication = NULL;
     m_qtArgc = 0;
-    m_qtArgv = NULL;
 }
 
 
 wxApp::~wxApp()
 {
-    // Only delete if the app was actually initialized
+    // Delete command line arguments
-    if ( m_qtApplication != NULL )
+    for ( int i = 0; i < m_qtArgc; ++i )
     {
-        m_qtApplication->deleteLater();
+        delete m_qtArgv[i];
-        delete [] m_qtArgv;
     }
 }
 
@@ -49,7 +46,7 @@ bool wxApp::Initialize( int &argc, wxChar **argv )
     // TODO: Check whether new/strdup etc. can be replaced with std::vector<>.
 
     // Clone and store arguments
-    m_qtArgv = new char *[argc + 1];
+    m_qtArgv.reset(new char* [argc + 1]);
     for ( int i = 0; i < argc; i++ )
     {
         m_qtArgv[i] = wxStrdupA(wxConvUTF8.cWX2MB(argv[i]));
@@ -57,7 +54,7 @@ bool wxApp::Initialize( int &argc, wxChar **argv )
     m_qtArgv[argc] = NULL;
     m_qtArgc = argc;
 
-    m_qtApplication = new QApplication( m_qtArgc, m_qtArgv );
+    m_qtApplication.reset(new QApplication(m_qtArgc, m_qtArgv.get()));
 
     // Use the args returned by Qt as it may have deleted (processed) some of them
     // Using QApplication::arguments() forces argument processing

src/qt/choice.cpp

@@ -20,6 +20,8 @@ namespace
 class LexicalSortProxyModel : public QSortFilterProxyModel
 {
 public:
+    explicit LexicalSortProxyModel(QObject* owner) : QSortFilterProxyModel(owner) {}
+
     bool lessThan( const QModelIndex &left, const QModelIndex &right ) const wxOVERRIDE
     {
         const QVariant leftData = sourceModel()->data( left );
@@ -77,7 +79,7 @@ wxChoice::wxChoice() :
 
 void wxChoice::QtInitSort( QComboBox *combo )
 {
-    QSortFilterProxyModel *proxyModel = new LexicalSortProxyModel();
+    QSortFilterProxyModel *proxyModel = new LexicalSortProxyModel(combo);
     proxyModel->setSourceModel(combo->model());
     combo->model()->setParent(proxyModel);
     combo->setModel(proxyModel);

src/qt/dc.cpp

@@ -49,6 +49,7 @@ wxQtDCImpl::wxQtDCImpl( wxDC *owner )
     : wxDCImpl( owner )
 {
     m_qtPixmap = NULL;
+    m_qtPainter = NULL;
     m_rasterColourOp = wxQtNONE;
     m_qtPenColor = new QColor;
     m_qtBrushColor = new QColor;

src/qt/dcclient.cpp

@@ -90,8 +90,6 @@ wxIMPLEMENT_CLASS(wxClientDCImpl,wxWindowDCImpl);
 wxClientDCImpl::wxClientDCImpl( wxDC *owner )
     : wxWindowDCImpl( owner )
 {
-    m_window = NULL;
-    m_qtPainter = NULL;
 }
 
 wxClientDCImpl::wxClientDCImpl( wxDC *owner, wxWindow *win )
@@ -99,8 +97,6 @@ wxClientDCImpl::wxClientDCImpl( wxDC *owner, wxWindow *win )
 {
     m_window = win;
 
-    m_qtPainter = new QPainter();
-
     m_pict.reset(new QPicture());
     m_ok = m_qtPainter->begin( m_pict.get() );
 

src/qt/menu.cpp

@@ -88,6 +88,12 @@ static void InsertMenuItemAction( const wxMenu *menu, const wxMenuItem *previous
             break;
         case wxITEM_NORMAL:
         {
+            // If the inserted action is a submenu, set the owner for the submenu.
+            if ( item->IsSubMenu() )
+            {
+                item->GetSubMenu()->GetHandle()->setParent(qtMenu, Qt::Popup);
+            }
+
             wxWindowID id = item->GetId();
             if ( wxIsStockID( id ) )
             {
@@ -113,6 +119,9 @@ static void InsertMenuItemAction( const wxMenu *menu, const wxMenuItem *previous
     // Insert the action into the actual menu:
     QAction *successiveItemAction = ( successiveItem != NULL ) ? successiveItem->GetHandle() : NULL;
     qtMenu->insertAction( successiveItemAction, itemAction );
+    // Menu items in Qt can be part of multiple menus, so a menu will not take ownership
+    // when one is added to it. Take it explicitly, otherwise it will create a memory leak.
+    itemAction->setParent(qtMenu);
 }
 
 wxMenuItem *wxMenu::DoAppend(wxMenuItem *item)
@@ -211,6 +220,9 @@ bool wxMenuBar::Append( wxMenu *menu, const wxString& title )
 
     QMenu *qtMenu = SetTitle( menu, title );
     m_qtMenuBar->addMenu( qtMenu );
+    // Menus in Qt can be reused as popups, so a menu bar will not take ownership when
+    // one is added to it. Take it explicitly, otherwise there will be a memory leak.
+    qtMenu->setParent(m_qtMenuBar, Qt::Popup); // must specify window type for correct display!
 
     return true;
 }
@@ -233,6 +245,7 @@ bool wxMenuBar::Insert(size_t pos, wxMenu *menu, const wxString& title)
     QMenu *qtMenu = SetTitle( menu, title );
     QAction *qtAction = GetActionAt( m_qtMenuBar, pos );
     m_qtMenuBar->insertMenu( qtAction, qtMenu );
+    qtMenu->setParent(m_qtMenuBar, Qt::Popup); // must specify window type for correct display!
 
     return true;
 }

src/qt/statusbar.cpp

@@ -48,7 +48,6 @@ bool wxStatusBar::Create(wxWindow *parent, wxWindowID WXUNUSED(winid),
                          long style, const wxString& WXUNUSED(name))
 {
     m_qtStatusBar = new wxQtStatusBar( parent, this );
-    m_qtPanes = new QList < QLabel* >;
 
     if ( style & wxSTB_SIZEGRIP )
         m_qtStatusBar->setSizeGripEnabled(true);
@@ -65,10 +64,10 @@ bool wxStatusBar::GetFieldRect(int i, wxRect& rect) const
     wxCHECK_MSG( (i >= 0) && ((size_t)i < m_panes.GetCount()), false,
                  "invalid statusbar field index" );
 
-    if ( static_cast<size_t>(m_qtPanes->count()) != m_panes.GetCount() )
+    if ( m_qtPanes.size() != m_panes.GetCount() )
         const_cast<wxStatusBar*>(this)->UpdateFields();
 
-    rect = wxQtConvertRect((*m_qtPanes)[i]->geometry());
+    rect = wxQtConvertRect(m_qtPanes[i]->geometry());
     return true;
 }
 
@@ -89,10 +88,10 @@ int wxStatusBar::GetBorderY() const
 
 void wxStatusBar::DoUpdateStatusText(int number)
 {
-    if ( static_cast<size_t>(m_qtPanes->count()) != m_panes.GetCount() )
+    if ( m_qtPanes.size() != m_panes.GetCount() )
         UpdateFields();
 
-    (*m_qtPanes)[number]->setText( wxQtConvertString( m_panes[number].GetText() ) );
+    m_qtPanes[number]->setText( wxQtConvertString( m_panes[number].GetText() ) );
 }
 
 // Called each time number/size of panes changes
@@ -106,18 +105,19 @@ void wxStatusBar::Refresh( bool eraseBackground, const wxRect *rect )
 void wxStatusBar::Init()
 {
     m_qtStatusBar = NULL;
-    m_qtPanes = NULL;
 }
 
 void wxStatusBar::UpdateFields()
 {
     // is it a good idea to recreate all the panes every update?
 
-    while ( !m_qtPanes->isEmpty() )
+    for ( wxVector<QLabel*>::const_iterator it = m_qtPanes.begin();
+          it != m_qtPanes.end(); ++it )
     {
         //Remove all panes
-        delete m_qtPanes->takeLast();
+        delete *it;
     }
+    m_qtPanes.clear();
 
     for (size_t i = 0; i < m_panes.GetCount(); i++)
     {
@@ -125,7 +125,7 @@ void wxStatusBar::UpdateFields()
         int width = m_panes[i].GetWidth();
 
         QLabel *pane = new QLabel( m_qtStatusBar );
-        m_qtPanes->append( pane );
+        m_qtPanes.push_back(pane);
 
         if ( width >= 0 )
         {

src/qt/window.cpp

@@ -211,19 +211,16 @@ void wxWindowQt::Init()
     m_vertScrollBar = NULL;
 
     m_qtPicture = NULL;
-    m_qtPainter = new QPainter();
+    m_qtPainter.reset(new QPainter());
 
     m_mouseInside = false;
 
 #if wxUSE_ACCEL
-    m_qtShortcutHandler = new wxQtShortcutHandler( this );
+    m_qtShortcutHandler.reset(new wxQtShortcutHandler(this));
     m_processingShortcut = false;
-    m_qtShortcuts = NULL;
 #endif
     m_qtWindow = NULL;
     m_qtContainer = NULL;
-
-    m_dropTarget = NULL;
 }
 
 wxWindowQt::wxWindowQt()
@@ -251,13 +248,6 @@ wxWindowQt::~wxWindowQt()
 
     DestroyChildren(); // This also destroys scrollbars
 
-    delete m_qtPainter;
-
-#if wxUSE_ACCEL
-    m_qtShortcutHandler->deleteLater();
-    delete m_qtShortcuts;
-#endif
-
 #if wxUSE_DRAG_AND_DROP
     SetDropTarget(NULL);
 #endif
@@ -266,23 +256,8 @@ wxWindowQt::~wxWindowQt()
     if (m_qtWindow)
     {
         wxLogTrace(TRACE_QT_WINDOW, wxT("wxWindow::~wxWindow %s m_qtWindow=%p"), GetName(), m_qtWindow);
-        // Avoid sending further signals (i.e. if deleting the current page)
+
-        m_qtWindow->blockSignals(true);
+        delete m_qtWindow;
-        // Reset the pointer to avoid handling pending event and signals
-        QtStoreWindowPointer( GetHandle(), NULL );
-        if ( m_horzScrollBar )
-        {
-            QtStoreWindowPointer( m_horzScrollBar, NULL );
-            m_horzScrollBar->deleteLater();
-        }
-        if ( m_vertScrollBar )
-        {
-            QtStoreWindowPointer( m_vertScrollBar, NULL );
-            m_vertScrollBar->deleteLater();
-        }
-        // Delete QWidget when control return to event loop (safer)
-        m_qtWindow->deleteLater();
-        m_qtWindow = NULL;
     }
     else
     {
@@ -622,6 +597,8 @@ QScrollBar *wxWindowQt::QtSetScrollBar( int orientation, QScrollBar *scrollBar )
 
 void wxWindowQt::SetScrollbar( int orientation, int pos, int thumbvisible, int range, bool WXUNUSED(refresh) )
 {
+    wxCHECK_RET(GetHandle(), "Window has not been created");
+
     //If not exist, create the scrollbar
     QScrollBar *scrollBar = QtGetScrollBar( orientation );
     if ( scrollBar == NULL )
@@ -706,6 +683,7 @@ void wxWindowQt::SetDropTarget( wxDropTarget *dropTarget )
     if ( m_dropTarget != NULL )
     {
         m_dropTarget->Disconnect();
+        delete m_dropTarget;
     }
 
     m_dropTarget = dropTarget;
@@ -1006,26 +984,25 @@ bool wxWindowQt::DoPopupMenu(wxMenu *menu, int x, int y)
 #if wxUSE_ACCEL
 void wxWindowQt::SetAcceleratorTable( const wxAcceleratorTable& accel )
 {
+    wxCHECK_RET(GetHandle(), "Window has not been created");
+
     wxWindowBase::SetAcceleratorTable( accel );
 
-    if ( m_qtShortcuts )
+    // Disable previously set accelerators
+    for ( wxVector<QShortcut*>::const_iterator it = m_qtShortcuts.begin();
+          it != m_qtShortcuts.end(); ++it )
     {
-        // Disable previously set accelerators
+        delete *it;
-        while ( !m_qtShortcuts->isEmpty() )
-            delete m_qtShortcuts->takeFirst();
-
-        // Create new shortcuts (use GetHandle() so all events inside
-        // the window are handled, not only in the container subwindow)
-        delete m_qtShortcuts;
     }
 
-    m_qtShortcuts = accel.ConvertShortcutTable( GetHandle() );
+    m_qtShortcuts = accel.ConvertShortcutTable(GetHandle());
 
     // Connect shortcuts to window
-    Q_FOREACH( QShortcut *s, *m_qtShortcuts )
+    for ( wxVector<QShortcut*>::const_iterator it = m_qtShortcuts.begin();
+          it != m_qtShortcuts.end(); ++it )
     {
-        QObject::connect( s, &QShortcut::activated, m_qtShortcutHandler, &wxQtShortcutHandler::activated );
+        QObject::connect( *it, &QShortcut::activated, m_qtShortcutHandler.get(), &wxQtShortcutHandler::activated );
-        QObject::connect( s, &QShortcut::activatedAmbiguously, m_qtShortcutHandler, &wxQtShortcutHandler::activated );
+        QObject::connect( *it, &QShortcut::activatedAmbiguously, m_qtShortcutHandler.get(), &wxQtShortcutHandler::activated );
     }
 }
 #endif // wxUSE_ACCEL
@@ -1191,7 +1168,7 @@ bool wxWindowQt::QtHandlePaintEvent ( QWidget *handler, QPaintEvent *event )
             else
             {
                 // Data from wxClientDC, paint it
-                m_qtPicture->play( m_qtPainter );
+                m_qtPicture->play( m_qtPainter.get() );
                 // Reset picture
                 m_qtPicture->setData( NULL, 0 );
                 handled = true;
@@ -1574,5 +1551,5 @@ void wxWindowQt::QtSetPicture( QPicture* pict )
 
 QPainter *wxWindowQt::QtGetPainter()
 {
-    return m_qtPainter;
+    return m_qtPainter.get();
 }