Bug 74176 - [Qt][WK2]REGRESSION(r102435): It made tst_QQuickWebView::show() crash
Summary: [Qt][WK2]REGRESSION(r102435): It made tst_QQuickWebView::show() crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Viatcheslav Ostapenko
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 70236 73591
  Show dependency treegraph
 
Reported: 2011-12-09 04:33 PST by Csaba Osztrogonác
Modified: 2012-01-13 05:44 PST (History)
7 users (show)

See Also:


Attachments
"Normal" X server glxinfo run log. (18.39 KB, text/plain)
2011-12-13 15:04 PST, Viatcheslav Ostapenko
no flags Details
Xvfb server glxinfo run log. (19.26 KB, text/plain)
2011-12-13 15:05 PST, Viatcheslav Ostapenko
no flags Details
xvfb glxinfo run log from the bot (18.62 KB, text/plain)
2011-12-14 02:24 PST, Csaba Osztrogonác
no flags Details
Patch (15.50 KB, patch)
2012-01-08 15:47 PST, Viatcheslav Ostapenko
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Use QGLContext compatible with previous Qt version (15.51 KB, patch)
2012-01-08 17:39 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Added multiWindow API test and other patch fixes. (17.33 KB, patch)
2012-01-09 16:29 PST, Viatcheslav Ostapenko
noam: review-
Details | Formatted Diff | Diff
Updated patch (19.57 KB, patch)
2012-01-10 08:53 PST, Viatcheslav Ostapenko
noam: review-
Details | Formatted Diff | Diff
Updated patch (20.84 KB, patch)
2012-01-10 14:13 PST, Viatcheslav Ostapenko
noam: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix changelog mess. (20.80 KB, patch)
2012-01-10 15:40 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2011-12-09 04:33:39 PST
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()
Comment 1 Csaba Osztrogonác 2011-12-09 06:52:03 PST
+info: it crashes in xvfb, but passes with X forward.
Comment 2 Csaba Osztrogonác 2011-12-12 03:09:13 PST
(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/
Comment 3 Viatcheslav Ostapenko 2011-12-13 15:04:22 PST
Created attachment 119092 [details]
"Normal" X server glxinfo run log.
Comment 4 Viatcheslav Ostapenko 2011-12-13 15:05:28 PST
Created attachment 119093 [details]
Xvfb server glxinfo run log.
Comment 5 Viatcheslav Ostapenko 2011-12-13 15:08:20 PST
(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.
Comment 6 Csaba Osztrogonác 2011-12-14 02:24:52 PST
Created attachment 119184 [details]
xvfb glxinfo run log from the bot
Comment 7 Viatcheslav Ostapenko 2012-01-08 15:27:57 PST
(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
Comment 8 Viatcheslav Ostapenko 2012-01-08 15:47:18 PST
Created attachment 121596 [details]
Patch

Ossy, please, could you take a look at this?
Comment 9 Early Warning System Bot 2012-01-08 16:00:50 PST
Comment on attachment 121596 [details]
Patch

Attachment 121596 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11113272
Comment 10 Viatcheslav Ostapenko 2012-01-08 17:39:21 PST
Created attachment 121605 [details]
Use QGLContext compatible with previous Qt version
Comment 11 Noam Rosenthal 2012-01-08 17:56:23 PST
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
Comment 12 Noam Rosenthal 2012-01-08 17:57:29 PST
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
Comment 13 Viatcheslav Ostapenko 2012-01-09 16:29:08 PST
Created attachment 121755 [details]
Added multiWindow API test and other patch fixes.
Comment 14 Noam Rosenthal 2012-01-09 16:37:41 PST
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.
Comment 15 Viatcheslav Ostapenko 2012-01-09 17:59:50 PST
(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?
Comment 16 Noam Rosenthal 2012-01-09 19:17:37 PST
> > 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.
Comment 17 Noam Rosenthal 2012-01-09 19:18:07 PST
(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.
Comment 18 Martin Robinson 2012-01-09 20:27:52 PST
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?
Comment 19 Martin Robinson 2012-01-09 23:14:40 PST
(In reply to comment #18)

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

Oops. Didn't see Noam's comment. Sorry!
Comment 20 Viatcheslav Ostapenko 2012-01-10 08:53:47 PST
Created attachment 121857 [details]
Updated patch
Comment 21 Noam Rosenthal 2012-01-10 09:28:12 PST
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.
Comment 22 Noam Rosenthal 2012-01-10 09:28:35 PST
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
Comment 23 Viatcheslav Ostapenko 2012-01-10 14:13:27 PST
Created attachment 121907 [details]
Updated patch
Comment 24 WebKit Review Bot 2012-01-10 15:20:37 PST
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
Comment 25 Viatcheslav Ostapenko 2012-01-10 15:40:30 PST
Created attachment 121921 [details]
Fix changelog mess.
Comment 26 WebKit Review Bot 2012-01-10 16:42:25 PST
Comment on attachment 121921 [details]
Fix changelog mess.

Clearing flags on attachment: 121921

Committed r104651: <http://trac.webkit.org/changeset/104651>
Comment 27 WebKit Review Bot 2012-01-10 16:42:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Simon Hausmann 2012-01-13 05:29:10 PST
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.
Comment 29 Simon Hausmann 2012-01-13 05:44:52 PST
(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.