Attributetable fixes crash #16492

[elpaso]

Fixes https://issues.qgis.org/issues/16492

The proposed solution is to check for feature validity before editing the attributes.
The problem arises when in the attr-table selected features are deleted while one attribute editor is active.

Needs-backporting

The three other commits are just minor typos and one added comment.

[nyalldawson]

For speed you could tighten up this request a lot - fetch no geometry or attributes.

[elpaso]

I'll do, thanks.

[elpaso]

@3nids any idea about what's going on here? https://travis-ci.org/qgis/QGIS/jobs/231445942#L283 it sais that *** SIP file not up to date: core/qgsfeature.sip but that file is not part of any commits in this PR.

[nyalldawson]

What's the underlying issue here? Is it that the model doesn't get correctly updated when the feature is deleted?

[elpaso]

The problem is that removeRows is called twice: the first for deleting the features, the second because the editor was active on one of the deleted features. This condition is triggered by the function that I patched in #4535 (diff) when attributes are modified and endEditCommand is called.

The second call to removeRows crashes QGIS.

[luipir]

hi @nyalldawson I fond this bug/regression during Essen, The problem is that selected row in AttributeTable trigger the editing of the first cell of the last selected row! something that can't be avoided (I tried all combination of trigger events possibilities). Then inf the selected rows are deleted and the editing delegate of the cell is still active => the row will be triggered for removing and at the same time an update raw will be triggered but working on a misaligned model, or at least the model index is not valid.

[luipir]

@elpaso did you try if the modelIndex is still valid? or the index is valid but the feature is already removed befiore the model update?

[elpaso]

@luipir yes, that's the first thing I checked, the index is valid even if the feature has been already deleted.

[luipir]

@m-kuhn @nyalldawson any idea if we should fix the model to avoid to have valid index on disappeared data?

[m-kuhn]

The second call to removeRows crashes QGIS.

Where exactly?

Can it be avoided that it's called two times?
At the point where the crash happens exactly, can we detect that it's about to do something dangerous (and hence probably called the second time) and avoid it?

[elpaso]

@m-kuhn

At the point where the crash happens exactly, can we detect that it's about to do something dangerous (and hence probably called the second time) and avoid it?

that was my intention with the patch: without the check for a valid feature, the function will proceed with editing the attributes (on a deleted feature!) in
https://github.com/qgis/QGIS/pull/4535/files#diff-3b89a76459e8f03cbbdb69daa324356dR114

As I mentioned in #4535 (comment) , here we have two edit actions: the deletion of the selected feature and the editing of an attribute on one of those (because the editor was active), but the second action (the attribute editing) is applied after the selected features have been removed.

So, when the selected features are removed QgsAttributeTableModel::featuresDeleted is called for the first time, then QAbstracItemView::commitData calls QgsAttributeTableDelegate::setModelData and this checks for changed attributes on a deleted row and proceed editing them, the crash happens when the QgsVectorLayer::endEditCommand is called (for the second time) because it emits featuresDeleted which is connected to QgsAttributeTableModel::featuresDeleted (second call!) but at this point the rows were already removed from the model.

So, it seems to me that preventing the second (attribute) edit on the deleted feature/row is the right fix.

[m-kuhn]

What about connecting the editor to the featureDeleted signal and then silently close it and discard any edits? Would that be safe?

This would get rid of a (potentially expensive) additional request.

[elpaso]

@m-kuhn

What about connecting the editor to the featureDeleted signal and then silently close it and discard any edits? Would that be safe?

I didn't try that way.

This would get rid of a (potentially expensive) additional request.

Maybe I should just move the check inside the if, so that the editing only happens if the attribute has changed && the feature is valid. In this case the getFeatures request would only happen if an attribute was being edited.

[elpaso]

@m-kuhn done with latest commit