Bug 54138 – [Qt] QWebPage with WebGL content crashes when rendering if no QWebView parent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Andrew Wason]

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.

[Andrew Wason]

Created attachment 81861 [details]
gdb backtrace when running client_crash test app

[Andrew Wason]

Created attachment 81863 [details]
Check webPageClient before referencing it

[Benjamin Poulain]

Comment on attachment 81863 [details]
Check webPageClient before referencing it

This should really have an autotest.

[Andrew Wason]

Created attachment 81911 [details]
Check webPageClient before referencing it

Add unit test

[Antonio Gomes]

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.

[Andrew Wason]

(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?

[Benjamin Poulain]

(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?

[Andreas Kling]

Why is there no QWebPageClient set?

[Andrew Wason]

(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.

[Benjamin Poulain]

(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.

[Andrew Wason]

(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.

[Andrew Wason]

(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

[Jarkko Sakkinen]

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.

[Jarkko Sakkinen]

This is also now listed in https://trac.webkit.org/wiki/QtWebKitWebGL

[Andrew Wason]

Created attachment 86288 [details]
Check webPageClient before referencing it

Fix patch to take into account upstream changes.
Enable QWebSettings::AcceleratedCompositingEnabled in testcase.

[Jarkko Sakkinen]

Looks good to me. Thanks.

[Benjamin Poulain]

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.

[Andrew Wason]

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().

[WebKit Commit Bot]

Comment on attachment 86462 [details]
Check webPageClient before referencing it

Clearing flags on attachment: 86462

Committed r81669: <http://trac.webkit.org/changeset/81669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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