Re: Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126385/ --- (Updated Jan. 6, 2016, 6:04 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 5ac5dfb28014035e3704ae8bf8076001e5955ceb by Jonathan Marten to branch master. Bugs: 355711 https://bugs.kde.org/show_bug.cgi?id=355711 Repository: kparts Description --- This appears to be the cause of a crash when exiting System Settings. More information in the bug report, but basically what happens is that the manager keeps track of widgets that it is managing in d->m_managedTopLevelWidgets. If a widget is a top level widget when it is added, but is no longer top level when it is destroyed, it is not removed from the list which results in an access-to-deleted-object in the destructor. This change unconditionally removes the widget even if it is no longer top level. Removing the widget from the list has no ill effects, the list is only actually used in Partmanager::eventFilter() which will never get an event for a deleted widget anyway. Aside: The problematic 'foreach (const QWidget *w, d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really superfluous, since all signal connections to 'this' are removed on destruction anyway. Diffs - src/partmanager.cpp 81bf73f Diff: https://git.reviewboard.kde.org/r/126385/diff/ Testing --- Built KParts with this change, and also systemsettings5 with the associated change (see associated review). Observed no crash when exiting the application. Also checked correct operation of Konqueror and Kate. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126385/#review90394 --- Ship it! I agree -- except about the "aside" in the commit log: "since all signal connections to 'this' are removed on destruction anyway". They are indeed, but *after* destroyed is emitted. So disconnecting does make a difference. - David Faure On Dec. 16, 2015, 1:33 p.m., Jonathan Marten wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126385/ > --- > > (Updated Dec. 16, 2015, 1:33 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 355711 > https://bugs.kde.org/show_bug.cgi?id=355711 > > > Repository: kparts > > > Description > --- > > This appears to be the cause of a crash when exiting System Settings. More > information in the bug report, but basically what happens is that the manager > keeps track of widgets that it is managing in d->m_managedTopLevelWidgets. > If a widget is a top level widget when it is added, but is no longer top > level when it is destroyed, it is not removed from the list which results in > an access-to-deleted-object in the destructor. > > This change unconditionally removes the widget even if it is no longer top > level. Removing the widget from the list has no ill effects, the list is > only actually used in Partmanager::eventFilter() which will never get an > event for a deleted widget anyway. > > Aside: The problematic 'foreach (const QWidget *w, > d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really > superfluous, since all signal connections to 'this' are removed on > destruction anyway. > > > Diffs > - > > src/partmanager.cpp 81bf73f > > Diff: https://git.reviewboard.kde.org/r/126385/diff/ > > > Testing > --- > > Built KParts with this change, and also systemsettings5 with the associated > change (see associated review). Observed no crash when exiting the > application. Also checked correct operation of Konqueror and Kate. > > > Thanks, > > Jonathan Marten > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126385/ --- Review request for KDE Frameworks. Bugs: 355711 https://bugs.kde.org/show_bug.cgi?id=355711 Repository: kparts Description --- This appears to be the cause of a crash when exiting System Settings. More information in the bug report, but basically what happens is that the manager keeps track of widgets that it is managing in d->m_managedTopLevelWidgets. If a widget is a top level widget when it is added, but is no longer top level when it is destroyed, it is not removed from the list which results in an access-to-deleted-object in the destructor. This change unconditionally removes the widget even if it is no longer top level. Removing the widget from the list has no ill effects, the list is only actually used in Partmanager::eventFilter() which will never get an event for a deleted widget anyway. Aside: The problematic 'foreach (const QWidget *w, d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really superfluous, since all signal connections to 'this' are removed on destruction anyway. Diffs - src/partmanager.cpp 81bf73f Diff: https://git.reviewboard.kde.org/r/126385/diff/ Testing --- Built KParts with this change, and also systemsettings5 with the associated change (see associated review). Observed no crash when exiting the application. Also checked correct operation of Konqueror and Kate. Thanks, Jonathan Marten ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
