Here is the test case : #include <QtGui> #include <QtWebKit> class MyGraphicsItemWithItemChange : public QGraphicsWidget { public: MyGraphicsItemWithItemChange(QGraphicsItem *parent = 0) : QGraphicsWidget(parent) { webView = new QGraphicsWebView(this); } QVariant itemChange(GraphicsItemChange change, const QVariant &value) { if (change == QGraphicsItem::ItemSceneHasChanged) { foreach (QGraphicsView *view, scene()->views()) { //FIXME: QWebPage _requires_ a QWidget view to not crash in places such as // WebCore::PopupMenu::show() due to hostWindow()->platformPageClient() == NULL // because QWebPage::d->client is NULL webView->page()->setView(view); } } return QGraphicsWidget::itemChange(change, value); } QGraphicsWebView *webView; }; int main (int argc, char**argv) { QApplication app(argc, argv); QGraphicsScene scene; QGraphicsView view(&scene); QGraphicsWidget grandGrandParent; grandGrandParent.resize(200, 200); scene.addItem(&grandGrandParent); QGraphicsWidget grandParent; grandParent.resize(200, 200); QGraphicsWidget parent(&grandParent); parent.resize(200, 200); MyGraphicsItemWithItemChange item(&parent); grandParent.setParentItem(&grandGrandParent); view.show(); return app.exec(); } It's a bit nasty but it crashes. I'm not sure the workaround in this code is still needed but at least it should not crash. It seems to be a memory corruption . You better grab a coffee while working on that bug :D. Happy face. KDE bugs related : https://bugs.kde.org/show_bug.cgi?id=227673 and also Amarok has the same issue (after I fixed another bug because of the same nasty itemChange thing but totally unrelated). IRC : darktears
Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs
OK, this is not the right solution, but I understand the issue. QtFallbackWebPopup sets the parent of the combo to pageClient()->ownerWidget() pageClient for the QGraphicsWebView is the QGraphicsWebViewPrivate, and ownerWidget returns q->Scene()->views().value(0); Now that is fixed for the lifetime of the QGraphicsWebView, so when you change the scene that would have to be changed as well. I still do not understand how hostWindow()->platformPageClient() becomes null. Ah, that might be due to line 1785 in qwebpage.cpp: setView(qobject_cast<QWidget *>(parent)); I guess this should be QWebView* view = qobject_cast<QWebView*>(parent); if (view) { // setView installs a proper page client for the QWebView, which is needed // for the Qt parts of WebCore to function proper. setView(view); } Would be nice if you could test this.
Created attachment 51446 [details] checking pageClient existance before showing popups.
(In reply to comment #3) > Created an attachment (id=51446) [details] > checking pageClient existance before showing popups. This fixes the crash but we have a lot of issues with the current code that we need to handle in different patches. It is indeed possible to not have a page client in the case you do not have a real view. In the case below that should not be the case though.
Comment on attachment 51446 [details] checking pageClient existance before showing popups. Rejecting patch 51446 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12531 test cases. inspector/timeline-recalculate-styles.html -> failed Exiting early after 1 failures. 8985 tests run. 173.13s total testing time 8984 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 6 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1189001
Seems like a flaky test
Comment on attachment 51446 [details] checking pageClient existance before showing popups. Yes. Please feel encouraged to file a bug any time you see the commit-queue fail due to a flaky test.
(In reply to comment #6) > Seems like a flaky test Filed bug 36507.
Created attachment 51460 [details] Patch
Comment on attachment 51446 [details] checking pageClient existance before showing popups. Clearing flags on attachment: 51446 Committed r56424: <http://trac.webkit.org/changeset/56424>
Is this a duplicate from https://bugs.webkit.org/show_bug.cgi?id=35051 , reported by Aaron Seigo? I just want confirm so we can close both ones when this is fixed...
Comment on attachment 51460 [details] Patch > + virtual bool isQWidgetClient() { return false; } This should be const. > + d->unsetPageIfExists(); > d->page = page; The pattern seems to call this function and then set the page. Why not merge both into one setPage() function, that unsets the previous page and sets the new one (even if it's null)? The rest of the patch looks good to me. I suggest to fix the const before landing (all three occurences), I leave the setPage vs. unsetPageIfExists() up to you :)
Comment on attachment 51460 [details] Patch Landed (with const fixes) in 56450
Comment on attachment 51446 [details] checking pageClient existance before showing popups. Landed in 56424
Fixes the crash. The fixes to make the below actually work will be tracked by other bugs.
Revision r56424 cherry-picked into qtwebkit-2.0 with commit 15c6faf45d0cf9a91361b051a817859c76e8aa50
Revision r56450 cherry-picked into qtwebkit-2.0 with commit 9df72bf8cf5f8ab361bec3eae9516ab49df58f46