| davidedmundson |
| Plasma |
when the window changes, the node will be deleted, and render()
will access an invalid pointer.
disconnect the render slot when the window change, as it
can't do anything useful until the next updatepaintnode.
Make the managedtexturenode actually do the management, as
textures were never deleted
BUG:388508
BUG:374280
BUG:365052
BUG:343576
The bug is easily reproducible without the patch, wasn't
able to reproduce it anymore with it
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| src/quickaddons/managedtexturenode.cpp | ||
|---|---|---|
| 42 ↗ | (On Diff #26373) | delete s_d.take(this); |
| 50 ↗ | (On Diff #26373) | auto *d = s_d.value(this);
if (!d) {
d = new ManagedTextureNodePrivate();
s_d.insert(this, d);
}
return d; |
| 68 ↗ | (On Diff #26373) | Can you perhaps prevent creation of the ManagedTextureNodePrivate for this check? d_ptr() would create it just to then see that its textureTracker is null. (Might be worth profiling, though) |
Good analysis on the plotter. Thanks for looking into it.
but in some rare cases, it can be deleted too by some external cause, usually when a widget changes its parent
If someone deletes something that's meant to be ref-counted it's being used wrong.
If there was a real bug in ManagedTextureNode we would have seen it in all the code that already used it; iconitem, framesvgitem, etc...
This is just a bug in plotter. If you do need to reverse the smart pointer logic to have a weak pointer, keep it within there.
Btw, you don't need managedtexturenode unless you're using the texturecache, since Qt 5.4 you can use QSGSimpleTextureNode::setOwnsTexture. It might help?
this is since the plotter does access that texture also somewhere else in the code, iconitem etc seems to use it only once..
what i'm suspecting, is that the texture can be deleted by someone else which is not the scoped poiter when the item changes window (by the scene itself perhaps?)
so in that case would be a problem that potentially could come up elsewhere as would make the logic of managedtexturenode not completely correct in those edge cases
Btw, you don't need managedtexturenode unless you're using the texturecache, since Qt 5.4 you can use QSGSimpleTextureNode::setOwnsTexture. It might help?
I'll try with that
so in that case would be a problem that potentially could come up elsewhere as would make the logic of managedtexturenode not completely correct in those edge cases
Textures will never be deleted by anyone else.
Docs say:
Warning: The returned texture is not memory managed by the scene graph and must be explicitly deleted by the caller on the rendering thread. This is achieved by deleting the texture from a QSGNode destructor or by using deleteLater() in the case where the texture already has affinity to the rendering thread.
However, because it's in a managed texture node, it will get deleted when the node dies.
The node definitely will be killed when you switch window.
*if* that's the case you're hitting, you're guarding the wrong thing, and will need to guard the node not the texture.
However, because it's in a managed texture node, it will get deleted when the node dies.
The node definitely will be killed when you switch window.*if* that's the case you're hitting, you're guarding the wrong thing, and will need to guard the node not the texture.
the dtor of the node is never hit, even when the window changes, regardless there is a crash or not.
it seems to be really just the texture being deleted
My valgrind, using your test instructions is showing it is.
at 0x75B5D3C: texture (qsgtexturematerial.h:58) by 0x75B5D3C: QSGSimpleTextureNode::texture() const (qsgsimpletexturenode.cpp:264) by 0x26044983: Plotter::render() (plotter.cpp:699) by 0x2604BFFF: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Plotter::*)()>::call(void (Plotter::*)(), Plotter*, void**) (qobjectdefs_impl.h:136) by 0x2604BE22: void QtPrivate::FunctionPointer<void (Plotter::*)()>::call<QtPrivate::List<>, void>(void (Plotter::*)(), Plotter*, void**) (qobjectdefs_impl.h:169) by 0x2604B08C: QtPrivate::QSlotObject<void (Plotter::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:398) by 0xB8E1E86: call (qobjectdefs_impl.h:378) by 0xB8E1E86: QMetaObject::activate(QObject*, int, int, void**) (qobject.cpp:3749) by 0xB8E236A: QMetaObject::activate(QObject*, QMetaObject const*, int, void**) (qobject.cpp:3628) by 0x764EC91: QQuickWindow::beforeRendering() (moc_qquickwindow.cpp:497) by 0x765202F: QQuickWindowPrivate::renderSceneGraph(QSize const&) (qquickwindow.cpp:452) by 0x75EE216: QSGRenderThread::syncAndRender() (qsgthreadedrenderloop.cpp:645) by 0x75F2EC0: QSGRenderThread::run() (qsgthreadedrenderloop.cpp:729) by 0xB6DF681: QThreadPrivate::start(void*) (qthread_unix.cpp:376) Address 0x25699360 is 336 bytes inside a block of size 400 free'd at 0x4C2E64B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x5099B29: ManagedTextureNode::~ManagedTextureNode() (managedtexturenode.h:45) by 0x7597A3E: QSGNode::destroy() (qsgnode.cpp:390) by 0x7597AE1: QSGNode::~QSGNode() (qsgnode.cpp:328) by 0x7597C64: QSGTransformNode::~QSGTransformNode() (qsgnode.cpp:1185) by 0x7597C72: QSGTransformNode::~QSGTransformNode() (qsgnode.cpp:1187) by 0x7651EEE: QQuickWindowPrivate::cleanupNodes() (qquickwindow.cpp:3080)
At which point after this patch we'll still be reading a QPointer in deleted memory, as that ManagedTextureNode is our m_node.