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
Product: WebKit
Classification: Unclassified
Component: WebKit Qt
: 528+ (Nightly build)
: PC All
: P1 Critical
Assigned To: Antonio Gomes
: Qt
Depends on:
Blocks: Qt46 30557 30558
  Show dependency treegraph
 
Reported: 2009-10-19 19:26 PDT by Antonio Gomes
Modified: 2009-11-05 07:53 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-10-19 19:26:10 PDT
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 Antonio Gomes 2009-10-19 19:30:54 PDT
Created attachment 41470 [details]
(commit in r49846) patch 0.1 - late emission of CursorChange event.
Comment 2 Antonio Gomes 2009-10-19 19:32:40 PDT
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 Kenneth Rohde Christiansen 2009-10-20 01:00:00 PDT
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 Antonio Gomes 2009-10-20 02:55:45 PDT
Comment on attachment 41470 [details]
(commit in r49846) patch 0.1 - late emission of CursorChange event.

clearing r+ flag since patch has landed.
Comment 5 Antonio Gomes 2009-10-20 02:58:46 PDT
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 Antonio Gomes 2009-10-20 03:09:25 PDT
(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