Review Request: KTabBar: Don't crash on middle-clicking tabs with Qt 4.5.2

[Urs Wolfer]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1017/#review1615
-----------------------------------------------------------

Ship it!


Looks good to me.

- Urs


On 2009-07-13 21:50:12, Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1017/
> -----------------------------------------------------------
> 
> (Updated 2009-07-13 21:50:12)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The combination of a QTabBar change in Qt 4.5.2 and the recent restoration of middle-click tab moving in KTabBars which have the movable attribute set leads to a crash if a tab is middle-clicked before the first "tab move" operation, see
> 
> http://bugs.kde.org/show_bug.cgi?id=198075
> 
> I think the reason is as follows:
> 
> 1. If the middle mouse button is pressed on a tab, KTabBar::mousePressEvent() creates a "faked" QMouseEvent (with middle button replaced by left button) and sends that to itself to make QTabBar think that the left mouse button was pressed and a tab move operation can be started. 
> 
> 2. The faked event ends up in QTabBar::mousePressEvent(). In that method, d->pressedIndex is set from the default value -1 to the index of the pressed tab.
> 
> [QTabBar::mouseMoveEvent(), which is never called in the problematic use case, uses d->pressedIndex to determine that a tab move operation is in progress, sets d->dragInProgress to true and calls QTabBarPrivate::setupMovableTab() which initialises the QWidget* d->movingTab, which is 0 initially]
> 
> 3. If the middle mouse button is released *before* the mouse is moved, KTabBar::mouseReleaseEvent() emits a mouseMiddleClick signal and returns. No faked QMouseEvent is generated in this case because d->mMiddleMouseTabMoveInProgress is false unless the mouse is moved between pressing and releasing the button.
> 
> [If QTabBar::mouseReleaseEvent() was called now, it would set d->pressedIndex back to its default value -1, indicating that no tab move is in progress].
> 
> 4. If either QTabBar::mouseMoveEvent() or QTabBar::removeTab() (in case middle clicking is configured to close tabs) are called now, they think a tab move operation is in progress because d->pressedIndex is not -1 and try to finish that operation. We therefore end up in QTabBarPrivate::_q_moveTabFinished(int index), which relies on the QWidget* movingTab being initialised -> we get a segfault.
> 
> I would suggest to fix it by also sending a faked QMouseEvent in 3. At the moment, I can't see any other way of resetting d->pressedIndex without breaking other things, but maybe I'm overlooking something.
> 
> 
> This addresses bug 198075.
>     https://bugs.kde.org/show_bug.cgi?id=198075
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/widgets/ktabbar.cpp 996095 
> 
> Diff: http://reviewboard.kde.org/r/1017/diff
> 
> 
> Testing
> -------
> 
> Fixes the crash for me (no matter if middle-clicking is configured to close tabs or not). Everything else (including ktabwidgettest and ktabwidget_unittest) seems to work fine.
> 
> 
> Thanks,
> 
> Frank
> 
>