Bug 30459 – [Qt] "dangling" pointer to qwebpage's view object can leads QGLauncher to crash

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Antonio Gomes]

Created attachment 41328 [details]
backtrace 1

Steps to reproduce:

1) Open QGLauncher
2) Go to File->Clone View
   (a 'cloned' view shows up)
3) Put focus on the first window and load a valid URL
   (problem better reproducible w/ sites that take a while to complete the load and require theming, e.g. a search on google.com)
4) While it is loading, move mouse over the second window and do some random movements (mouse hover)
5) and yet while it is loading, close the second window.

QGLauncher crashes while accessing qwebpage::view(), which holds a "dangling" pointer.

[Antonio Gomes]

Created attachment 41329 [details]
backtrace 2

another backtrace ... page->view() again

[Antonio Gomes]

Root of the problem: in QGWV's hoverMoveEvent method, a 'view' object (QWidget)  is set to 'page' via page->setView(ev->widget())

void QGraphicsWebView::hoverMoveEvent(QGraphicsSceneHoverEvent* ev)
{
    if (d->interactive && d->page) {
        const bool accepted = ev->isAccepted();
        QMouseEvent me = QMouseEvent(QEvent::MouseMove,
                ev->pos().toPoint(), Qt::NoButton,
                Qt::NoButton, Qt::NoModifier);
        d->page->setView(ev->widget());
(...)

This 'ev->widget()' object is tied to the 'QGraphicsView' that originated the event (probably "QWidget* view->viewport()") not to the 'QGraphicsScene' object.

When we close the second window, the 'QGraphicsView' and its 'viewport' objects are deleted. At this point, qwebpage::view() refers to a "dangling" pointer.

[Antonio Gomes]

taking .. patch coming

[Kenneth Rohde Christiansen]

d->page->setView(ev->widget()); are not needed anymore after our QWebPageClient changes. Please remove them all :-)

[Antonio Gomes]

Created attachment 41331 [details]
patch 0.1 - use QPointer to make 'view' 0 at its deletion.

[Kenneth Rohde Christiansen]

Patch is not right.

The right fix is to remove those setPage calls that make no sense anymore.

[Antonio Gomes]

Comment on attachment 41331 [details]
patch 0.1 - use QPointer to make 'view' 0 at its deletion.

ok, patch does more than it should. 

i will make the deletion part into another separated patch and patch kenneth's suggestion.

[Antonio Gomes]

Created attachment 41333 [details]
patch 0.2 - remove setView calls at event handling

[Kenneth Rohde Christiansen]

LGTM! Please review someone.

[Simon Hausmann]

Comment on attachment 41333 [details]
patch 0.2 - remove setView calls at event handling

Looks good to me, too :-)

[Antonio Gomes]

thx simon and kenneth.

landed in r49729

[Antonio Gomes]

Comment on attachment 41333 [details]
patch 0.2 - remove setView calls at event handling

clearing r+ flag since patch has landed.