Bug 32670 - QGraphicsWebView crash
Summary: QGraphicsWebView crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Critical
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-12-17 10:08 PST by Anders Bakken
Modified: 2010-03-23 15:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch to fix bug (1.21 KB, patch)
2009-12-17 10:09 PST, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2010-03-23 14:48 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Bakken 2009-12-17 10:08:32 PST
QGraphicsWebViewPrivate assumes that it has been added to a scene and can crash when webkit calls QWebPageClient functions on it.

This attached example crashes on Qt/X11 (but likely on all platforms).
The attached patch takes care of the problem.
Comment 1 Anders Bakken 2009-12-17 10:09:07 PST
Created attachment 45082 [details]
Patch to fix bug
Comment 2 Laszlo Gombos 2009-12-24 03:50:39 PST
Anders, thanks for the patch. 

Every patch requires a ChangeLog.  See http://webkit.org/coding/contributing.html for how to create one. Next time you should also mark the patch for review to get some attentions from reviewers. 

The code changes looks good to me.
Comment 3 Tor Arne Vestbø 2010-03-10 06:27:36 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords
Comment 4 Simon Hausmann 2010-03-18 15:47:54 PDT
Anders, any update on this? It seems only the changelog is missing :)
Comment 5 Simon Hausmann 2010-03-18 15:49:23 PDT
Changing severity to critical, it's a crasher.
Comment 6 Jesus Sanchez-Palencia 2010-03-23 14:48:06 PDT
Created attachment 51455 [details]
Patch
Comment 7 Jesus Sanchez-Palencia 2010-03-23 14:52:38 PDT
Since this is critical, I have applied the patch to trunk and added a Changelog to it. I kept Anders Bakken as the primary author of the patch.
Comment 8 WebKit Commit Bot 2010-03-23 15:40:23 PDT
Comment on attachment 51455 [details]
Patch

Clearing flags on attachment: 51455

Committed r56423: <http://trac.webkit.org/changeset/56423>
Comment 9 WebKit Commit Bot 2010-03-23 15:40:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Kenneth Rohde Christiansen 2010-03-23 15:43:41 PDT
Not solved properly yet, so reopening.
Comment 11 Kenneth Rohde Christiansen 2010-03-23 15:44:32 PDT
Wrong bug :-(
Comment 12 Jesus Sanchez-Palencia 2010-03-23 15:53:21 PDT
(In reply to comment #7)
> Since this is critical, I have applied the patch to trunk and added a Changelog
> to it. I kept Anders Bakken as the primary author of the patch.

What I mean is that I have updated the patch and not really landed it into trunk. It is now landed (thanks, Kenneth!).


I was taking another look at qgraphicswebview.cpp and noticed three other places that assume that q->scene() isn't null without checking:

- QGraphicsWebViewPrivate::~QGraphicsWebViewPrivate(), line 169.
- QGraphicsWebViewPrivate::setRootGraphicsLayer(QGraphicsItem* layer), line 199.
- QGraphicsWebViewPrivate::_q_updateMicroFocus(), line 254.

The first two we just have when ACCELERATED_COMPOSITING is enable. So I _guess_ that we could assume that we always have a QGScene on this situation. Maybe only a ASSERT is necessary?

The last one, at _q_updateMicroFocus(), would need a check as the ones from the previous patch, imho.

What do you think?


Also,
(In reply to comment #0)
> This attached example crashes on Qt/X11 (but likely on all platforms).

I think that the example is missing, Anders. It would be nice to have it as a test on QtWebKit, maybe.