Bug 30549 - [Qt] Infinite loop (leading to crash) when setting cursor in QGraphicsWebView
: [Qt] Infinite loop (leading to crash) when setting cursor in QGraphicsWebView
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: PC All
: P1 Critical
Assigned To:
:
: Qt
:
: 29799 30557 30558
  Show dependency treegraph
 
Reported: 2009-10-19 19:26 PST by
Modified: 2009-11-05 07:53 PST (History)


Attachments
(commit in r49846) patch 0.1 - late emission of CursorChange event. (3.53 KB, patch)
2009-10-19 19:30 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-19 19:26:10 PST
The http://trac.webkit.org/changeset/49782 commit has reveal a bug in QGWV's setCursor method.

* Problem (manual backtrace):

_1) Widget[Qt]::setCursor
_2) QWebPageClient::setCursor
_3) QGraphicsWebViewPrivate::updateCursor
_4) QGraphicsItem::setCursor.
_5) QGraphicsWidget::itemChange method is called as the first action of qgraphicsitem.cpp::setCursor (see qt/src/gui/graphicsview/qgraphicsitem.cpp).
_6) QGraphicsWidget::itemChange fires QApplication::sendEvent(CursorChange), that is captured by QGWV's ::event.

At this point QGWV cursor has not been set yet (remember we are in the middle of _3_ yet).

_7) the quote below is executed:

void QGraphicsWebView::event() {
    (...)
    if (event->type() == QEvent::CursorChange) {
        if (cursor().shape() == Qt::ArrowCursor)
        d->resetCursor();
    }
    (...)
}

_8) d->resetCursor goes to 3) again, in an infinite loop ....... crash !

Solution:
When QGraphicsItem::setCursor calls QGraphicsWidget::itemChange as its first action, it passes 'CursorChange' as 'change'. However we can not emit 'CursoChange' event yet (see loop above).

At the end of setCursor method (when cursor had already been set), QGraphicsWidget::itemChange method is called again, but now passing the 'CursorHasChanged' as 'change'. This is the time we have to act in QGraphicsWebView.

patch coming ...
------- Comment #1 From 2009-10-19 19:30:54 PST -------
Created an attachment (id=41470) [details]
patch 0.1 - late emission of CursorChange event.
------- Comment #2 From 2009-10-19 19:32:40 PST -------
QWebView works because QWidget emits the CursorChange event just like the proposed patch does (at the end of the setCursor method):

void QWidget::setCursor(const QCursor &cursor)
{
    Q_D(QWidget);
    (...)
    setAttribute(Qt::WA_SetCursor);
    d->setCursor_sys(cursor);

    QEvent event(QEvent::CursorChange);
    QApplication::sendEvent(this, &event);
}
------- Comment #3 From 2009-10-20 01:00:00 PST -------
Seems like a workaround. Shouldn't this be fixed in Qt instead? 

Right now it seems that with QWidgets, the event is being called after the cursor has been set, and that that is not the case with QGraphicsWidgets. That is different behaviour and if that is not intentional (which I guess not), it can be considered a bug.
------- Comment #4 From 2009-10-20 02:55:45 PST -------
(From update of attachment 41470 [details])
clearing r+ flag since patch has landed.
------- Comment #5 From 2009-10-20 02:58:46 PST -------
as i talked to ariya on irc and pointed out in bug description and comment #2, i agree w/ kenneth that it has to be investigated in qt side about why such behaviour, however i do not need we have to be crashy until then. So i am landing the fix/workaround and filing to followup bugs:

1) one for auto test this
2) for investigating the real reason on why qt differ from qwidget to qgraphicsitem here.
------- Comment #6 From 2009-10-20 03:09:25 PST -------
(In reply to comment #5)
> as i talked to ariya on irc and pointed out in bug description and comment #2,
> i agree w/ kenneth that it has to be investigated in qt side about why such
> behaviour, however i do not need we have to be crashy until then. So i am
> landing the fix/workaround and filing to followup bugs:
> 
> 1) one for auto test this
> 2) for investigating the real reason on why qt differ from qwidget to
> qgraphicsitem here.

1) 30557

2) 30558