D6624: do not crash qaccessible by causing a resize in a resize event
This revision was automatically updated to reflect the committed changes. Closed by commit R236:ca40063c4e49: do not crash qaccessible by causing a resize in a resize event (authored by sitter). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D6624?vs=16502=16860#toc REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6624?vs=16502=16860 REVISION DETAIL https://phabricator.kde.org/D6624 AFFECTED FILES src/kcharselect.cpp To: sitter, gladhorn, cfeck Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
cfeck accepted this revision. cfeck added a comment. This revision is now accepted and ready to land. Would be nice if you also commit the conditional in the KCharSelectItemModel ::setColumnCount. REPOSITORY R236 KWidgetsAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn, cfeck Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter added a comment. In https://phabricator.kde.org/D6624#126506, @cfeck wrote: > Let's say someone fixes the referenced Qt bug, and we are at a point requiring that Qt version anyway. Are you going to remove the workaround? Anyone who happens to stumble upon it can. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
cfeck added a comment. Let's say someone fixes the referenced Qt bug, and we are at a point requiring that Qt version anyway. Are you going to remove the workaround? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter added a comment. In https://phabricator.kde.org/D6624#126503, @cfeck wrote: > If you are sure that your fix is "correct", then please remove the comment. From reading it, it looks like a workaround for a Qt bug. It is a workaround. It crashes because the life time of the qaccessible objects is managed rather badly and fixing that is less trivial than this workaround. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
cfeck added a comment. Reading your comment it is also wrong. Resizing the model does not cause a resize of the widget. The widget size is controlled by the layout manager. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
cfeck added a comment. If you are sure that your fix is "correct", then please remove the comment. From readiong it, it looks like a workaround for a Qt bug. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter added a comment. In https://phabricator.kde.org/D6624#126465, @cfeck wrote: > > It probably does. > > Were you able to test? I would prefer the simpler patch. I cannot test it, because my system does not have accessibility enabled. Yes, I did not manage to crash it that way. As I said though, I do not think making the smallest possible fix is prudent here. While that change would be simpler it would also be more fragile. In resizeEvent() we'd still call a function of which the intention is to change the size of cells, which is meant to result in a layout change and thus potentially causes a resize, bringing us back to the crashing call chain. Making the column update conditional does fix the immediate cause of this **right now**. Conceptually the issue would still be there: we call a method of which the intention is to change the layout from within a resizeEvent handler. Something that is not safe to do with the current qaccessible lifetime management. If someone goes ahead and changes the way the models are being used and/or //when// size calculation happens in the future we'll be crashing again. TLDR I don't find introducing the `if` a future-proof solution. It's only treating the symptom. We'd reinforce a bone with a metal plate whilst ignoring the fact that the patient has a disorder that makes her run in front of cars. (I do think that introducing that if would be handy eitherway, not as a measure of dealing with the crash though) REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
cfeck added a comment. > It probably does. Were you able to test? I would prefer the simpler patch. I cannot test it, because my system does not have accessibility enabled. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
gladhorn added a comment. Considering that the Qt bug will not be fixed in the next few days (I hope to get around to it, but it's involved), this makes sense. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter added a comment. In https://phabricator.kde.org/D6624#124077, @cfeck wrote: > Reading your description on the referenced QTBUG, does it help to use a compare with previous m_columns in KCharSelectItemModel ::setColumnCount() before doing the emit dance? It probably does. To me, that would seem a bit too close to the cause though. If tomorrow another bit of the resizing calculation starts causing a re-layout it would crash again. That said, I think adding an `if` would be handy either way, what with saving a bunch of cpu cycles. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
cfeck added a comment. Reading your description on the referenced QTBUG, does it help to use a compare with previous m_columns in KCharSelectItemModel ::setColumnCount() before doing the emit dance? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: cfeck, anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kcharselect.cpp:251 > QSignalBlocker blockResize(this) ? Block inhibits signal emission/slot calling. That is not what we want. We want the signals to run, just not on the same call chain as the resize event. REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
anthonyfieroni added inline comments.
INLINE COMMENTS
> kcharselect.cpp:251
> +connect(timer, ::timeout, [&,timer]() {
> +d->_k_resizeCells();
> +timer->deleteLater();
QSignalBlocker blockResize(this) ?
REPOSITORY
R236 KWidgetsAddons
REVISION DETAIL
https://phabricator.kde.org/D6624
To: sitter, gladhorn
Cc: anthonyfieroni, #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter added a comment. FWIW, this is a bit of workaround. Not having kcharselect crash for just about every search is well worth it though. Also, I am not sure I particularly like the qtimer code. It does beat having to pass `const char *` method names to `invokeMethod` and turning the private into a qobject. With the lambda the resize call at least gets checked by the compiler ¯\_(ツ)_/¯ REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D6624 To: sitter, gladhorn Cc: #frameworks
D6624: do not crash qaccessible by causing a resize in a resize event
sitter created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY When enabling accessibility qaccessible will automatically add a11y support constructs to core qt types such as qtableview. Unfortunately for qtableview specifically a change of the layout/size will discard the accessible objects modeling the individual cells in our table. Combined with the way we control layout through the model [I am not sure why we do this to begin with] this can result in call chains where qaccessible triggers a resizeEvent which we'll handle by rejiggering our model to get a new layout, resulting in qaccessible deleting the object which originally caused the event and ending in a segfault. To prevent this problem we delay the rejiggering of our model by running the call through the eventloop (i.e. the resize is executed once the stack unwinds again to the event loop). CHANGELOG: Fixed a crash when searching with accessibility support enabled BUG: 374933 Also see https://bugreports.qt.io/browse/QTBUG-58153 TEST PLAN spent a good while searching and copy pasting and all that fun stuff. no more crashing. REPOSITORY R236 KWidgetsAddons BRANCH master REVISION DETAIL https://phabricator.kde.org/D6624 AFFECTED FILES src/kcharselect.cpp To: sitter, gladhorn Cc: #frameworks
