Summary: | [EFL][Qt][WebGl] Random crash in GraphicsContext3D::drawArrays | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Viatcheslav Ostapenko <ostap73> | ||||||||
Component: | WebGL | Assignee: | 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
Viatcheslav Ostapenko
2013-01-17 14:21:03 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. Created attachment 183293 [details]
Patch
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 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 (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? Created attachment 183471 [details]
Patch
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. (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). (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. (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. (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. (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. (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. (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 ?? (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 (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. 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 Created attachment 183796 [details]
Patch for landing
Comment on attachment 183796 [details] Patch for landing Clearing flags on attachment: 183796 Committed r140342: <http://trac.webkit.org/changeset/140342> All reviewed patches have been landed. Closing bug. |