Bug 74176 – [Qt][WK2]REGRESSION(r102435): It made tst_QQuickWebView::show() crash

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

QFATAL : tst_QQuickWebView::show() Received signal 11
FAIL!  : tst_QQuickWebView::show() Received a fatal error.

This crash hides 2 other test:
PASS   : tst_QQuickWebView::showWebView()
PASS   : tst_QQuickWebView::cleanupTestCase()

[Csaba Osztrogonác]

+info: it crashes in xvfb, but passes with X forward.

[Csaba Osztrogonác]

(In reply to comment #1)
> +info: it crashes in xvfb, but passes with X forward.

I meant xvfb-run -a --server-args="-screen 0 1024x768x24" Tools/Scripts/run-qtwebkit-tests --output-file=qt-unit-tests.html --do-not-open-results --timeout=120 WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/

[Viatcheslav Ostapenko]

Created attachment 119092 [details]
"Normal" X server glxinfo run log.

[Viatcheslav Ostapenko]

Created attachment 119093 [details]
Xvfb server glxinfo run log.

[Viatcheslav Ostapenko]

(In reply to comment #2)
> (In reply to comment #1)
> > +info: it crashes in xvfb, but passes with X forward.
> 
> I meant xvfb-run -a --server-args="-screen 0 1024x768x24" Tools/Scripts/run-qtwebkit-tests --output-file=qt-unit-tests.html --do-not-open-results --timeout=120 WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/

It seems Xvfb just doesn't support shaders. Check attached logs.


Could you run

xvfb-run -a --server-args="-screen 0 1024x768x24" glxinfo

on build bot and attach log here?

Thanks.

[Csaba Osztrogonác]

Created attachment 119184 [details]
xvfb glxinfo run log from the bot

[Viatcheslav Ostapenko]

(In reply to comment #6)
> Created an attachment (id=119184) [details]
> xvfb glxinfo run log from the bot

1. xvfb GL support on ubuntu depends on mesa, which is missing mesa GLX if HW nvidia or ATI drivers installed. The only way to run this correctly is to remove HW drivers and use mesa GLX implementation.
2. Deletion of test window in API test also cause destruction of GL context making all global GL objects in TextureMapperGL invalid. Also, current implementation will not work with multiple windows (because they will have multiple canvases and multiple GL contexts).
Here is QOpenGLContest destruction call stack:
0	QOpenGLContext::~QOpenGLContext	qopenglcontext.cpp	193	0xb660e919	
1	QOpenGLContext::~QOpenGLContext	qopenglcontext.cpp	194	0xb660e983	
2	QQuickTrivialWindowManager::hide	qquickwindowmanager.cpp	1123	0xb7447aab	
3	QQuickTrivialWindowManager::canvasDestroyed	qquickwindowmanager.cpp	1130	0xb7447ae5	
4	QQuickCanvas::~QQuickCanvas	qquickcanvas.cpp	635	0xb7362a00	
5	QQuickView::~QQuickView	qquickview.cpp	178	0xb742119f	
6	TestWindow::~TestWindow	testwindow.h	29	0x8051f27	
7	TestWindow::~TestWindow	testwindow.h	29	0x8051f77	
8	QScopedPointerDeleter<TestWindow>::cleanup	qscopedpointer.h	62	0x8051fa2	
9	QScopedPointer<TestWindow, QScopedPointerDeleter<TestWindow> >::reset	qscopedpointer.h	149	0x80517c9	
10	tst_QQuickWebView::cleanup	tst_qquickwebview.cpp	73	0x804ea90	
11	tst_QQuickWebView::qt_static_metacall	tst_qquickwebview.moc	74	0x8050a24	
12	QMetaMethod::invoke	qmetaobject.cpp	1634	0xb630b991	
13	QMetaMethod::invoke	qmetaobject.h	120	0xb6a4167f	
14	QTest::invokeMethod	qtestcase.cpp	998	0xb6a3b927	
15	QTest::qInvokeTestMethodDataEntry	qtestcase.cpp	1505	0xb6a3dafa	
16	QTest::qInvokeTestMethod	qtestcase.cpp	1605	0xb6a3e217	
17	QTest::qInvokeTestMethods	qtestcase.cpp	1759	0xb6a3e9b2	
18	QTest::qExec	qtestcase.cpp	1972	0xb6a3ee77	
19	main	tst_qquickwebview.cpp	287	0x8050957

[Viatcheslav Ostapenko]

Created attachment 121596 [details]
Patch

Ossy, please, could you take a look at this?

[Early Warning System Bot]

Comment on attachment 121596 [details]
Patch

Attachment 121596 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11113272

[Viatcheslav Ostapenko]

Created attachment 121605 [details]
Use QGLContext compatible with previous Qt version

[Noam Rosenthal]

Comment on attachment 121605 [details]
Use QGLContext compatible with previous Qt version

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

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:43
> +#include <GL/glx.h>

This needs to be inside OS(UNIX) or OS(LINUX) or something like that.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:217
> +            GLContextDataMap::const_iterator mapEnd =  glContextDataMap().end();

Double space

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:219
> +            for (i = glContextDataMap().begin(); i != mapEnd; ++i)

Curly braces

[Noam Rosenthal]

Can't we make it simpler, by making this GL data non-static, and not try to share it between TextureMapperGL instances in the same GL context? It seems a bit superfluous

[Viatcheslav Ostapenko]

Created attachment 121755 [details]
Added multiWindow API test and other patch fixes.

[Noam Rosenthal]

Comment on attachment 121755 [details]
Added multiWindow API test and other patch fixes.

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

On the right tracks, but meeds some improvements...

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:127
> +#else

if OS(LINUX/UNIX) ???

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:201
> +            GLuint shaders[4];

Why 4?

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:230
> +            GLContextDataMap::iterator i;

it instead of i

> Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:276
> +    showWebView();

The right test for this is not multiple top-level windows, but rather multiple web-views in the same scene.

[Viatcheslav Ostapenko]

(In reply to comment #14)
> (From update of attachment 121755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121755&action=review
> 
> On the right tracks, but meeds some improvements...
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:127
> > +#else
> 
> if OS(LINUX/UNIX) ???

Without any default it will fail build in any case. I can make some default  GLContext/getCurrentGLContext returning incrementing integer this way disabling any sharing. Would it work?

> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:201
> > +            GLuint shaders[4];
> 
> Why 4?

Should be 2.
I had some problems and was guessing around that there are no other shaders or something and forgot to change back. Or may be it would be better just to save shader id's?

> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:230
> > +            GLContextDataMap::iterator i;
> 
> it instead of i

Ok

> > Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:276
> > +    showWebView();
> 
> The right test for this is not multiple top-level windows, but rather multiple web-views in the same scene.

Actually, in this case it will be single GL context.
But we are missing multi-view test also.
What you prefer - add multiple views to this test or created separate test with multiple views on one window?

[Noam Rosenthal]

> > if OS(LINUX/UNIX) ???
> 
> Without any default it will fail build in any case. I can make some default  GLContext/getCurrentGLContext returning incrementing integer this way disabling any sharing. Would it work?
Yes, I don't see why not.

> I had some problems and was guessing around that there are no other shaders or something and forgot to change back. Or may be it would be better just to save shader id's?
Yes, probably. That code seems unnecessarily complex.

> > > Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:276
> > > +    showWebView();
> > 
> > The right test for this is not multiple top-level windows, but rather multiple web-views in the same scene.
> 
> Actually, in this case it will be single GL context.
> But we are missing multi-view test also.
> What you prefer - add multiple views to this test or created separate test with multiple views on one window?
Have two tests together with this patch, multipleWebViewsInSingleWindow and multipleWebViewWindows.

[Noam Rosenthal]

(In reply to comment #16)
> > > if OS(LINUX/UNIX) ???
> > 
btw UNIX/LINUX doesn't necessarily mean GLX... we have to account for EGL as well.

[Martin Robinson]

Comment on attachment 121755 [details]
Added multiWindow API test and other patch fixes.

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

>>> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:127
>>> +#else
>> 
>> if OS(LINUX/UNIX) ???
> 
> Without any default it will fail build in any case. I can make some default  GLContext/getCurrentGLContext returning incrementing integer this way disabling any sharing. Would it work?

Won't this fail for systems using EGL instead of GLX?

[Martin Robinson]

(In reply to comment #18)

> Won't this fail for systems using EGL instead of GLX?

Oops. Didn't see Noam's comment. Sorry!

[Viatcheslav Ostapenko]

Created attachment 121857 [details]
Updated patch

[Noam Rosenthal]

Comment on attachment 121857 [details]
Updated patch

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

Some nitpicks, almost there.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:249
> +            GLContextDataMap::const_iterator mapEnd = glContextDataMap().end();

Use end instead of mapEnd

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:408
>  void TextureMapperGL::initializeShaders()

I think we should move this function into SharedData constructor, would save us lots of code.

[Noam Rosenthal]

Comment on attachment 121857 [details]
Updated patch

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

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:160
> +        static PassRefPtr<SharedGLData> curentSharedGLData()

curent -> current

[Viatcheslav Ostapenko]

Created attachment 121907 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 121907 [details]
Updated patch

Rejecting attachment 121907 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
kit-commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 7 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
45>At revision 7.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11141659

[Viatcheslav Ostapenko]

Created attachment 121921 [details]
Fix changelog mess.

[WebKit Review Bot]

Comment on attachment 121921 [details]
Fix changelog mess.

Clearing flags on attachment: 121921

Committed r104651: <http://trac.webkit.org/changeset/104651>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Comment on attachment 121921 [details]
Fix changelog mess.

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

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:122
> +        static GLContext getCurrentGLContext()
> +        {
> +            return eglGetCurrentContext();
> +        }

The use of functions from libEGL also requires changes to the .pro files to link in libEGL. This is missing at the moment and prevent builds on platforms where EGL is used.

[Simon Hausmann]

(In reply to comment #28)
> (From update of attachment 121921 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121921&action=review
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:122
> > +        static GLContext getCurrentGLContext()
> > +        {
> > +            return eglGetCurrentContext();
> > +        }
> 
> The use of functions from libEGL also requires changes to the .pro files to link in libEGL. This is missing at the moment and prevent builds on platforms where EGL is used.

Filed bug #76268.