Bug 54138

Summary: [Qt] QWebPage with WebGL content crashes when rendering if no QWebView parent
Product: WebKit Reporter: Andrew Wason <rectalogic>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, commit-queue, eric, jarkko.j.sakkinen, kling, webkit.review.bot
Priority: P5 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 53431    
Bug Blocks:    
Attachments:
Description Flags
Qt app that demonstrates the crash
none
gdb backtrace when running client_crash test app
none
Check webPageClient before referencing it
benjamin: review-
Check webPageClient before referencing it
none
Check webPageClient before referencing it
benjamin: review-
Check webPageClient before referencing it none

Description Andrew Wason 2011-02-09 13:24:08 PST
Created attachment 81860 [details]
Qt app that demonstrates the crash

This bug depends on a working WebGL build from bug 53431

If a QWebPage content contains WebGL, when the page is rendered it crashes because WebCore::GraphicsContext3DInternal::getOwnerGLWidget assumes we have a QWebPageClient. Need to check and return 0 if no QWebPageClient was set.

Simple demo app attached, and backtrace from running it.
Comment 1 Andrew Wason 2011-02-09 13:25:02 PST
Created attachment 81861 [details]
gdb backtrace when running client_crash test app
Comment 2 Andrew Wason 2011-02-09 13:28:28 PST
Created attachment 81863 [details]
Check webPageClient before referencing it
Comment 3 Benjamin Poulain 2011-02-09 13:49:14 PST
Comment on attachment 81863 [details]
Check webPageClient before referencing it

This should really have an autotest.
Comment 4 Andrew Wason 2011-02-09 18:43:54 PST
Created attachment 81911 [details]
Check webPageClient before referencing it

Add unit test
Comment 5 Antonio Gomes 2011-02-09 21:23:43 PST
Comment on attachment 81911 [details]
Check webPageClient before referencing it

View in context: https://bugs.webkit.org/attachment.cgi?id=81911&action=review

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2482
> +#if defined(ENABLE_WEBGL) && ENABLE_WEBGL

this looks odd...

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2483
> +// https://bugs.webkit.org/show_bug.cgi?id=54138

usually git blame and the changelog are enough for reference bugs like this.
Comment 6 Andrew Wason 2011-02-10 08:07:39 PST
(In reply to comment #5)
> (From update of attachment 81911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81911&action=review
> 
> > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2482
> > +#if defined(ENABLE_WEBGL) && ENABLE_WEBGL
> 
> this looks odd...

I know the conventional way is:
#if ENABLE(WEBGL)
but the ENABLE macro isn't available in tst_qwebpage.cpp - not sure what header I need to get it.

Should I just remove the ifdefs altogether?
Comment 7 Benjamin Poulain 2011-02-10 08:10:35 PST
(In reply to comment #6)
> I know the conventional way is:
> #if ENABLE(WEBGL)
> but the ENABLE macro isn't available in tst_qwebpage.cpp - not sure what header I need to get it.

I doubt ENABLE_WEBGL is available for the tests, this is WebKit internal.

Maybe you should check if WebGL is available at runtime, like I did for geolocation?
Comment 8 Andreas Kling 2011-02-10 08:12:43 PST
Why is there no QWebPageClient set?
Comment 9 Andrew Wason 2011-02-10 08:14:29 PST
(In reply to comment #7)
> I doubt ENABLE_WEBGL is available for the tests, this is WebKit internal.

Well, the test builds and runs (when passing --3d-canvas to build-webkit) so it must be available.

> Maybe you should check if WebGL is available at runtime, like I did for geolocation?

OK, I'll do that.
Comment 10 Benjamin Poulain 2011-02-10 08:18:36 PST
(In reply to comment #9)
> (In reply to comment #7)
> > I doubt ENABLE_WEBGL is available for the tests, this is WebKit internal.
> 
> Well, the test builds and runs (when passing --3d-canvas to build-webkit) so it must be available.

That particular function runs? Ok.
Comment 11 Andrew Wason 2011-02-10 08:30:38 PST
(In reply to comment #8)
> Why is there no QWebPageClient set?

It looks like the client is set when you set the view (in QWebPage::setView in Source/WebKit/qt/Api/qwebpage.cpp) and we don't have a view in this case.
Comment 12 Andrew Wason 2011-02-10 08:36:38 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Well, the test builds and runs (when passing --3d-canvas to build-webkit) so it must be available.
> 
> That particular function runs? Ok.

Yes:

./tst_qwebpage webGLScreenshotWithoutView
********* Start testing of tst_QWebPage *********
Config: Using QTest library 4.7.1, Qt 4.7.1
PASS   : tst_QWebPage::initTestCase()
PASS   : tst_QWebPage::webGLScreenshotWithoutView()
PASS   : tst_QWebPage::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped
********* Finished testing of tst_QWebPage *********
LEAK: 7 WebCoreNode
LEAK: 291 Structure
Comment 13 Jarkko Sakkinen 2011-03-20 13:03:58 PDT
Hi, what's the status of this? I have implemented couple of fixes to upstream that you have to take into account:
- https://bugs.webkit.org/show_bug.cgi?id=56555
- https://bugs.webkit.org/show_bug.cgi?id=56339

I looked at your fix and these changes must be done:
- In the test case enable QWebSettings::AcceleratedCompositing. We support (at least for the first releasable WebGL support) creation only when accelerated compositing is enabled.
- Put the fix into getViewportGLWidget().

Either submit a revised fix *or* inform me if you don't want to do it. Thank you.
Comment 14 Jarkko Sakkinen 2011-03-20 13:05:18 PDT
This is also now listed in https://trac.webkit.org/wiki/QtWebKitWebGL
Comment 15 Andrew Wason 2011-03-20 18:40:38 PDT
Created attachment 86288 [details]
Check webPageClient before referencing it

Fix patch to take into account upstream changes.
Enable QWebSettings::AcceleratedCompositingEnabled in testcase.
Comment 16 Jarkko Sakkinen 2011-03-20 23:14:33 PDT
Looks good to me. Thanks.
Comment 17 Benjamin Poulain 2011-03-22 06:27:48 PDT
Comment on attachment 86288 [details]
Check webPageClient before referencing it

View in context: https://bugs.webkit.org/attachment.cgi?id=86288&action=review

The change make sense to me, I would just like better testing.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2482
> +void tst_QWebPage::webGLScreenshotWithoutView()

I would like this test, with and without AcceleratedCompositingEnabled. Just to have good coverage.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2484
> +    QWebPage* page = new QWebPage;

You should allocate the QWebPage on the stack to avoid dealing with the memory manually.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2492
> +    ::waitForSignal(mainFrame, SIGNAL(loadFinished(bool)), 2000);

Is loadFinished() called for setHtml? Otherwise the test would always spend 2 seconds waiting.
Comment 18 Andrew Wason 2011-03-22 08:20:14 PDT
Created attachment 86462 [details]
Check webPageClient before referencing it

Changed testcase to:
* Test with and without AcceleratedCompositing enabled.
* Allocate QWebPage on stack.
* Do not wait for loadFinished signal, it is not called for setHtml().
Comment 19 WebKit Commit Bot 2011-03-22 08:49:18 PDT
Comment on attachment 86462 [details]
Check webPageClient before referencing it

Clearing flags on attachment: 86462

Committed r81669: <http://trac.webkit.org/changeset/81669>
Comment 20 WebKit Commit Bot 2011-03-22 08:49:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2011-03-22 11:39:38 PDT
http://trac.webkit.org/changeset/81669 might have broken GTK Linux 32-bit Debug
The following tests are not passing:
inspector/debugger/debug-inlined-scripts.html