D10250: track the validity of the texture

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R296 KDeclarative

BRANCH
  phab/guardTexture2

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[Marco Martin]

mart updated this revision to Diff 26596.
mart added a comment.


  different approach

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10250?vs=26373=26596

BRANCH
  phab/guardTexture2

REVISION DETAIL
  https://phabricator.kde.org/D10250

AFFECTED FILES
  src/qmlcontrols/kquickcontrolsaddons/plotter.cpp
  src/qmlcontrols/kquickcontrolsaddons/plotter.h

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[David Edmundson]

davidedmundson added a comment.


  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::List<>, void, void (Plotter::*)()>::call(void (Plotter::*)(), 
Plotter*, void**) (qobjectdefs_impl.h:136)
  by 0x2604BE22: void QtPrivate::FunctionPointer::call, void>(void (Plotter::*)(), Plotter*, 
void**) (qobjectdefs_impl.h:169)
  by 0x2604B08C: QtPrivate::QSlotObject, 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.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[Marco Martin]

mart added a comment.


  > 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

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[David Edmundson]

davidedmundson added a comment.


  > 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.

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D10250#199624, @davidedmundson wrote:
  
  > 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.
  
  
  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

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  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?

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma, davidedmundson
Cc: davidedmundson, broulik, ngraham, plasma-devel, #frameworks, michaelh, 
ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D10250: track the validity of the texture

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> managedtexturenode.cpp:42
> +{
> +if (s_d.contains(this)) {
> +delete s_d[this];

`delete s_d.take(this);`

> managedtexturenode.cpp:50
> +{
> +if (s_d.contains(this)) {
> +return s_d[this];

auto *d = s_d.value(this);
  if (!d) {
  d = new ManagedTextureNodePrivate();
  s_d.insert(this, d);
  }
  return d;

> managedtexturenode.cpp:68
> +{
> +return d_ptr()->textureTracker;
> +}

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)

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D10250

To: mart, #plasma
Cc: broulik, ngraham, plasma-devel, #frameworks, michaelh, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10250: track the validity of the texture

[Marco Martin]

mart created this revision.
mart added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.
mart requested review of this revision.

REVISION SUMMARY
  the texture of ManagedTextureNode gets deleted by the
  refcountung of the internal QSharedPointer,
  but in some rare cases, it can be deleted too by some
  external cause, usually when a widget changes its parent
  window (either changing containment or switching between
  full/collapsed view)
  dragging a system monitor applet that uses the plotter
  component between the desktop and panel quite reliably
  crashes plasma, as it was trying to access the texture that may
  have become invalid at the moment.
  track it also with a qpointer which is used only to provide
  an hasValidTexture boolean which should be checked before
  accessing the texture of the node.
  
  BUG:388508
  BUG:374280
  BUG:365052
  BUG:343576

TEST PLAN
  The bug is easily reproducible without the patch, wasn't
  able to reproduce it anymore with it

REPOSITORY
  R296 KDeclarative

BRANCH
  phab/guardTexture

REVISION DETAIL
  https://phabricator.kde.org/D10250

AFFECTED FILES
  src/qmlcontrols/kquickcontrolsaddons/plotter.cpp
  src/qmlcontrols/kquickcontrolsaddons/plotter.h
  src/quickaddons/managedtexturenode.cpp
  src/quickaddons/managedtexturenode.h

To: mart, #plasma
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, ngraham, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart