Bug 29185 – [Qt] QWebGraphicsItem should check for null QWebPage

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

[Jakub Wieczorek]

QWebGraphicsItem segfaults when the associated page is null.

[Jakub Wieczorek]

Created attachment 39436 [details]
patch to fix the segfaults

[Antonio Gomes]

jakub, looks good overall ... just one question: in comment  https://bugs.webkit.org/show_bug.cgi?id=28862#c14 simon suggested to not use page() (maybe to avoid a unnecessary method call overhead), but d->page ... maybe we should respect that ?

ps: although i agree w/ your idea since page() willalways return a non-null pointer

[Jakub Wieczorek]

(In reply to comment #2)
> jakub, looks good overall ... just one question: in comment 
> https://bugs.webkit.org/show_bug.cgi?id=28862#c14 simon suggested to not use
> page() (maybe to avoid a unnecessary method call overhead), but d->page ...
> maybe we should respect that ?
> 
> ps: although i agree w/ your idea since page() willalways return a non-null
> pointer

I agree with the comment as far as it concerns functions that should silently return when the page is null. But most of them should just create a default page when they are called. That's also how QWebView works and I believe that QWebGraphicsItem should be symmetrical to that. Otherwise, you would never be able to load an url because the page would always be null.

[Jakub Wieczorek]

Created attachment 39446 [details]
patch to fix the segfaults

This one fixes one more crash.

[Kenneth Rohde Christiansen]

but then again I guess setGeometry etc should also create a page, so that I can do

->setGeometry(...)
->load(...)

and it will actually respect the geometry.


> I agree with the comment as far as it concerns functions that should silently
> return when the page is null. But most of them should just create a default
> page when they are called. That's also how QWebView works and I believe that
> QWebGraphicsItem should be symmetrical to that. Otherwise, you would never be
> able to load an url because the page would always be null.

[Jakub Wieczorek]

(In reply to comment #5)
> but then again I guess setGeometry etc should also create a page, so that I can
> do
> 
> ->setGeometry(...)
> ->load(...)
> 
> and it will actually respect the geometry.

It does. QWebGraphicsItem::setPage(), that would be triggered by the load() call, sets the viewport size.

[Kenneth Rohde Christiansen]

ah great! maybe add a unit test for this?

[Simon Hausmann]

Comment on attachment 39446 [details]
patch to fix the segfaults

r=me. Thanks Jakub!

[WebKit Commit Bot]

Comment on attachment 39446 [details]
patch to fix the segfaults

Rejecting patch 39446 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39446 [details]
patch to fix the segfaults

media/video-source-error.html -> timed out
You were bitten by bug 28845.

[WebKit Commit Bot]

Comment on attachment 39446 [details]
patch to fix the segfaults

Rejecting patch 39446 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39446 [details]
patch to fix the segfaults

svg/custom/group-opacity.svg -> failed
It would appear we have a flakey SVG test.

[WebKit Commit Bot]

Comment on attachment 39446 [details]
patch to fix the segfaults

Clearing flags on attachment: 39446

Committed r48357: <http://trac.webkit.org/changeset/48357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.