Bug 35051 – PopupMenu::show() assumes FrameView::hostWindow()->platformPageClient() is non-null

• Component: WebKit Qt
• Status: RESOLVED WORKSFORME

[Aaron Seigo]

If a QWebPage is created without a QWidget parent, such as when being used on a QGraphicsScene where the parent is a QGraphicsWidget, then QWebPage::d->client will be null, and this is what ChromeClientQt::platformPageClient() returns. PopupMenu::show() assumes that it is non-null:

void PopupMenu::show(const IntRect& r, FrameView* v, int index)
{
    QWebPageClient* client = v->hostWindow()->platformPageClient();
    populate(r);
    QRect rect = r;
    rect.moveTopLeft(v->contentsToWindow(r.topLeft()));
    rect.setHeight(m_popup->sizeHint().height());

    if (QGraphicsView* view = qobject_cast<QGraphicsView*>(client->ownerWidget())) {

and therefore any popup, such as triggered by clicking on a combobox in an HTML form, will trigger a crash. the backtrace ends up looking like this:

#
Thread 1 (Thread 0xb3091b30 (LWP 6207)):
#
[KCrash Handler]
#
#6  0xb48b4916 in WebCore::PopupMenu::show(WebCore::IntRect const&, WebCore::FrameView*, int) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#7  0xb4815114 in WebCore::RenderMenuList::showPopup() () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#8  0xb451be32 in WebCore::SelectElement::menuListDefaultEventHandler(WebCore::SelectElementData&, WebCore::Element*, WebCore::Event*, WebCore::HTMLFormElement*) ()
#
   from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#9  0xb451c734 in WebCore::SelectElement::defaultEventHandler(WebCore::SelectElementData&, WebCore::Element*, WebCore::Event*, WebCore::HTMLFormElement*) ()
#
   from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#10 0xb46107f3 in WebCore::HTMLSelectElement::defaultEventHandler(WebCore::Event*) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#11 0xb44fb365 in WebCore::Node::dispatchGenericEvent(WTF::PassRefPtr<WebCore::Event>) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#12 0xb44fad50 in WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#13 0xb44fc35c in WebCore::Node::dispatchMouseEvent(WebCore::AtomicString const&, int, int, int, int, int, int, bool, bool, bool, bool, bool, WebCore::Node*, WTF::PassRefPtr<WebCore::Event>) ()
#
   from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#14 0xb44fbb3f in WebCore::Node::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WebCore::AtomicString const&, int, WebCore::Node*) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#15 0xb46ec782 in WebCore::EventHandler::dispatchMouseEvent(WebCore::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) ()
#
   from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#16 0xb46ea830 in WebCore::EventHandler::handleMousePressEvent(WebCore::PlatformMouseEvent const&) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#17 0xb48da4bf in QWebPagePrivate::mousePressEvent(QMouseEvent*) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#18 0xb48e1a89 in QWebPage::event(QEvent*) () from /home/aseigo/kde4/qt-copy/lib/libQtWebKit.so.4
#
#19 0xb73017bb in Plasma::WebView::mousePressEvent (this=0x8268958, event=0xbf82d42c) at /home/aseigo/kde4/KDE/kdelibs/plasma/widgets/webview.cpp:294
#
#20 0xb66387c4 in QGraphicsItem::sceneEvent (this=0x8268960, event=0xbf82d42c) at /home/aseigo/kde4/qt/src/gui/graphicsview/qgraphicsitem.cpp:6497
#
#21 0xb669ec31 in QGraphicsWidget::sceneEvent (this=0x8268958, event=0xbf82d42c) at /home/aseigo/kde4/qt/src/gui/graphicsview/qgraphicswidget.cpp:1129
#
#22 0xb66614be in QGraphicsScenePrivate::sendEvent (this=0x8176728, item=0x8268960, event=0xbf82d42c) at /home/aseigo/kde4/qt/src/gui/graphicsview/qgraphicsscene.cpp:1181
#
#23 0xb667081b in QGraphicsScene::sendEvent (this=0xbf82e604, item=0x8268960, event=0xbf82d42c) at /home/aseigo/kde4/qt/src/gui/graphicsview/qgraphicsscene.cpp:5564
#
#24 0xb7290086 in Plasma::KineticScrolling::eventFilter (this=0x823a370, watched=0x8268958, event=0xbf82d42c) at /home/aseigo/kde4/KDE/kdelibs/plasma/private/kineticscroll.cpp:385
#
#25 0xb6c47af3 in QCoreApplicationPrivate::sendThroughObjectEventFilters (this=0x80776a0, receiver=0x8268958, event=0xbf82d42c) at /home/aseigo/kde4/qt/src/corelib/kernel/qcoreapplication.cpp:819


right now we are working around this in libplasma (part of KDE's kdelibs package) by passing a QGraphicsView into QWebPage::setView. however, this is quite sub-optimal since there is often more than one QGraphicsView on the same QGraphicsItem in our case, so the QGraphicsView passed to QWebPage::setView is fairly "random" in terms of being the current view. also, if that view were to go away, i expect it would once again be able to easily trigger crashes.

a possible solution might be to allow QWebPage to have a QGraphicsObject as parent to note that it is being used on a scene and create a QWebPageWidgetClient in that case as well. of course, i'm sure the QtWebkit devs will have even better ideas :)

[Benjamin Poulain]

Could you provide a test case?

I am not sure to understand why you don't use QGraphicsWebView in Plasma, but that is another story.

[Kent Hansen]

Created attachment 50694 [details]
Testcase

[Aaron Seigo]

(In reply to comment #1)
> I am not sure to understand why you don't use QGraphicsWebView in Plasma, but
> that is another story.

Originally we didn't use QGraphicsWebView because QGraphicsWebView didn't exist. In mainline/trunk we do use QGraphicsWebView (well, KGraphicsWebView, which is QGraphicsWebview with all the KDE integration bits registered by default). It is still wrapped in libplasma to meet the libplasma scripting API requirements.

Plasma moves very fast towards solutions, often faster than Qt can provide them. We also adapt quickly to solutions when they become available. :)

More on topic, thanks, Kent for the test case.

This looks related to bug #32670, btw. The patch attached to that report will likely fail for Plasma, however, as we often have multiple views on the canvas and the view may change (e.g. when moving an item from a desktop panel to the desktop layer itself, the item on the canvas, in this case a webview, will change where it is in the QGraphicsScene and the view will also change; ditto when adding it to a small panel and then later making the panel large enough to contain the webview in whole.)

[Kenneth Rohde Christiansen]

Seems to be the same bug as #36436, where I left a comment.

The pageClient is indeed not supposed to be null in order for WebKit to work properly.

[Jesus Sanchez-Palencia]

(In reply to comment #4)
> Seems to be the same bug as #36436, where I left a comment.
> 
> The pageClient is indeed not supposed to be null in order for WebKit to work
> properly.

Yes, looks like a duplicate. Aaron, the patches being landed on https://bugs.webkit.org/show_bug.cgi?id=36436 solve this?

I just want confirm so we can close both bugs when this is fixed.

[Aaron Seigo]

(In reply to comment #5)
> (In reply to comment #4)
> > Seems to be the same bug as #36436, where I left a comment.
> > 
> > The pageClient is indeed not supposed to be null in order for WebKit to work
> > properly.
> 
> Yes, looks like a duplicate.

they are related, yes.

> Aaron, the patches being landed on
> https://bugs.webkit.org/show_bug.cgi?id=36436 solve this?

i haven't tested them, but from reading the patches i'm note sure they do.

to get a pageClient(), it's still required to call setView with a QWidget. this will not work for apps with more than one view on the QGraphicsScene (either simultaneously or over time) for reasons already mentioned above.

and now in QtFallbackWebPopup::show() it just returns quietly if there is no view set, which looks like (again, without testing, so mea culpa if i'm reading it wrong) we will get no popups without a view being set.

imho, this really ought to be fetching the widget that the mouse event comes from; QGraphicsWidget knows this via QGraphicsSceneEvent::widget(), so the information does exist.

and hopefully there are no other places in the code that assumes pageClient() is not-null (since we'll get the same kind of crash all over again there :)

[Kenneth Rohde Christiansen]

> i haven't tested them, but from reading the patches i'm note sure they do.

They won't entirely as that would need some more work. Aaron is not using the QGraphicsWebView but using the QWebPage together with a QGraphicsItem and not a QWidget.

[Simon Hausmann]

(In reply to comment #7)
> > i haven't tested them, but from reading the patches i'm note sure they do.
> 
> They won't entirely as that would need some more work. Aaron is not using the
> QGraphicsWebView but using the QWebPage together with a QGraphicsItem and not a
> QWidget.

That is a use-case we _could_ make work, just like it works with QWidget and QWebPage.

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (In reply to comment #7)
> > > i haven't tested them, but from reading the patches i'm note sure they do.
> > 
> > They won't entirely as that would need some more work. Aaron is not using the
> > QGraphicsWebView but using the QWebPage together with a QGraphicsItem and not a
> > QWidget.
> 
> That is a use-case we _could_ make work, just like it works with QWidget and
> QWebPage.

Actually, Jesus has some patches that _makes_ this work :-) basically name this QGraphicsWebView private not inherit the page client and make it similar to that of the QWebView, by installing the page client on demand. I looked at his patches today and gave some feedback, but I'm sure he will soon have patches up for review.

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> Actually, Jesus has some patches that _makes_ this work :-) basically name this
> QGraphicsWebView private not inherit the page client and make it similar to
> that of the QWebView, by installing the page client on demand. I looked at his
> patches today and gave some feedback, but I'm sure he will soon have patches up
> for review.

And it is just waiting for review!

https://bugs.webkit.org/show_bug.cgi?id=37858 was just a refactoring for the QWidget PageClient and it is landed. Now I'm just waiting for review on https://bugs.webkit.org/show_bug.cgi?id=37866, which might solve this.

[Jesus Sanchez-Palencia]

We have a patch for this use case but we are discussing if this should go into QtWebKit 2.0 or 2.1. Removing the blockers for now and adding the PageClient patch dependency.

[Luiz Agostini]

Can we close this bug?

[Benjamin Poulain]

(In reply to comment #12)
> Can we close this bug?

Yep, looks like it.