Summary: | [Qt] QWebPage with WebGL content crashes when rendering if no QWebView parent | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrew Wason <rectalogic> | ||||||||||||||
Component: | WebGL | Assignee: | 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
Andrew Wason
2011-02-09 13:24:08 PST
Created attachment 81861 [details]
gdb backtrace when running client_crash test app
Created attachment 81863 [details]
Check webPageClient before referencing it
Comment on attachment 81863 [details]
Check webPageClient before referencing it
This should really have an autotest.
Created attachment 81911 [details]
Check webPageClient before referencing it
Add unit test
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. (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? (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? Why is there no QWebPageClient set? (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. (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. (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. (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 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. This is also now listed in https://trac.webkit.org/wiki/QtWebKitWebGL Created attachment 86288 [details]
Check webPageClient before referencing it
Fix patch to take into account upstream changes.
Enable QWebSettings::AcceleratedCompositingEnabled in testcase.
Looks good to me. Thanks. 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. 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 on attachment 86462 [details] Check webPageClient before referencing it Clearing flags on attachment: 86462 Committed r81669: <http://trac.webkit.org/changeset/81669> All reviewed patches have been landed. Closing bug. 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 |