<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112241/">http://git.reviewboard.kde.org/r/112241/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 24th, 2013, 3:17 p.m. UTC, <b>Eike Hein</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hm, on the face of it, this patch doesn't really make sense ... launcher items don't have an associated task, so the function should already return early and the extra condition should be redundant. Unless there's a race condition in the library somewhere ... but then it still wouldn't crash on translating a QPoint.
Thank you for the patch, but I really still have to find a way to reproduce this bug before just applying this blindly - it might be treating a symptom instead of addressing the root cause.</pre>
</blockquote>
<p>On August 24th, 2013, 3:22 p.m. UTC, <b>Bhushan Shah</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Are you sure that this happens on 32 bit only?</pre>
</blockquote>
<p>On August 24th, 2013, 3:28 p.m. UTC, <b>Eike Hein</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">No, I'm not - except I can't reproduce it on any of my 64 bit machines, and I have to assume if it were a widespread bug, the number of reports we'd be getting would be much, much higher. Note that we didn't even get any reports through any of the pre-releases about this, AFAIR.
I'm typing this from a cellphone right now, but I hope when I get home tonight I'll finally get around to setting up a 32 bit VM, and I'm hoping it'll crash there so I can throw all the gdb/valgrind/asan at it we got :).</pre>
</blockquote>
<p>On August 24th, 2013, 3:30 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What bug and is "WId" involved?</pre>
</blockquote>
<p>On August 24th, 2013, 3:32 p.m. UTC, <b>Bhushan Shah</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">https://bugs.kde.org/show_bug.cgi?id=322283</pre>
</blockquote>
<p>On August 24th, 2013, 3:32 p.m. UTC, <b>Hrvoje Senjan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@Thomas
bug#322283</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks.
>From a brief review of libtaskmanager, TaskGroup::getMemberById(int id) returns AbstractGroupableItem* which could be (reimplemented)
- TaskGroup*
- LauncherItem*
- TaskItem*
but is happily static_cast'ed to TaskItem* and then accessed.
The fact(?) that itemType returns LauncherItemType indicates that there can very well be different returns and then you're accessing random memory portions - it does absolutely not matter that the function pointer for ::task() points into garbage when the item is actually a Launcher - the garbage is still rather not null, there's no guarantee that this basic deref somehow would crash or fail (virtual ::task() needed to be moved to AbstractGroupableItem then)
To know whether or why this has different implications on different architectures:
Valgrind will tell you.
For the moment
- either ::getMemberById() is supposed to return different types than TaskItem here, then static_cast needs to be qobject_cast or dynamic_cast (pending on linkage)
- or and ::getMemberById() that is *not* TaskItem must be considered a bug, then i'd start by adding an Q_ASSERT(!static_cast<TaskManager::AbstractGroupableItem*>(taskItem->itemType() == TaskManager::TaskItemType))
I don't really know, but as this thing seems to manage all kinds of items, but only updating rects for TaskItem's (and actually *every* grouped TaskItem for a TaskGroupType return ... so we don't get more bugs on "windows don't minimize to proper icon ;-P") makes sense, the correct solution is probably indeed:
AbstractGroupableItem *item = rootGroup()->getMemberById(id);
if (item->itemType() == TaskManager::LauncherItemType)
return; // launchers have no windows, we just cause X11 errors and with a little luck a stupid gobject abort
if (item->itemType() == TaskManager::TaskGroupType) {
// build a WId list of all grouped Items
...
} else //if (item->itemType() == TaskManager::TaskItemType) {
tasks << static_cast<TaskManager::TaskItem*>(taskItem)->task();
}
// search parrent and other juggling to figure the proper rectangle
...</pre>
<br />
<p>- Thomas</p>
<br />
<p>On August 24th, 2013, 2 p.m. UTC, Bhushan Shah wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kde-workspace, Plasma and Eike Hein.</div>
<div>By Bhushan Shah.</div>
<p style="color: grey;"><i>Updated Aug. 24, 2013, 2 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Fix the crash in plasma-desktop caused by newer QML taskbar widget.
Simple steps to reproduce this crash.
1) Pin any task/application to taskbar using "show launcher when not running" option.
2) Close application.
3) Desktop crashes.
Reason :
1) In Tasks::itemGeometryChanged(int,int,int,int,int) line 300 it checks for three conditions,
-> pointer to task is not null
-> taskItem itself is not null
-> scene is not null
2) This condition gets false when item is LauncherItem. In function later line 334 when calling iconRect.moveTopLeft(QPoint) function it gets crashed.
Patch :
This patch adds check in if condition to check if taskItem is TaskManager::LauncherItemType and return from function if this is launcher item.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Testing
compilation - check
installation - check
plasmoidviewer - check
in panel - check
independently - check</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plasma/desktop/applets/tasks/tasks.cpp <span style="color: grey">(c4aef4b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112241/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>