I use some fork of QMapControl and found a bug. GeometryPointImage has getter for image const QPixmap& image() const
:
const QPixmap& GeometryPointImage::image() const
{
// Is the image pixmap currently null?
if (m_image == nullptr) { //std::shared_ptr<QPixmap> m_image;
// Have we already constructed the null image pixmap?
if (m_image_null == nullptr) { //std::unique_ptr<QPixmap> m_image_null;
// Construct the null image pixmap.
m_image_null.reset(new QPixmap);
}
// Return the null image pixmap.
return *(m_image_null.get());
} else {
// Return the image pixmap.
return *(m_image.get());
}
}
There are various setters for m_image
, and image()
getter is used in draw()
function:
painter.drawPixmap(-pixmap_rect_px.rawRect().width() / 2.0, -pixmap_rect_px.rawRect().height() / 2.0, image()); // void drawPixmap(int x, int y, const QPixmap &pm)
I can catch behaviour like this: draw()
function calls image()
that dereferences shared pointer, it goes into drawPixmap
and some other event calls setImage()
where m_image
assigns to new value, and destructor of shared pointer destructs QPixmap
object referenced by drawPixmap()
and then app goes SIGSEGV.
I think getter that returns reference to something owned by shared pointer is no such good practice, but what is most suitable solution? I don't want to copy QPixMap
object or add a mutex into getters, setters and draw()
. Is there a way to prolong life of referenced object (with something similar to qAsConst
maybe)? Should getter return std::shared_ptr<QPixmap>
?
UPD:
In details: setImage()
is called from main thread and this setter is expected to emit signal to redraw object. But QMapControl main class also uses QtConcurrent::run()
to redraw whole scene, and it touches pixmap from some other thread. And thread #1 deleting object when thread e.g. #6 (or #7) does drawPixmap()
.
Well, I don't expect someone to give another answer to this question by now, so let there be my solution, it may help someone who will face such a problem later:
std::shared_ptr<QPixmap> GeometryPointImage::image() const
{
// Is the image pixmap currently null?
if (m_image == nullptr)
{
// Have we already constructed the null image pixmap?
if (m_image_null == nullptr)
{
// Construct the null image pixmap.
m_image_null.reset(new QPixmap);
}
// Return the null image pixmap.
return std::make_shared<QPixmap>(*m_image_null.get());
}
else
{
// Return the image pixmap.
return m_image;
}
}
....
painter.drawPixmap(-pixmap_rect_px.rawRect().width() / 2.0, -pixmap_rect_px.rawRect().height() / 2.0, *image());
Now, returning a shared_ptr
prolongates QPixMap's lifetime even if it has been reset by something else. Moreover, image()
method was only used in class and nowhere from outside, so it was easy to fix this issue.