2016-09-28 Leave a comment
We have a nasty leak in
setWidget(), we create the
QWidgetItemV2 corresponding to the passed
QWidget, and pass that on to
Private::setItem, which has a bunch of error returns (guard clauses) that do not delete the item, among them negative row index and cell already occupied.
We could easily fix that missing
delete, but this function is also used by
setLayout(), say, where the item is the layout.
Conceptually deleting that item (= the nested layout) is completely OK, because the user should be able to rely on the form layout to take ownership of the nested layout, without ifs and buts.
But then we have code in
tst_qformlayout that breaks. Simplified, it reads:
QFormLayout layout; QHBoxLayout l4; layout.addLayout(-1, QFormLayout::FieldRole, &l4);
I guess you spot the problem? If
l4 had been added, everything would’ve been peachy: The
QHBoxLayout destructor unregistered itself from
layout, which does not attempt to delete
l4 when it itself is deleted.
l4 is not added for some reason, like in the test code above, the fixed code will attempt to delete
l4, which is undefined behaviour, of course (double-delete).
I think such broken code deserves to be broken, for the greater good of fixing a resource leak. Esp. since a double-delete should complain much louder than a leak, and the API user can do something about the double-delete while she can’t do anything about the leak (the pointer is not reachable from outside
I feel uneasy about adding this to 5.6 LTS, though, so I’ll make a minimal fix there, just for
What do you think about the larger fix?