Bug 30459 - [Qt] "dangling" pointer to qwebpage's view object can leads QGLauncher to crash
Summary: [Qt] "dangling" pointer to qwebpage's view object can leads QGLauncher to crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-10-16 14:29 PDT by Antonio Gomes
Modified: 2009-10-16 20:55 PDT (History)
2 users (show)

See Also:


Attachments
backtrace 1 (4.37 KB, text/plain)
2009-10-16 14:29 PDT, Antonio Gomes
no flags Details
backtrace 2 (7.42 KB, text/plain)
2009-10-16 14:30 PDT, Antonio Gomes
no flags Details
patch 0.1 - use QPointer to make 'view' 0 at its deletion. (3.70 KB, patch)
2009-10-16 14:42 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 0.2 - remove setView calls at event handling (5.35 KB, patch)
2009-10-16 15:09 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-10-16 14:29:57 PDT
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.
Comment 1 Antonio Gomes 2009-10-16 14:30:58 PDT
Created attachment 41329 [details]
backtrace 2

another backtrace ... page->view() again
Comment 2 Antonio Gomes 2009-10-16 14:31:34 PDT
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.
Comment 3 Antonio Gomes 2009-10-16 14:32:24 PDT
taking .. patch coming
Comment 4 Kenneth Rohde Christiansen 2009-10-16 14:33:09 PDT
d->page->setView(ev->widget()); are not needed anymore after our QWebPageClient changes. Please remove them all :-)
Comment 5 Antonio Gomes 2009-10-16 14:42:45 PDT
Created attachment 41331 [details]
patch 0.1 - use QPointer to make 'view' 0 at its deletion.
Comment 6 Kenneth Rohde Christiansen 2009-10-16 14:48:12 PDT
Patch is not right.

The right fix is to remove those setPage calls that make no sense anymore.
Comment 7 Antonio Gomes 2009-10-16 14:52:14 PDT
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.
Comment 8 Antonio Gomes 2009-10-16 15:09:14 PDT
Created attachment 41333 [details]
patch 0.2 - remove setView calls at event handling
Comment 9 Kenneth Rohde Christiansen 2009-10-16 15:14:42 PDT
LGTM! Please review someone.
Comment 10 Simon Hausmann 2009-10-16 18:15:31 PDT
Comment on attachment 41333 [details]
patch 0.2 - remove setView calls at event handling

Looks good to me, too :-)
Comment 11 Antonio Gomes 2009-10-16 20:51:57 PDT
thx simon and kenneth.

landed in r49729
Comment 12 Antonio Gomes 2009-10-16 20:55:23 PDT
Comment on attachment 41333 [details]
patch 0.2 - remove setView calls at event handling

clearing r+ flag since patch has landed.