Review Request 120057: Fix scrollbar appearing on FolderView

[Mark Harmstone]

> On Sept. 5, 2014, 11:55 a.m., David Edmundson wrote:
> > I'm not very happy with this. I'm sure it works, but it's a workaround without knowing the root problem (or at least I'm not convinced we do). It almost certainly is not memory corruption, and blind patching isn't a good longterm strategy. 
> > 
> > I imagine the case will be:
> >  - widget is made (with QGraphicsScene that corana inherits from having size 0,0)
> >  - we run this FolderView::constraintsEvent
> >  - corona resizes
> >  - folderview doesn't see it has resized
> > or something along these lines
> >   
> > Can you test what path we take in Corona::screenGeometry? and what the sceneRect is at the point this runs?
> 
> David Edmundson wrote:
>     forgot to say: Thanks for looking into this, it's really appreciated.
> 
> Mark Harmstone wrote:
>     You're welcome, no problem.
>     
>     Blind patching might not be a good idea in general, but I think it's worth bearing in mind that this fixes a major usability bug, and one that for two years has proven nearly impossible to debug, let alone fix. Plus it obviously doesn't break anything - and, by way of comparison, the comment a few lines above it has read "FIXME: a pretty horrible hack" for a couple of years now...
>     
>     I'm almost certain this is a memory corruption issue, though. If you insert a line "volatile QRect r3(r);" just before DesktopCorona::availableScreenRect returns, and put a breakpoint on the return, gdb says that when this bug is triggered, r3 and r differ. (Specifically, r = {x1 = 0, y1 = 0, x2 = 1919, y2 = 1079}, as it should be, but r3 = {x1 = 0, y1 = 0, x2 = 1919, y2 = 0}, which is what is actually returned.)
>     I've uploaded a disassembly of DesktopCorona::availableScreenRect, as compiled by gcc -O2, to http://burntcomma.com/dc.txt. There's a couple of "volatile" debugging lines there, which is why the line numbering doesn't match up exactly. I don't understand assembly all that well - certainly not well enough to diagnose and fix the problem in gcc, if there is one - but it looks like r.bottom() is being stored in DWORD PTR [rsp+0x3c]. Even though there's nothing wrong with the C++ code (as far as I can see), this is then being clobbered within the foreach loop, for some reason.
> 
> David Edmundson wrote:
>     well that's me close to happy. Good investigation :)
>     
>     Any chance you can put a breakdown just after
>     QRect r3(r)
>     
>     and type 
>     "watch r3.y2"
>     
>     we can see who does the clobbering.
> 
> Mark Harmstone wrote:
>     If I set a watchpoint for r3.y2, it only triggers on the definition line itself:
>     
>     Old value = 1079
>     New value = 0
>     0x00007ffff7b915fc in DesktopCorona::availableScreenRect (this=<optimized out>, id=0)
>         at /var/tmp/portage/kde-base/plasma-workspace-4.11.11/work/plasma-workspace-4.11.11/plasma/desktop/shell/desktopcorona.cpp:358
>     358         volatile QRect r3(r);
>     
>     
>     The assembly line there is mov rax,QWORD PTR [rsp+0x80].
>     
>     If I comment out the volatiles and set a watch for r.y2, it doesn't trigger at all - gdb thinks r is unaltered by the loop, as it should be.
>     
>     It looks like the bug is actually that the y2 of QRects can get clobbered on assignment - as far as I can see, the availRect that FolderView::updateScreenRegion gets isn't actually r, but a copy made in DesktopCorona::qt_static_metacall.
> 
> David Edmundson wrote:
>     Interestingly old value is the correct value, so we're saying in the one call
>     QRect r3(r) it sets the correct value, and then unsets it?
>     
>     It should be a copy, as we exit availableScreenRegion It does make sense that that part of the memory is getting messed up _after_ we exit availableScreenRect, as it won't be on the stack anymore.
>     
>     I was under the impression you were saying things break before we hit "return r" or are you saying all QRect(const QRect &other) calls are broken?

QRect r3(r) is setting the value of r.y2 to 0, in addition to doing the r3 assignments... I don't understand it at all. What gdb is saying is that if you comment the debugging line out the function would return the correct result, though what the caller actually receives is the same as r3 would have been. I don't think all of the QRect constructors are broken, as it works fine further up in the function.

Regardless, this is obviously some sort of arcane gcc bug. I'll try to put together some sort of minimal test case and send it over there for them to have a look at. In the meantime, could we include this patch? If you look at that bug report, people have been getting very annoyed and frustrated with it for quite some time. The patch does no harm and would make a lot of people happy...


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120057/#review65827
-----------------------------------------------------------


On Sept. 5, 2014, 11:25 a.m., Mark Harmstone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120057/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2014, 11:25 a.m.)
> 
> 
> Review request for Plasma and Ignat Semenov.
> 
> 
> Bugs: 294795
>     http://bugs.kde.org/show_bug.cgi?id=294795
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Fixes bug 294795. See there for further details.
> 
> 
> Diffs
> -----
> 
>   plasma/applets/folderview/folderview.cpp f56cab1fee8848c551d3687dbd480ed1ed5ac591 
> 
> Diff: https://git.reviewboard.kde.org/r/120057/diff/
> 
> 
> Testing
> -------
> 
> The patch fixes a race condition, meaning it is difficult to reproduce. However, with the patch applied I have not been able to reproduce it after dozens of reboots on a handful of different machines. Plus the scrollbar issue was always accompanied by the toolbox changing position, which still happens sporadically - strongly implying that the scrollbar is fixed.
> 
> 
> Thanks,
> 
> Mark Harmstone
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140907/acdab926/attachment-0001.html>