Bug 107178 – [EFL][Qt][WebGl] Random crash in GraphicsContext3D::drawArrays

• Component: WebGL
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

0	??			0xfffffffe	
1	pipe_resource_reference	u_inlines.h	123	0xac07f29f	
2	lp_setup_set_fragment_sampler_views	lp_setup.c	663	0xac07f29f	
3	llvmpipe_update_derived	lp_state_derived.c	180	0xac086772	
4	llvmpipe_draw_vbo	lp_draw_arrays.c	64	0xac071185	
5	st_draw_vbo	st_draw.c	1128	0xac13792a	
6	vbo_draw_arrays	vbo_exec_array.c	600	0xac2265c6	
7	WebCore::GraphicsContext3D::drawArrays	GraphicsContext3DOpenGLCommon.cpp	559	0xb4394cb8	
8	WebCore::TextureMapperGL::drawUnitRect	TextureMapperGL.cpp	478	0xb439f410	
9	WebCore::TextureMapperGL::draw	TextureMapperGL.cpp	500	0xb439f5fd	
10	WebCore::TextureMapperGL::drawTexturedQuadWithProgram	TextureMapperGL.cpp	535	0xb439f910	
11	WebCore::TextureMapperGL::drawTexture	TextureMapperGL.cpp	418	0xb439efc5	
12	WebCore::TextureMapperGL::drawTexture	TextureMapperGL.cpp	390	0xb439ee26	
13	WebCore::TextureMapperTile::paint	TextureMapperBackingStore.cpp	105	0xb38c281b	
14	WebKit::CoordinatedBackingStore::paintTilesToTextureMapper	CoordinatedBackingStore.cpp	126	0xb0801f9a	
15	WebKit::CoordinatedBackingStore::paintToTextureMapper	CoordinatedBackingStore.cpp	174	0xb080240d	
16	WebCore::TextureMapperLayer::paintSelf	TextureMapperLayer.cpp	138	0xb38c5cf6	
17	WebCore::TextureMapperLayer::paintSelfAndChildren	TextureMapperLayer.cpp	161	0xb38c5ea2	
18	WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica	TextureMapperLayer.cpp	287	0xb38c6615	
19	WebCore::TextureMapperLayer::paintRecursive	TextureMapperLayer.cpp	330	0xb38c6720	
20	WebCore::TextureMapperLayer::paintSelfAndChildren	TextureMapperLayer.cpp	171	0xb38c5fad	
21	WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica	TextureMapperLayer.cpp	287	0xb38c6615	
22	WebCore::TextureMapperLayer::paintRecursive	TextureMapperLayer.cpp	330	0xb38c6720	
23	WebCore::TextureMapperLayer::paintSelfAndChildren	TextureMapperLayer.cpp	171	0xb38c5fad	
24	WebCore::TextureMapperLayer::paintSelfAndChildrenWithReplica	TextureMapperLayer.cpp	287	0xb38c6615	
25	WebCore::TextureMapperLayer::paintRecursive	TextureMapperLayer.cpp	330	0xb38c6720	
26	WebCore::TextureMapperLayer::paint	TextureMapperLayer.cpp	104	0xb38c592d	
27	WebKit::LayerTreeRenderer::paintToCurrentGLContext	LayerTreeRenderer.cpp	131	0xb08104b1	
28	EwkViewImpl::displayTimerFired	EwkViewImpl.cpp	402	0xb095b77c	
29	WebCore::Timer<EwkViewImpl>::fired	Timer.h	106	0xb0962e2c	
30	WebCore::ThreadTimers::sharedTimerFiredInternal	ThreadTimers.cpp	116	0xb381993f	
31	WebCore::ThreadTimers::sharedTimerFired	ThreadTimers.cpp	93	0xb3819863	
32	WebCore::timerEvent	SharedTimerEfl.cpp	52	0xb432af8b	
33	_ecore_call_task_cb	ecore_private.h	267	0xaff326d6	
34	_ecore_timer_expired_call	ecore_timer.c	792	0xaff326d6	
35	_ecore_timer_expired_timers_call	ecore_timer.c	746	0xaff32894	
36	_ecore_main_loop_iterate_internal	ecore_main.c	1813	0xaff2f19b	
37	ecore_main_loop_begin	ecore_main.c	956	0xaff2f8af	
38	WTR::TestController::platformRunUntil	TestControllerEfl.cpp	75	0x807797d	
39	WTR::TestController::runUntil	TestController.cpp	766	0x8063edf	
40	WTR::TestInvocation::invoke	TestInvocation.cpp	228	0x806a4ea	
41	WTR::TestController::runTest	TestController.cpp	706	0x8063c4b	
42	WTR::TestController::run	TestController.cpp	739	0x8063e2d	
43	WTR::TestController::TestController	TestController.cpp	111	0x8061975	
44	main	main.cpp	49	0x8077ac7

[Viatcheslav Ostapenko]

It crashes because canvas texture is created from glx pixmap that is created on different glx connection. The display connection is closed when pixmap and texture are destroyed, but internally llvm pipe objects can be deleted later and they still hold pointers to already deallocated screen objects.

[Viatcheslav Ostapenko]

Created attachment 183293 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 183293 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Workaround for problem in mesa when internal llvm pipe object is deleted
> +        later then screen object. Screen object is deleted because corresponding 

later than* the*
because the* corresponding

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:148
> +    struct DisplayStatic {

I don't like this name

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:168
> +        static DisplayStatic displayStatic;
> +        return displayStatic.display();

// Display connection will only be broken at program shutdown.
static DisplayConnection connection;
return connection.display() ?

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:249
> -        , m_xPixmap(0)
> +        :  m_xPixmap(0)

wrong indentation

[Kalyan]

Comment on attachment 183293 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-198
> -        : m_display(m_offScreenWindow.display())

Minor one: Why not use the member variable as before, the display connection is guaranteed to be alive for app life time??

> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-237
> -        : m_display(m_offScreenWindow.display())

same here

[Kalyan]

(In reply to comment #2)
> Created an attachment (id=183293) [details]
> Patch

I took the patch into use in my local build and seems WebGL is broken after reloading the WebPage.

Did u have any similar issue?

[Viatcheslav Ostapenko]

Created attachment 183471 [details]
Patch

[Viatcheslav Ostapenko]

Comment on attachment 183293 [details]
Patch

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

>> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-198
>> -        : m_display(m_offScreenWindow.display())
> 
> Minor one: Why not use the member variable as before, the display connection is guaranteed to be alive for app life time??

What for?
All display() calls will be inlined and convert to the direct call to displayConnection::display(), which also will be inlined.
Extra m_display member is extra memory. Not much, but it adds up.
Also, all this m_display everywhere break general common sense rule to program for API. If there is display() API, m_display member normally shouldn't be used outside initialization and destruction.

[Kalyan]

(In reply to comment #7)
> (From update of attachment 183293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183293&action=review
> 
> >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-198
> >> -        : m_display(m_offScreenWindow.display())
> > 
> > Minor one: Why not use the member variable as before, the display connection is guaranteed to be alive for app life time??
> 
> What for?
> All display() calls will be inlined and convert to the direct call to displayConnection::display(), which also will be inlined.
> Extra m_display member is extra memory. Not much, but it adds up.
> Also, all this m_display everywhere break general common sense rule to program for API. If there is display() API, m_display member normally shouldn't be used outside initialization and destruction.

LGTM, if we can ensure that this doesn't break anything(Comment 5).

[Viatcheslav Ostapenko]

(In reply to comment #5)
> (In reply to comment #2)
> > Created an attachment (id=183293) [details] [details]
> > Patch
> 
> I took the patch into use in my local build and seems WebGL is broken after reloading the WebPage.
> 
> Did u have any similar issue?

No, I don't have this issue.
It passes most of the webgl tests (crashes only on type conversion) and works both on EFL and Qt.
How is it broken? Does it crash or do you have some other issue.

[Viatcheslav Ostapenko]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 183293 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183293&action=review
> > 
> > >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-198
> > >> -        : m_display(m_offScreenWindow.display())
> > > 
> > > Minor one: Why not use the member variable as before, the display connection is guaranteed to be alive for app life time??
> > 
> > What for?
> > All display() calls will be inlined and convert to the direct call to displayConnection::display(), which also will be inlined.
> > Extra m_display member is extra memory. Not much, but it adds up.
> > Also, all this m_display everywhere break general common sense rule to program for API. If there is display() API, m_display member normally shouldn't be used outside initialization and destruction.
> 
> LGTM, if we can ensure that this doesn't break anything(Comment 5).

I'll try to check after updating my builds. So far I didn't see any issues.

[Kalyan]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 183293 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=183293&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-198
> > > >> -        : m_display(m_offScreenWindow.display())
> > > > 
> > > > Minor one: Why not use the member variable as before, the display connection is guaranteed to be alive for app life time??
> > > 
> > > What for?
> > > All display() calls will be inlined and convert to the direct call to displayConnection::display(), which also will be inlined.
> > > Extra m_display member is extra memory. Not much, but it adds up.
> > > Also, all this m_display everywhere break general common sense rule to program for API. If there is display() API, m_display member normally shouldn't be used outside initialization and destruction.
> > 
> > LGTM, if we can ensure that this doesn't break anything(Comment 5).
> 
> I'll try to check after updating my builds. So far I didn't see any issues.

Try any WebGL Demo.

For example load: https://www.khronos.org/registry/webgl/sdk/demos/webkit/WebGL+CSS.html

Once pot is visible try to refresh the page. Pot doesn't come up anymore.

[Viatcheslav Ostapenko]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (From update of attachment 183293 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=183293&action=review
> > > > 
> > > > >> Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp:-198
> > > > >> -        : m_display(m_offScreenWindow.display())
> > > > > 
> > > > > Minor one: Why not use the member variable as before, the display connection is guaranteed to be alive for app life time??
> > > > 
> > > > What for?
> > > > All display() calls will be inlined and convert to the direct call to displayConnection::display(), which also will be inlined.
> > > > Extra m_display member is extra memory. Not much, but it adds up.
> > > > Also, all this m_display everywhere break general common sense rule to program for API. If there is display() API, m_display member normally shouldn't be used outside initialization and destruction.
> > > 
> > > LGTM, if we can ensure that this doesn't break anything(Comment 5).
> > 
> > I'll try to check after updating my builds. So far I didn't see any issues.
> 
> Try any WebGL Demo.
> 
> For example load: https://www.khronos.org/registry/webgl/sdk/demos/webkit/WebGL+CSS.html
> 
> Once pot is visible try to refresh the page. Pot doesn't come up anymore.

Very strange. I have reload problem even without my patch.
Jelly fishes (http://aleksandarrodic.com/p/jellyfish/) have the same problem.
Also webgl-composite-modes and webgl-composite-modes-repaint tests sometimes don't show anything, sometimes webproces crash. After webprocess crash next reload shows normally.
Seems to be new regression.

[Kalyan]

(In reply to comment #12)
> Seems to be new regression.

>>Very strange. I have reload problem even without my patch.
>>Jelly fishes (http://aleksandarrodic.com/p/jellyfish/) have the same problem.
>>Also webgl-composite-modes and webgl-composite-modes-repaint tests sometimes >>don't show anything, sometimes webproces crash. After webprocess crash next >>reload shows normally.
>>Seems to be new regression.

k, I didnt see any issues if I didnt take this patch into use. That's why I added the comment here. I was having a clean build with the patch from 106878 and this one. We do seem to have a regression though which is not related to this patch. The regression seems to be that resizing the Window breaks some demos, I think the case is when canvas(not viewport) is resized. JellyFish draws fine but doesn't draw anything if Window is resized. I will create a new bug for this.

[Kalyan]

(In reply to comment #13)
> (In reply to comment #12)
> > Seems to be new regression.
> 
> k, I didnt see any issues if I didnt take this patch into use. That's why I >added the comment here. I was having a clean build with the patch from 106878 >and this one. We do seem to have a regression though which is not related to >this patch. The regression seems to be that resizing the Window breaks some >demos, I think the case is when canvas(not viewport) is resized. JellyFish >draws fine but doesn't draw anything if Window is resized. I will create a new >bug for this.

Can you check after taking 106878 into use ??

[Kalyan]

(In reply to comment #13)
> (In reply to comment #12)
> > Seems to be new regression.
> Also webgl-composite-modes and webgl-composite-modes-repaint tests sometimes > > don't show anything, sometimes webproces crash. After webprocess crash next 
> reload shows normally.
Seems to be new regression.
>  I think the case is when canvas(not viewport) is resized. JellyFish draws >fine but doesn't draw anything if Window is resized. I will create a new bug >for this.

k, I found reason for the regression. This is being tracked in 107366

[Kalyan]

(In reply to comment #15)
> (In reply to comment #13)
> k, I found reason for the regression. This is being tracked in 107366

After the fix from 107366, I don't see any regressions with this patch. Sorry for the false alarm.

[WebKit Review Bot]

Comment on attachment 183471 [details]
Patch

Rejecting attachment 183471 [details] from commit-queue.

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

Last 500 characters of output:
 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp
Hunk #11 FAILED at 442.
1 out of 11 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/surfaces/glx/GraphicsSurfaceGLX.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Noam Rosenthal']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

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

[Viatcheslav Ostapenko]

Created attachment 183796 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183796 [details]
Patch for landing

Clearing flags on attachment: 183796

Committed r140342: <http://trac.webkit.org/changeset/140342>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.