30549 2009-10-19 19:26:10 -0700 [Qt] Infinite loop (leading to crash) when setting cursor in QGraphicsWebView 2009-11-05 07:53:59 -0800 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) PC All RESOLVED FIXED Qt P1 Critical --- 29799 30557 30558 1 tonikitoo tonikitoo ariya.hidayat hausmann kenneth vestbo oldest_to_newest 156022 0 tonikitoo 2009-10-19 19:26:10 -0700 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 ... 156026 1 41470 tonikitoo 2009-10-19 19:30:54 -0700 Created attachment 41470 (commit in r49846) patch 0.1 - late emission of CursorChange event. 156027 2 tonikitoo 2009-10-19 19:32:40 -0700 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); } 156093 3 kenneth 2009-10-20 01:00:00 -0700 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. 156112 4 41470 tonikitoo 2009-10-20 02:55:45 -0700 Comment on attachment 41470 (commit in r49846) patch 0.1 - late emission of CursorChange event. clearing r+ flag since patch has landed. 156113 5 tonikitoo 2009-10-20 02:58:46 -0700 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. 156118 6 tonikitoo 2009-10-20 03:09:25 -0700 (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 41470 2009-10-19 19:30:54 -0700 2009-10-20 02:55:45 -0700 (commit in r49846) patch 0.1 - late emission of CursorChange event. 0001--Qt-Infinite-loop-leading-to-crash-when-setting-c.patch text/plain 3616 tonikitoo RnJvbSA1NjA2ZGM0ZGQ5NzVkMmVhYTk4NzYzODZkM2RiNGRlNDFlODExYTVjIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBbnRvbmlvIEdvbWVzIDx0b25pa2l0b29Ad2Via2l0Lm9yZz4K RGF0ZTogTW9uLCAxOSBPY3QgMjAwOSAyMjoyOToyOSAtMDQwMApTdWJqZWN0OiBbUEFUQ0hdIFtR dF0gSW5maW5pdGUgbG9vcCAobGVhZGluZyB0byBjcmFzaCkgd2hlbiBzZXR0aW5nIGN1cnNvciBp biBRR3JhcGhpY3NXZWJWaWV3CiBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MzA1NDkKClBhdGNoIGJ5IEFudG9uaW8gR29tZXMgPHRvbmlraXRvb0B3ZWJraXQub3JnPiBv biAyMDA5LTEwLTE5ClJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgoKUGF0Y2ggcmVpbXBsZW1l bnRzIFFHcmFwaGljc0l0ZW0ncyBpdGVtQ2hhbmdlIG1ldGhvZCwgYW5kIG1ha2UKQ3Vyc29yQ2hh bmdlIGV2ZW50IHRvIGJlIGVtaXR0ZWQgYWZ0ZXIgY3Vyc29yIGhhcyBhbHJlYWR5IGJlZW4Kc2V0 LgoKUVdpZGdldDo6c2V0Q3Vyc29yIHNlbmQgdGhlIGV2ZW50IGp1c3QgYWZ0ZXIgaXQgc2V0cyB0 aGUgY3Vyc29yLAp0aGVuIHBhdGNoIG1ha2VzIGJvdGggYmVoYXZpb3JzIGNvbXBhdGlibGUuCgoq IEFwaS9xZ3JhcGhpY3N3ZWJ2aWV3LmNwcDoKKFFHcmFwaGljc1dlYlZpZXc6Oml0ZW1DaGFuZ2Up OgoqIEFwaS9xZ3JhcGhpY3N3ZWJ2aWV3Lmg6Ci0tLQogV2ViS2l0L3F0L0FwaS9xZ3JhcGhpY3N3 ZWJ2aWV3LmNwcCB8ICAgMjIgKysrKysrKysrKysrKysrKysrKysrKwogV2ViS2l0L3F0L0FwaS9x Z3JhcGhpY3N3ZWJ2aWV3LmggICB8ICAgIDEgKwogV2ViS2l0L3F0L0NoYW5nZUxvZyAgICAgICAg ICAgICAgICB8ICAgMTggKysrKysrKysrKysrKysrKysrCiAzIGZpbGVzIGNoYW5nZWQsIDQxIGlu c2VydGlvbnMoKyksIDAgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvV2ViS2l0L3F0L0FwaS9x Z3JhcGhpY3N3ZWJ2aWV3LmNwcCBiL1dlYktpdC9xdC9BcGkvcWdyYXBoaWNzd2Vidmlldy5jcHAK aW5kZXggN2U0ODVhMC4uOTBkYzAxZCAxMDA2NDQKLS0tIGEvV2ViS2l0L3F0L0FwaS9xZ3JhcGhp Y3N3ZWJ2aWV3LmNwcAorKysgYi9XZWJLaXQvcXQvQXBpL3FncmFwaGljc3dlYnZpZXcuY3BwCkBA IC0yNDYsNiArMjQ2LDI4IEBAIGJvb2wgUUdyYXBoaWNzV2ViVmlldzo6c2NlbmVFdmVudChRRXZl bnQqIGV2ZW50KQogCiAvKiEgXHJlaW1wCiAqLworUVZhcmlhbnQgUUdyYXBoaWNzV2ViVmlldzo6 aXRlbUNoYW5nZShHcmFwaGljc0l0ZW1DaGFuZ2UgY2hhbmdlLCBjb25zdCBRVmFyaWFudCYgdmFs dWUpCit7CisgICAgc3dpdGNoIChjaGFuZ2UpIHsKKyAgICAvLyBEaWZmZXJlbnRseSBmcm9tIFFX ZWJWaWV3LCBpdCBpcyBpbnRlcmVzdGluZyB0byBRR3JhcGhpY3NXZWJWaWV3IHRvIGhhbmRsZQor ICAgIC8vIHBvc3QgbW91c2UgY3Vyc29yIGNoYW5nZSBub3RpZmljYXRpb25zLiBSZWFzb246ICdJ dGVtQ3Vyc29yQ2hhbmdlJyBpcyBzZW50CisgICAgLy8gYXMgdGhlIGZpcnN0IGFjdGlvbiBpbiBR R3JhcGhpY3NJdGVtOjpzZXRDdXJzb3IgaW1wbGVtZW50YXRpb24sIGFuZCBhdCB0aGF0CisgICAg Ly8gaXRlbSB3aWRnZXQncyBjdXJzb3IgaGFzIG5vdCBiZWVuIGVmZmVjdGl2ZWx5IGNoYW5nZWQg eWV0LgorICAgIC8vIEFmdGVyIGN1cnNvciBpcyBwcm9wZXJseSBzZXQgKGF0ICdJdGVtQ3Vyc29y SGFzQ2hhbmdlZCcgZW1pc3Npb24gdGltZSksIHdlCisgICAgLy8gZmlyZSAnQ3Vyc29yQ2hhbmdl Jy4KKyAgICBjYXNlIEl0ZW1DdXJzb3JDaGFuZ2U6CisgICAgICAgIHJldHVybiB2YWx1ZTsKKyAg ICBjYXNlIEl0ZW1DdXJzb3JIYXNDaGFuZ2VkOgorICAgICAgICBRRXZlbnQgZXZlbnQoUUV2ZW50 OjpDdXJzb3JDaGFuZ2UpOworICAgICAgICBRQXBwbGljYXRpb246OnNlbmRFdmVudCh0aGlzLCAm ZXZlbnQpOworICAgICAgICByZXR1cm4gdmFsdWU7CisgICAgfQorCisgICAgcmV0dXJuIFFHcmFw aGljc1dpZGdldDo6aXRlbUNoYW5nZShjaGFuZ2UsIHZhbHVlKTsKK30KKworLyohIFxyZWltcAor Ki8KIGJvb2wgUUdyYXBoaWNzV2ViVmlldzo6ZXZlbnQoUUV2ZW50KiBldmVudCkKIHsKICAgICAv LyBSZS1pbXBsZW1lbnRlZCBpbiBvcmRlciB0byBhbGxvd3MgZml4aW5nIGV2ZW50LXJlbGF0ZWQg YnVncyBpbiBwYXRjaCByZWxlYXNlcy4KZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcWdyYXBo aWNzd2Vidmlldy5oIGIvV2ViS2l0L3F0L0FwaS9xZ3JhcGhpY3N3ZWJ2aWV3LmgKaW5kZXggMjZm NzM3NC4uNDNjZjU5YSAxMDA2NDQKLS0tIGEvV2ViS2l0L3F0L0FwaS9xZ3JhcGhpY3N3ZWJ2aWV3 LmgKKysrIGIvV2ViS2l0L3F0L0FwaS9xZ3JhcGhpY3N3ZWJ2aWV3LmgKQEAgLTg1LDYgKzg1LDcg QEAgcHVibGljOgogICAgIHZpcnR1YWwgdm9pZCBzZXRHZW9tZXRyeShjb25zdCBRUmVjdEYmIHJl Y3QpOwogICAgIHZpcnR1YWwgdm9pZCB1cGRhdGVHZW9tZXRyeSgpOwogICAgIHZpcnR1YWwgdm9p ZCBwYWludChRUGFpbnRlciosIGNvbnN0IFFTdHlsZU9wdGlvbkdyYXBoaWNzSXRlbSogb3B0aW9u cywgUVdpZGdldCogd2lkZ2V0ID0gMCk7CisgICAgdmlydHVhbCBRVmFyaWFudCBpdGVtQ2hhbmdl KEdyYXBoaWNzSXRlbUNoYW5nZSBjaGFuZ2UsIGNvbnN0IFFWYXJpYW50JiB2YWx1ZSk7CiAgICAg dmlydHVhbCBib29sIGV2ZW50KFFFdmVudCopOwogCiBwdWJsaWMgUV9TTE9UUzoKZGlmZiAtLWdp dCBhL1dlYktpdC9xdC9DaGFuZ2VMb2cgYi9XZWJLaXQvcXQvQ2hhbmdlTG9nCmluZGV4IDc5ODc2 MTMuLjhjNWFkZDUgMTAwNjQ0Ci0tLSBhL1dlYktpdC9xdC9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0 L3F0L0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDA5LTEwLTE5ICBBbnRvbmlvIEdvbWVz ICA8dG9uaWtpdG9vQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgW1F0XSBJbmZpbml0ZSBsb29wIChsZWFkaW5nIHRvIGNyYXNoKSB3 aGVuIHNldHRpbmcgY3Vyc29yIGluIFFHcmFwaGljc1dlYlZpZXcKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMwNTQ5CisKKyAgICAgICAgUGF0Y2ggcmVp bXBsZW1lbnRzIFFHcmFwaGljc0l0ZW0ncyBpdGVtQ2hhbmdlIG1ldGhvZCwgYW5kIG1ha2UKKyAg ICAgICAgQ3Vyc29yQ2hhbmdlIGV2ZW50IHRvIGJlIGVtaXR0ZWQgYWZ0ZXIgY3Vyc29yIGhhcyBh bHJlYWR5IGJlZW4KKyAgICAgICAgc2V0LgorCisgICAgICAgIFFXaWRnZXQ6OnNldEN1cnNvciBz ZW5kIHRoZSBldmVudCBqdXN0IGFmdGVyIGl0IHNldHMgdGhlIGN1cnNvciwKKyAgICAgICAgdGhl biBwYXRjaCBtYWtlcyBib3RoIGJlaGF2aW9ycyBjb21wYXRpYmxlLgorCisgICAgICAgICogQXBp L3FncmFwaGljc3dlYnZpZXcuY3BwOgorICAgICAgICAoUUdyYXBoaWNzV2ViVmlldzo6aXRlbUNo YW5nZSk6CisgICAgICAgICogQXBpL3FncmFwaGljc3dlYnZpZXcuaDoKKwogMjAwOS0xMC0xOSAg TmF0ZSBDaGFwaW4gIDxqYXBoZXRAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFVucmV2aWV3ZWQs IGJ1aWxkIGZpeC4KLS0gCjEuNi4wLjQKCg==