Bug 36436

Summary: QGraphicsWebView crash when calling setView on the QWebPage...
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Critical CC: commit-queue, eric, hausmann, jesus, kenneth
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
checking pageClient existance before showing popups.
none
Patch hausmann: review+

Description Alexis Menard (darktears) 2010-03-21 23:53:56 PDT
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
Comment 1 Tor Arne Vestbø 2010-03-22 06:36:27 PDT
Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs
Comment 2 Kenneth Rohde Christiansen 2010-03-22 10:49:26 PDT
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.
Comment 3 Luiz Agostini 2010-03-23 13:13:03 PDT
Created attachment 51446 [details]
checking pageClient existance before showing popups.
Comment 4 Kenneth Rohde Christiansen 2010-03-23 13:40:52 PDT
(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 5 WebKit Commit Bot 2010-03-23 13:54:29 PDT
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
Comment 6 Kenneth Rohde Christiansen 2010-03-23 14:33:35 PDT
Seems like a flaky test
Comment 7 Eric Seidel (no email) 2010-03-23 14:40:34 PDT
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.
Comment 8 Eric Seidel (no email) 2010-03-23 14:42:34 PDT
(In reply to comment #6)
> Seems like a flaky test

Filed bug 36507.
Comment 9 Kenneth Rohde Christiansen 2010-03-23 15:18:37 PDT
Created attachment 51460 [details]
Patch
Comment 10 WebKit Commit Bot 2010-03-23 15:57:26 PDT
Comment on attachment 51446 [details]
checking pageClient existance before showing popups.

Clearing flags on attachment: 51446

Committed r56424: <http://trac.webkit.org/changeset/56424>
Comment 11 Jesus Sanchez-Palencia 2010-03-23 16:17:58 PDT
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 12 Simon Hausmann 2010-03-24 08:31:32 PDT
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 13 Kenneth Rohde Christiansen 2010-03-24 12:05:44 PDT
Comment on attachment 51460 [details]
Patch

Landed (with const fixes) in 56450
Comment 14 Kenneth Rohde Christiansen 2010-03-24 12:06:19 PDT
Comment on attachment 51446 [details]
checking pageClient existance before showing popups.

Landed in 56424
Comment 15 Kenneth Rohde Christiansen 2010-03-24 12:07:52 PDT
Fixes the crash. The fixes to make the below actually work will be tracked by other bugs.
Comment 16 Simon Hausmann 2010-03-26 07:18:20 PDT
Revision r56424 cherry-picked into qtwebkit-2.0 with commit 15c6faf45d0cf9a91361b051a817859c76e8aa50
Comment 17 Simon Hausmann 2010-03-26 07:18:31 PDT
Revision r56450 cherry-picked into qtwebkit-2.0 with commit 9df72bf8cf5f8ab361bec3eae9516ab49df58f46