Bug 36436 – QGraphicsWebView crash when calling setView on the QWebPage...

• Component: WebKit Qt
• Status: CLOSED FIXED

[Alexis Menard (darktears)]

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

[Tor Arne Vestbø]

Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs

[Kenneth Rohde Christiansen]

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.

[Luiz Agostini]

Created attachment 51446 [details]
checking pageClient existance before showing popups.

[Kenneth Rohde Christiansen]

(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.

[WebKit Commit Bot]

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

[Kenneth Rohde Christiansen]

Seems like a flaky test

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

(In reply to comment #6)
> Seems like a flaky test

Filed bug 36507.

[Kenneth Rohde Christiansen]

Created attachment 51460 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 51446 [details]
checking pageClient existance before showing popups.

Clearing flags on attachment: 51446

Committed r56424: <http://trac.webkit.org/changeset/56424>

[Jesus Sanchez-Palencia]

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...

[Simon Hausmann]

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 :)

[Kenneth Rohde Christiansen]

Comment on attachment 51460 [details]
Patch

Landed (with const fixes) in 56450

[Kenneth Rohde Christiansen]

Comment on attachment 51446 [details]
checking pageClient existance before showing popups.

Landed in 56424

[Kenneth Rohde Christiansen]

Fixes the crash. The fixes to make the below actually work will be tracked by other bugs.

[Simon Hausmann]

Revision r56424 cherry-picked into qtwebkit-2.0 with commit 15c6faf45d0cf9a91361b051a817859c76e8aa50

[Simon Hausmann]

Revision r56450 cherry-picked into qtwebkit-2.0 with commit 9df72bf8cf5f8ab361bec3eae9516ab49df58f46