Bug 107178

Summary: [EFL][Qt][WebGl] Random crash in GraphicsContext3D::drawArrays
Product: WebKit Reporter: Viatcheslav Ostapenko <ostap73>
Component: WebGLAssignee: Viatcheslav Ostapenko <ostap73>
Status: RESOLVED FIXED    
Severity: Normal CC: kalyan.kondapally, kenneth, noam, webkit.review.bot, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 104506    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Viatcheslav Ostapenko
Reported 2013-01-17 14:21:03 PST
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
Attachments
Patch (12.38 KB, patch)
2013-01-17 15:13 PST, Viatcheslav Ostapenko 		no flags
DetailsFormatted DiffDiff
Patch (12.51 KB, patch)
2013-01-18 08:59 PST, Viatcheslav Ostapenko 		no flags
DetailsFormatted DiffDiff
Patch for landing (12.60 KB, patch)
2013-01-21 09:18 PST, Viatcheslav Ostapenko 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Viatcheslav Ostapenko
Comment 1 2013-01-17 14:26:14 PST
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
Comment 2 2013-01-17 15:13:51 PST
Created attachment 183293 [details] Patch
Kenneth Rohde Christiansen
Comment 3 2013-01-18 07:14:27 PST
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 4 2013-01-18 08:46:16 PST
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
Comment 5 2013-01-18 08:50:19 PST
(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
Comment 6 2013-01-18 08:59:23 PST
Created attachment 183471 [details] Patch
Viatcheslav Ostapenko
Comment 7 2013-01-18 09:13:02 PST
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
Comment 8 2013-01-18 10:50:24 PST
(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
Comment 9 2013-01-18 12:52:48 PST
(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
Comment 10 2013-01-18 12:54:25 PST
(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
Comment 11 2013-01-18 14:30:12 PST
(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
Comment 12 2013-01-18 21:06:52 PST
(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
Comment 13 2013-01-19 00:17:07 PST
(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
Comment 14 2013-01-19 00:20:08 PST
(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
Comment 15 2013-01-19 05:36:30 PST
(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
Comment 16 2013-01-19 07:01:19 PST
(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 17 2013-01-21 09:00:51 PST
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
Comment 18 2013-01-21 09:18:07 PST
Created attachment 183796 [details] Patch for landing
WebKit Review Bot
Comment 19 2013-01-21 09:58:34 PST
Comment on attachment 183796 [details] Patch for landing Clearing flags on attachment: 183796 Committed r140342: <http://trac.webkit.org/changeset/140342>
WebKit Review Bot
Comment 20 2013-01-21 09:58:39 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.