Re: Review Request 114895: Guard against null QX11Info::connection()
On Jan. 7, 2014, 3:14 p.m., Martin Gräßlin wrote: checking obviously makes sense, though it shouldn't be needed. There must be something else which is wrong here, too. Could you try what the value of WId is in these cases? I wouldn't be surprised if it were 0. Oh and that code has unit tests, so I would appreciate if you extend the tests for that case. David Edmundson wrote: WId seems to be valid. If I check the dialog with xwininfo before closing plasmoidviewer it shows the same ID. Here is a full backtrace of it being needed: http://pastebin.kde.org/pxjhgw95d I can guard against it inside plasma with if (!QApplication::closingDown()) around the KWindowEffects calls. I changed to guarding in the library as I can imagine others hitting it in the future and in general library code shouldn't crash on reasonable inputs. Martin Gräßlin wrote: The backtrace includes QWindow::destroy which as the name says will do an xcb_destroy_window. After that the DialogProxy::onVisibleChanged will be invoked. At that point the window doesn't exist any more so the X calls are just wrong. I'd say this needs fixing in DialogProxy to not call the X specific calls for a destroyed window. Then one could think about whether invoking the methods without a valid xcb_connection is supposed to work. I'd say we should add asserts there instead of the null checks. What do you think? I just had a look at the QWindow implementation and whether it could provide us a useable way to figure out whether there is a window at all. Unfortunately it doesn't. So ShipIt! - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/#review46967 --- On Jan. 7, 2014, 2:57 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 7, 2014, 2:57 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114895: Guard against null QX11Info::connection()
On Jan. 7, 2014, 2:14 p.m., Martin Gräßlin wrote: checking obviously makes sense, though it shouldn't be needed. There must be something else which is wrong here, too. Could you try what the value of WId is in these cases? I wouldn't be surprised if it were 0. Oh and that code has unit tests, so I would appreciate if you extend the tests for that case. David Edmundson wrote: WId seems to be valid. If I check the dialog with xwininfo before closing plasmoidviewer it shows the same ID. Here is a full backtrace of it being needed: http://pastebin.kde.org/pxjhgw95d I can guard against it inside plasma with if (!QApplication::closingDown()) around the KWindowEffects calls. I changed to guarding in the library as I can imagine others hitting it in the future and in general library code shouldn't crash on reasonable inputs. Martin Gräßlin wrote: The backtrace includes QWindow::destroy which as the name says will do an xcb_destroy_window. After that the DialogProxy::onVisibleChanged will be invoked. At that point the window doesn't exist any more so the X calls are just wrong. I'd say this needs fixing in DialogProxy to not call the X specific calls for a destroyed window. Then one could think about whether invoking the methods without a valid xcb_connection is supposed to work. I'd say we should add asserts there instead of the null checks. What do you think? Martin Gräßlin wrote: I just had a look at the QWindow implementation and whether it could provide us a useable way to figure out whether there is a window at all. Unfortunately it doesn't. So ShipIt! I had a try at making a unit test but couldn't reproduce the crash I was seeing in Plasma; deleting the m_widget and m_window before calling KWindowEffect::whatever didn't seem to be enough. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/#review46967 --- On Jan. 7, 2014, 1:57 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 7, 2014, 1:57 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114895: Guard against null QX11Info::connection()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 8, 2014, 12:18 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114895: Guard against null QX11Info::connection()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/#review47047 --- This review has been submitted with commit 11775f9351b9fe944d522039400d0bb118fc4733 by David Edmundson to branch master. - Commit Hook On Jan. 7, 2014, 1:57 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 7, 2014, 1:57 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114895: Guard against null QX11Info::connection()
On Jan. 7, 2014, 3:14 p.m., Martin Gräßlin wrote: checking obviously makes sense, though it shouldn't be needed. There must be something else which is wrong here, too. Could you try what the value of WId is in these cases? I wouldn't be surprised if it were 0. Oh and that code has unit tests, so I would appreciate if you extend the tests for that case. David Edmundson wrote: WId seems to be valid. If I check the dialog with xwininfo before closing plasmoidviewer it shows the same ID. Here is a full backtrace of it being needed: http://pastebin.kde.org/pxjhgw95d I can guard against it inside plasma with if (!QApplication::closingDown()) around the KWindowEffects calls. I changed to guarding in the library as I can imagine others hitting it in the future and in general library code shouldn't crash on reasonable inputs. Martin Gräßlin wrote: The backtrace includes QWindow::destroy which as the name says will do an xcb_destroy_window. After that the DialogProxy::onVisibleChanged will be invoked. At that point the window doesn't exist any more so the X calls are just wrong. I'd say this needs fixing in DialogProxy to not call the X specific calls for a destroyed window. Then one could think about whether invoking the methods without a valid xcb_connection is supposed to work. I'd say we should add asserts there instead of the null checks. What do you think? Martin Gräßlin wrote: I just had a look at the QWindow implementation and whether it could provide us a useable way to figure out whether there is a window at all. Unfortunately it doesn't. So ShipIt! David Edmundson wrote: I had a try at making a unit test but couldn't reproduce the crash I was seeing in Plasma; deleting the m_widget and m_window before calling KWindowEffect::whatever didn't seem to be enough. I think it must be really shutting down and that might not be a testable case in the unit tests. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/#review46967 --- On Jan. 7, 2014, 2:57 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 7, 2014, 2:57 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114895: Guard against null QX11Info::connection()
On Jan. 7, 2014, 2:14 p.m., Martin Gräßlin wrote: checking obviously makes sense, though it shouldn't be needed. There must be something else which is wrong here, too. Could you try what the value of WId is in these cases? I wouldn't be surprised if it were 0. Oh and that code has unit tests, so I would appreciate if you extend the tests for that case. WId seems to be valid. If I check the dialog with xwininfo before closing plasmoidviewer it shows the same ID. Here is a full backtrace of it being needed: http://pastebin.kde.org/pxjhgw95d I can guard against it inside plasma with if (!QApplication::closingDown()) around the KWindowEffects calls. I changed to guarding in the library as I can imagine others hitting it in the future and in general library code shouldn't crash on reasonable inputs. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/#review46967 --- On Jan. 7, 2014, 1:57 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 7, 2014, 1:57 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 114895: Guard against null QX11Info::connection()
On Jan. 7, 2014, 3:14 p.m., Martin Gräßlin wrote: checking obviously makes sense, though it shouldn't be needed. There must be something else which is wrong here, too. Could you try what the value of WId is in these cases? I wouldn't be surprised if it were 0. Oh and that code has unit tests, so I would appreciate if you extend the tests for that case. David Edmundson wrote: WId seems to be valid. If I check the dialog with xwininfo before closing plasmoidviewer it shows the same ID. Here is a full backtrace of it being needed: http://pastebin.kde.org/pxjhgw95d I can guard against it inside plasma with if (!QApplication::closingDown()) around the KWindowEffects calls. I changed to guarding in the library as I can imagine others hitting it in the future and in general library code shouldn't crash on reasonable inputs. The backtrace includes QWindow::destroy which as the name says will do an xcb_destroy_window. After that the DialogProxy::onVisibleChanged will be invoked. At that point the window doesn't exist any more so the X calls are just wrong. I'd say this needs fixing in DialogProxy to not call the X specific calls for a destroyed window. Then one could think about whether invoking the methods without a valid xcb_connection is supposed to work. I'd say we should add asserts there instead of the null checks. What do you think? - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/#review46967 --- On Jan. 7, 2014, 2:57 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114895/ --- (Updated Jan. 7, 2014, 2:57 p.m.) Review request for KDE Frameworks, Martin Gräßlin and Marco Martin. Repository: kwindowsystem Description --- Guard against null QX11Info::connection() This can fail if the application is currently shutting down, this is currently causing a crash on closing plasma with dialogs open. Diffs - src/kwindoweffects_x11.cpp 72cbb71 Diff: https://git.reviewboard.kde.org/r/114895/diff/ Testing --- Opened plasmoidviewer -a org.kde.example.widgetgallery expanded the applet, then closed plasmoidviewer It used to crash, now it doesn't. Thanks, David Edmundson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
