> On Aug. 24, 2013, 3:17 p.m., Eike Hein wrote: > > 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. > > Bhushan Shah wrote: > Are you sure that this happens on 32 bit only? > > Eike Hein wrote: > 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 :). > > Thomas Lübking wrote: > What bug and is "WId" involved? > > Bhushan Shah wrote: > https://bugs.kde.org/show_bug.cgi?id=322283 > > Hrvoje Senjan wrote: > @Thomas > bug#322283 > > Thomas Lübking wrote: > 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 > ...
Thomas: Building the list of group child windows is already done (the traversal happens on the QML side). - Eike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112241/#review38481 ----------------------------------------------------------- On Aug. 24, 2013, 2 p.m., Bhushan Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112241/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2013, 2 p.m.) > > > Review request for kde-workspace, Plasma and Eike Hein. > > > Description > ------- > > 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. > > > Diffs > ----- > > plasma/desktop/applets/tasks/tasks.cpp c4aef4b > > Diff: http://git.reviewboard.kde.org/r/112241/diff/ > > > Testing > ------- > > Testing > > compilation - check > installation - check > plasmoidviewer - check > in panel - check > independently - check > > > Thanks, > > Bhushan Shah > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
